Repository navigation
fix(aggregator): serialize JSON store persists + unique temp paths, 5xx on persistence failure (#1509) - #1640
Merged
Merged
Conversation
…x on persistence failure (#1509) Comments/Actions stores and the live session registry persisted via a pid-only temp path with no serialization: concurrent writes shared the temp file, the first rename won, the rest threw ENOENT, and fulfilled calls diverged from disk. Persists now run through a per-store promise chain with a unique temp path per write and the snapshot taken when the queued write runs. Fixing that exposed a second race the regression test caught: load() set a boolean and let concurrent first callers proceed before the read finished; the read completion then reset the in-memory array and wiped records just pushed (5 concurrent first adds -> 1 in memory). load() is now a memoized promise in both stores. POST /api/comments and /api/actions return 500 on persistence failure; 400 remains for validation/body errors only. Regression: test/store-concurrency.test.ts - 20 concurrent adds per store, fulfilled == reloaded == 20. Full aggregator suite on laptop: 303 pass / 0 fail; tsc --noEmit clean.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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.
Fixes #1509 (queue task 1769, from triage task 1548).
Problem: the aggregator JSON stores persisted through a temp file named only by pid with no write serialization — concurrent writes shared one temp path, the first rename won, the rest threw ENOENT, and records on disk diverged from calls that succeeded (triage repro: 20 concurrent adds → 2 fulfilled, 18 ENOENT, 19 on disk).
Fix:
comments.ts/actions.ts: persists serialized through a per-store promise chain; unique temp path per write (tmp-<pid>-<seq>); snapshot taken when the queued write runs so the last write carries full current state; a failed write rejects its own caller without poisoning the chain.sources/live.tsregistry write: same serialization + unique temp (its best-effort catch stays, but serialized writes no longer lose updates into it).load()set a boolean flag and let concurrent first callers proceed before the read finished; the read's completion then reset the in-memory array, wiping records just pushed (5 concurrent first adds → 1 in memory, even with serialized persists).load()is now a memoized promise in both stores. Without this, the acceptance criterion (fulfilled == reloaded) cannot hold for a cold store.server.ts: POST /api/comments and /api/actions return 500 on persistence failure; 400 remains for validation/body-parse errors only.Tests: new
test/store-concurrency.test.ts— 20 concurrent adds per store, fulfilled == reloaded == 20, no ENOENT. Full aggregator suite on the laptop: 303 pass / 0 fail;tsc --noEmitclean. CI verification pending (fleet runner stall #1599).Need help on this PR? Tag
@codesmith-botwith what you need.