Skip to content

perf: write periodic station progress off the main thread - #33

Merged
XxFran10xX merged 3 commits into
mainfrom
perf/batched-durable-saves
Oct 30, 2026
Merged

XxFran10xX merged 3 commits into
mainfrom
perf/batched-durable-saves

Conversation

@XxFran10xX

Copy link
Copy Markdown
Contributor

Why

Main profiles, 22 hourly spark dumps 10-29 07:00 → 10-30 05:00 (peak ~30 players, ~23 ms/tick total). StationManager$1.run → Database.saveStationsNow fsynced on a main-thread timer: 25 s of main-thread fsync over the day.

Change

  • The 30 s progress save copies station state on the main thread and writes it on one background thread (VFBuilders station writer) with the same temp → fsync → atomic rename.
  • Each copy carries a sequence number, so a queued older copy never overwrites a newer synchronous save.
  • Construction start, completion, cancel, removal, reload and disable still save synchronously as before. Disable then drains the writer (30 s timeout with a warning).
  • Crash guarantees are unchanged: the file is always a complete old or new version, and at most ~30 s of countdown progress is lost.

Testing

  • StationWriterTest (6 new) and updated SchedulerTest; 61 pass. LifecycleTest fails only on Windows (POSIX permissions).
  • Dev (10-30, 11:55–16:47 UTC): all ten perf branches from this round ran together (InteractibleFurniture, Cooking, Magic, RPCharacters, Thievery, VFBuilders, SimpleFactions, Archaeo, VehicleFramework, TrialRooms). Bot probes ran on these jars and again on the previous jars for comparison. No errors from any of the ten plugins in the log, including through a scheduled restart. Dev has no active construction, so this covered startup, the periodic timer and two clean shutdowns. No errors and no leftover stations-*.tmp.

🤖 Generated with Claude Code

The 30-second progress save fsynced stations.json on the server thread
(about 25 s of main-thread fsync per day on Main). It now snapshots the
stations on the main thread and writes them with the same temp file,
fsync and atomic rename on a single station writer thread.

Every state change (construction start, completion, cancellation,
station removal, reload and disable) still saves synchronously and
reports failure as before. Each snapshot carries a sequence number and
a queued snapshot older than one already written is dropped, so a
background write can never put back state older than a synchronous
checkpoint. Disable saves synchronously, then drains and stops the
writer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bb99e95c-935c-48bd-96b3-0d738ae2f9a5
Commits

Reviewing files that changed from the base of the PR and between d3349dd and 05db267.

Files selected for processing: 5
  • src/main/java/net/tfminecraft/vfbuilders/data/Database.java
  • src/main/java/net/tfminecraft/vfbuilders/managers/StationManager.java
  • src/test/java/net/tfminecraft/vfbuilders/DatabaseFixture.java
  • src/test/java/net/tfminecraft/vfbuilders/SchedulerTest.java
  • src/test/java/net/tfminecraft/vfbuilders/data/StationWriterTest.java

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Improvements
    • Station data is now saved in the background during periodic saves, helping keep the application responsive.
    • Saves are written in order, and outdated snapshots are skipped to prevent older data from replacing newer data.
    • Station files are resolved from the plugin’s current data folder, and saves use a temporary file before replacing the existing one.
    • On shutdown, pending station data is saved before the database writer closes.

Station persistence now resolves the data file when needed and supports queued snapshot writes. The station manager uses queued writes for periodic saves and closes the database after saving station state during shutdown.

Changes: Station persistence

Layer / File(s) Summary
Snapshot writing and writer lifecycle
src/main/java/net/tfminecraft/vfbuilders/data/Database.java, src/test/java/net/tfminecraft/vfbuilders/data/StationWriterTest.java, src/test/java/net/tfminecraft/vfbuilders/DatabaseFixture.java
Database resolves the station file at call time, captures snapshots, and supports synchronous and queued writes. It serialises writes and provides writer shutdown. Tests cover snapshot ordering, failures, shutdown, timeout, interruption, and restart.
Periodic saves and shutdown
src/main/java/net/tfminecraft/vfbuilders/managers/StationManager.java, src/test/java/net/tfminecraft/vfbuilders/SchedulerTest.java
The station manager queues periodic saves when a station has a blueprint and closes the database after saving state during shutdown. Scheduler tests check the queued save and shutdown order.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant StationManager
  participant Database
  participant WriterExecutor
  participant StationsFile
  StationManager->>Database: saveStationsLater(stations)
  Database->>Database: Capture snapshot on calling thread
  Database->>WriterExecutor: Submit snapshot
  WriterExecutor->>StationsFile: Write snapshot
Loading
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size:M 30-99 effective changed lines (tests excluded in mixed PRs). label Oct 30, 2026
XxFran10xX and others added 2 commits October 30, 2026 17:57
Database fixes its file when the class first loads. On CI another test loads it
under its own temp folder first, so the writer tests read the fixture path and
found nothing. They now ask Database for the real path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Database fixed its path from whichever plugin data folder was set when the class
first loaded, so test classes broke each other depending on run order. The path
now comes from the plugin's data folder at save and load time; the background
writer gets it inside the snapshot taken on the calling thread. On the server the
data folder never changes, so saves and loads use the same file as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added size:L 100-499 effective changed lines (tests excluded in mixed PRs). and removed size:M 30-99 effective changed lines (tests excluded in mixed PRs). labels Oct 30, 2026
@XxFran10xX

Copy link
Copy Markdown
Contributor Author

CI found an order-dependent test failure, now fixed:

  • Database fixed its stations path from whichever plugin data folder was set when the class first loaded. Adding StationWriterTest changed surefire's class order on Linux, so first the new tests and then DatabaseTest read a different folder from the one written.
  • 05db267 resolves the path from the plugin's data folder at save and load time. The background writer gets it inside the snapshot taken on the calling thread. On the server the data folder never changes, so saves and loads use the same file as before.
  • Local: alphabetical, reverse and 3 random class orders all pass (only the Windows-only LifecycleTest fails, as on main). CI is green.
  • Dev retest at 17:05 UTC with the new jar: stations loaded from the same file (same md5), no leftover stations-*.tmp, no VFBuilders warnings.
  • Dev has no construction in progress. The 30 s timer only saves while a station has a blueprint (same condition as main), so the background write itself is covered by StationWriterTest, not by Dev.

@XxFran10xX
XxFran10xX marked this pull request as ready for review October 30, 2026 17:28
@XxFran10xX

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@XxFran10xX
XxFran10xX merged commit 145d450 into main Oct 30, 2026
5 checks passed
@XxFran10xX
XxFran10xX deleted the perf/batched-durable-saves branch October 30, 2026 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 effective changed lines (tests excluded in mixed PRs).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant