Skip to content

Improve code quality, reliability, and data safety - #2

Draft
aidenfarley wants to merge 1 commit into
masterfrom
agent/improve-code-quality-and-data-safety
Draft

Improve code quality, reliability, and data safety#2
aidenfarley wants to merge 1 commit into
masterfrom
agent/improve-code-quality-and-data-safety

Conversation

@aidenfarley

Copy link
Copy Markdown
Owner

Summary

This pull request completes the staged code-quality and reliability plan for ServerUtilities. It separates domain state from persistence and scheduling, hardens backup/restore behavior against partial writes and unsafe archive paths, improves runtime resource cleanup, preserves compatibility boundaries, and adds focused regression coverage.

This is intentionally a draft PR against aidenfarley/ServerUtilities:master for review on the fork. It does not target GTNewHorizons/ServerUtilities, and nothing is being merged upstream.

Motivation

The previous implementation had several recurring maintenance and reliability risks:

  • large mutable domain classes combined state ownership, lookup, persistence, and task scheduling;
  • callers could mutate internal player, team, universe, and claimed-chunk collections without consistently preserving invariants;
  • backup and restore operations had limited protection against archive traversal, interruption, partial replacement, and process failure;
  • NBT and world metadata writes were not consistently atomic or failure-aware;
  • networking and rendering paths did not always release resources or restore state on exceptional exits;
  • identifiers, UUIDs, locale-sensitive formatting, and fuzzy player lookup behavior were spread across call sites;
  • core contracts had little automated regression coverage or architecture documentation.

The goal of this work is not to redesign gameplay. It is to make the existing behavior easier to reason about, safer under failure, and more testable while retaining established data and protocol compatibility.

What changed

1. Domain state ownership and persistence boundaries

  • Extracted UniverseRepository, UniversePersistence, and UniverseTaskScheduler from Universe so lookup, disk I/O, and scheduled work have explicit responsibilities.
  • Extracted ForgeTeamMembership and ForgeTeamPersistence from ForgeTeam to isolate membership rules and NBT serialization.
  • Encapsulated mutable player, team, universe, and claim state behind read-only views and explicit domain mutation methods.
  • Added duplicate team ID and UID checks at registration boundaries instead of silently replacing existing entries.
  • Corrected ForgeTeam equality/hash-code behavior so equality is symmetric and includes universe scope.
  • Made exact and fuzzy player lookup deterministic, including explicit fake-player precedence.
  • Updated command, GUI, event, packet, and compatibility call sites to use the new mutation and lookup boundaries.

These changes reduce hidden coupling and make invariants enforceable at a small number of domain entry points.

2. Persistence and data-integrity hardening

  • Added checked, atomic NBT writes through sibling temporary files followed by replacement.
  • Retain dirty state when a write or delete fails so data is not incorrectly treated as persisted.
  • Log corrupt or unreadable persisted data instead of silently losing the failure context.
  • Made the single-player cheat toggle update level.dat atomically while preserving level.dat_old.
  • Added focused helpers for backup glob expansion, identifiers, locale-stable number formatting, and UUID parsing/formatting.
  • Preserved legacy formatting contracts where behavior is externally visible, including MOTD HALF_EVEN rounding and existing StringUtils edge cases.
  • Standardized try-with-resources usage, error logging, and interrupted-thread restoration across affected I/O paths.

3. Backup lifecycle and restore safety

  • Consolidated the Commons and legacy ZIP implementations under AbstractZipCompressor.
  • Centralized archive path validation in BackupArchivePathPolicy.
  • Reject archive entries that are absolute, traverse outside the destination, use linked paths, or fall outside the selected-world allowlist.
  • Treat $WORLDNAME substitutions literally when generating glob patterns.
  • Enforce archive entry-count, extracted-size, and available-disk-space limits before unsafe expansion can proceed.
  • Perform archive copies in interruption-aware byte chunks so cancellation can stop long operations cooperatively.
  • Added RestoreTransaction, which stages extraction before replacement and can roll back a partially applied restore.
  • Added a persistent, disk-forced restore journal and startup recovery through RestoreRecovery from client pre-initialization.
  • Protect restore journal, staging, and archive locations in both ancestor and descendant directions.
  • Added lifecycle tokens covering synchronous and asynchronous backups, immutable per-run save-state snapshots, and cooperative cancellation.
  • Validate custom backup names as safe path components.
  • Corrected force-only-claimed backup handling, including the zero-claimed-chunk case.

The resulting restore sequence is transactional at the application level: validate, stage, journal, replace, and either commit or recover/roll back.

