reliability(mainmenu): SafeWrite migration — 4 client-side save sites#289
Merged
Conversation
Closes the client-side counterpart to the #279 server/editor SafeWrite sweep. 4 sites in MainMenu.bb that wrote directly to production files now use the established atomic SafeWriteOpen/SafeWriteCommit pair, with .bak retention on a 1-cycle recovery window. ## Sites migrated (4) 1. **`Data\Last Username.dat`** (line 857, inside `LogIn()`) -- the username/password cache. WriteFile failure would leave the cache empty, locking the user out of the pre-fill on next login (forcing manual re-entry). 2. **`Data\Game Data\Misc.dat`** (line 1500, inside `LogIn()`) -- Music checkbox update. Empty Misc.dat would cause the next launch to read "no game name, no version" and fail project load. 3. **`Data\Game Data\Misc.dat`** (line 3694, inside `GameOptionsMenu()`) -- duplicate of site 2 in the in-game options screen. Same fix. 4. **`SaveTo$`** (line 3847, inside the HTTP downloader) -- the most-complex site. The write happens incrementally in a read- bytes loop with a content-length-completion check. The temp is only promoted on `BytesIn = BytesToRead`; partial downloads (EOF before completion, TCP drop, user cancellation) abort the temp and leave the prior on-disk file (if any) intact. This is meaningful because the downloader fetches game-update files and a corrupted partial write previously could leave a half-downloaded .dat in place; the next launch would read it and either crash or run with corrupt data. ## Lifecycle differences from server SafeWrite migration * MainMenu.bb is included only by Client.bb, which already includes Logging.bb (verified). No new include needed. * `LogIn()` and `GameOptionsMenu()` are different functions, so the `Local MMFinal$ / MMTemp$` declarations don't conflict despite both Misc.dat sites using the same local names. * The downloader path needs `SafeWriteAbort` on the partial path (BytesIn != BytesToRead) -- distinct from the success-path `SafeWriteCommit`. Pattern matches the RC Terrain Editor's `pad_or_optimize` site already migrated in PR #279. ## Verification * `compile.bat -t` clean (Client compiles). * `test.bat` -- 26/26 pass on retry. First run hit the documented OnlinePlayerChainTest CI/local intermittent flake (~30% rate per the feedback_blitzforge_test_handle_object_gc memory). ## Deferred The client-side SafeWrite migration is now complete for MainMenu.bb. Other remaining client/UI writes (if any) would be discovered by the next per-Tools or per-MainMenu audit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c31d9ceff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+3906
to
+3907
| If BytesIn = BytesToRead | ||
| SafeWriteCommit%(DLTemp$, SaveTo$, Save) |
There was a problem hiding this comment.
DownloadFile ignores the return value of SafeWriteCommit%, so a full network read is reported as success even when the local promote step fails (e.g., destination not writable or copy failure). In that case this function still returns BytesIn > 0, and its callers treat non-zero as a successful download, which can make them continue with stale/missing on-disk data. Capture the commit result and convert failure to a 0 return (and/or abort path) so call sites can handle it as a failed download.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the client-side counterpart to PR #279's server/editor SafeWrite sweep. 4 sites in
MainMenu.bbthat wrote directly to production files now use the established atomic SafeWriteOpen/SafeWriteCommit pair, with.bakretention on a 1-cycle recovery window.Non-technical
A crash, disk-full, or process kill during one of these client-side save operations previously left the file empty or partially-written. The user would see locked-out login pre-fill, broken project load, or a corrupted partial download. After this PR, any of those failure modes leaves the prior file intact.
Sites migrated (4)
Data\Last Username.datLogIn()Data\Game Data\Misc.datLogIn()Data\Game Data\Misc.datGameOptionsMenu()SaveTo$(HTTP downloader)DownloadFile()BytesIn = BytesToRead; partial downloads abort the tempThe downloader path (site 4) is the most-complex: it writes incrementally in a read-bytes loop and only commits if the full content-length arrived. Partial downloads (EOF before completion, TCP drop, user cancellation) abort the temp and leave the prior file (if any) intact. Same shape as RC Terrain Editor's
pad_or_optimizesite already migrated in #279.Acceptance criteria
MMFinal$/MMTemp$locals don't conflict (different functions)compile.bat -tcleanTest plan
compile.bat -texit 0test.bat— 26/26 pass (retry needed for the documented OnlinePlayerChainTest flake; passes on second run)Trade-offs / deferred
🤖 Generated with Claude Code