fix(webkit/window): publish state and layout saves atomically - #44
Conversation
lthn and core/ide both persist window state into the shared Core config directory. StateManager.save and LayoutManager.save wrote with plain truncate-and-write, so two processes racing the same file could splice one writer's short document onto the tail of the other's longer one — window_state.json arrived in production exactly that shape (a complete 538-byte document followed by 145 bytes of the previous 683-byte save), after which every load fails with "invalid character after top-level value" and all saved positions are lost. coreWriteFileAtomic writes a uniquely-suffixed sibling temp file and renames it into place: rename is atomic on POSIX, so readers and racing writers only ever observe complete documents. The suffix must be unique per writer — a fixed ".tmp" would move the same splice into the temp file. Mirrors core/go's Fs.WriteAtomic shape with the mode preserved. The Ugly test runs the two-manager storm against one path and asserts every observed read parses; it catches the old splice probabilistically and the new path deterministically. Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesAtomic persistence
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go/display/webkit/pkg/window/core_helpers.go`:
- Around line 40-41: Update the write-failure branch in the surrounding
temporary-file helper to remove tmp before returning the error, ensuring cleanup
occurs even when core.WriteFile fails; preserve the existing error wrapping and
normal cleanup path.
- Line 39: Replace the probabilistic temporary path generation in the
surrounding helper with an exclusive temporary-file creation API, or create the
generated path using O_CREATE_EXCL before writing. Ensure concurrent writers
cannot reuse an existing .tmp. sibling file, while preserving the subsequent
write and rename behavior.
In `@go/display/webkit/pkg/window/state_test.go`:
- Around line 760-783: Update the concurrent save/read test around the writer
closure and reader loop so both writers signal readiness before reads begin,
ensuring saves overlap the read phase. Propagate each sm.save() error through
the test’s synchronization channel and assert it in the test instead of
discarding it, while preserving the existing JSON corruption check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cfba034-6dc0-45a8-a00c-e983ec65510c
📒 Files selected for processing (4)
go/display/webkit/pkg/window/core_helpers.gogo/display/webkit/pkg/window/layout.gogo/display/webkit/pkg/window/state.gogo/display/webkit/pkg/window/state_test.go
…oves overlap CreateTemp (O_EXCL) replaces the probabilistic random suffix, so two writers can never share a temp file even in principle; the caller's mode contract is restored via Chmod before publish, and every failure branch now removes its temp file. The concurrent test seeds one save before the storm, reads until BOTH writers finish (overlap guaranteed), and propagates every save error instead of discarding them. Co-Authored-By: Virgil <virgil@lethean.io>
) The four shells #46 could not touch while PR #44 was in flight, now adjudicated per case rather than swept. register_test.go — genuine fake. All three tests asserted that the string "Register:good" contains "good" and never named Register at all. Replaced with a real Good/Bad/Ugly triplet that calls it: the returned constructor yields an OK Result carrying a *Service wired to the given platform with manager and spec registry initialised (Good); a nil platform still constructs rather than panicking during Core wiring, long before any window opens (Bad); and the constructor is reusable, each call producing an INDEPENDENT service — a shared specs map would let two Cores overwrite each other's window registrations (Ugly). window_test.go — mislabelled but real. These tests DO call WithName / WithTitle / WithURL and assert properly; the ax7Variant lines were dead weight prepended to working bodies. Removed those two lines per test, 116 in total, leaving every assertion byte-identical. Receipts: coverage 68.3% before, 68.3% after — nothing real was removed Register 100.0% (was already covered by service_screen_test.go's real use; the deleted shells contributed nothing) go test -race -count=2 clean vet + gofmt clean
1 participant
Two processes sharing the Core config directory (lthn + core/ide) can splice
window_state.jsonunder truncate-and-write — production carried exactly that corruption (complete short document + stale tail, every load failing withinvalid character after top-level value). State and layout saves now publish via a uniquely-suffixed temp file + atomic rename.Receipts: library suite 58 packages ok; new
SaveAtomicGood/Ugly tests pass under-race(the pre-existingTestTaskEvalJS_Gooddata race on main is untouched and unrelated); vet + gofmt clean.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Bug Fixes
Tests