4. Networking, HTTP, and runtime cleanup

  • Removed trust-all TLS behavior from HttpDataReader; normal JVM certificate validation is now used.
  • Added 15-second connection and 30-second read timeouts and guaranteed HTTP disconnect cleanup.
  • Updated the Aurora Netty handler to release FullHttpRequest objects, calculate response lengths from UTF-8 bytes, close error/non-keepalive connections, and clean up event loops after bind/start failures.
  • Ensured IconRenderer restores OpenGL state on all exits and reports queued callback failures accurately.
  • Improved resource cleanup and interruption handling in supporting runtime tasks.

5. Public APIs and compatibility contracts

  • Added a documented serverutils.api.ServerUtilitiesRegistry facade in the main artifact.
  • Retained and deprecated the legacy registry bridge rather than removing it.
  • Clarified that callback/action implementation types keep their existing compatibility status; this change does not claim a new standalone API-only artifact.
  • Added stable action identifiers and deterministic action lookup behavior.
  • Moved packet identity handling toward UUIDs while retaining legacy player-name fallback for compatibility.
  • Updated registry, action, packet, and serialization call sites to use the clarified contracts.

6. Documentation and tests

  • Added ARCHITECTURE.md describing ownership, persistence, backup/restore, networking, and API boundaries.
  • Added CONTRIBUTING.md with build, formatting, testing, and compatibility guidance.
  • Linked the new documentation from the README.
  • Added JUnit 5 configuration and dependencies.
  • Added 15 focused test classes with 54 test cases covering:
    • public registry behavior;
    • Aurora response handling;
    • atomic level.dat cheat changes;
    • restore transactions and recovery;
    • claimed-chunk mutation boundaries;
    • action identifiers and lookup;
    • team equality, membership, and persistence behavior;
    • universe repository and task scheduling behavior;
    • packet/data serialization;
    • NBT and string utility contracts;
    • both ZIP compressor implementations and path policy;
    • backup lifecycle state and threaded backup behavior.

Intentional behavior changes

  • Unsafe archive paths and linked extraction paths are rejected instead of being accepted.
  • Duplicate team identifiers fail at registration rather than replacing prior state.
  • Player lookup is deterministic when exact, fuzzy, real, and fake-player candidates overlap.
  • HTTP reads use certificate validation and finite timeouts.
  • A failed or interrupted persistence operation remains dirty and eligible for retry.
  • An incomplete restore journal is recovered during startup; recovery fails closed if consistency cannot be established.

Compatibility

  • Existing NBT keys, configuration names, and packet layouts are preserved unless a compatibility fallback is explicitly provided.
  • Legacy registry entry points remain available and are marked deprecated.
  • Legacy name-based packet identity remains available as a fallback while UUID identity is preferred.
  • The public registry facade ships in the normal mod artifact; no separate API JAR is introduced or promised.
  • No data migration or configuration migration is required.

Validation

  • \.\gradlew.bat spotlessCheck — passed
  • \.\gradlew.bat build — passed
  • Test result — 54 tests across 15 test classes, with 0 failures, 0 errors, and 0 skipped
  • git diff --check / staged whitespace validation — passed
  • Working tree was clean after commit creation

Suggested review order

  1. RestoreTransaction, RestoreRecovery, and BackupArchivePathPolicy for failure and path-safety semantics.
  2. Universe, UniverseRepository, UniversePersistence, ForgeTeam, and the extracted team helpers for ownership and compatibility.
  3. BackupTask, ThreadBackup, and lifecycle/snapshot classes for cancellation and save-state restoration.
  4. AuroraServer, HttpDataReader, and IconRenderer for resource lifecycle changes.
  5. The public registry facade, action/packet contracts, tests, and documentation.

Risk and rollback notes

This is a broad internal refactor, so the PR remains a draft for focused review and any additional integration testing. The highest-risk areas are restore replacement ordering, startup recovery, persistence failure handling, and compatibility call-site updates. The added tests concentrate on those boundaries.

There are no schema migrations. Rolling back consists of deploying the prior build. If a process terminates during a restore, the new journal is intentionally retained so the same build can recover the operation on startup.

Non-goals

  • No new gameplay features.
  • No upstream pull request or upstream merge.
  • No automatic merge into the fork's master branch.
  • No removal of legacy compatibility bridges.
  • No separate API-only publication.

Review checklist

  • Formatting checks pass
  • Full Gradle build passes
  • Regression tests pass
  • Architecture and contribution documentation added
  • Compatibility fallbacks retained
  • Maintainer review
  • Any desired manual/integration testing
  • Mark ready for review only after fork-level feedback is addressed

Refactor state ownership and persistence, harden backup/restore and networking lifecycles, and add regression coverage and architecture documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant