Repository navigation
fix(persist): fence async commits against late-landing stale writes (#373) - #409
Merged
Merged
Conversation
…373) writeInner snapshots the payload, then commits via async rename. A flushSync writing a NEWER payload could return, and the stale in-flight rename could land afterward and clobber it — the flushSync success contract (disk holds the newest payload on return) was violated, and callers that drop in-memory state on success could lose data. Root fix: per-id commit fence. Every committed write (sync or async) bumps a seq. writeInner captures the seq BEFORE build() and re-checks it inside a SYNCHRONOUS commit block (fence check + renameSync), so the event loop cannot interleave a flushSync between check and rename. The spill fallback gets the same fenced synchronous write (its slot is shared per id, same clobber shape). Superseded writes settle quietly — no failure recorded, no throw into the chain; a newer payload owns disk. Regression test uses a flushSync re-entering from inside the async builder's build(), making the interleave deterministic: fails on the old code (disk holds the stale snapshot), passes on the fence. Spill and retry-path tests migrate their failure-injection seam from the async fsp.rename surface to the new synchronous commit surface; their intent is unchanged.
Contributor
📦 Built Package ArtifactBranch: Option A — Install from npm PR tag (recommended)In your adapter project: npm install acp-kernel@pr-409Each push to this PR publishes a new version under the Option B — Download artifact
tar xzf acp-kernel-pr409.tgz
npm install ./packageThis comment is automatically updated on each push. |
This was referenced Sep 25, 2026
This was referenced Sep 25, 2026
Merged
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 #373. Also closes the phantom-delivery gap recorded in #408 (a prior comment claimed a PR #374 / commit dddf0a4 that never existed).
The bug (as diagnosed in the issue, confirmed here)
writeInnersnapshots the payload at chain time, then commits via asyncfsp.rename.flushSyncwrites synchronously and returns — but never joinswriteChains. Sequence: in-flight write snapshots v1 → state advances to v2 →flushSyncwrites v2, returns true → the in-flight rename lands v1 afterward. The documented contract ("callers that use the result to drop in-memory state" may drop on success) is violated: disk regresses to v1 while memory is gone.The same shape exists on the spill path: the
.fb.jsonslot is shared per id, so a stale in-flight spill write could overwrite a newer spill.The fix: per-id commit fence + synchronous commit block
commitSeq: Map<id, number>— bumped by every committed write (flushSync canonical, flushSync spill, writeInner canonical, writeInner spill).writeInnercaptures the seq beforebuild()(a flushSync re-entering from inside build() must fence this snapshot too), keeps async staging (mkdir/writeFile to a unique tmp — safe to interleave), then commits in one synchronous block: fence check →renameSync→ seq bump. The event loop cannot run a flushSync between the check and the rename, so a stale payload can never land after a newer commit.writeFileSyncinside the same block (same shared-slot clobber shape). Rejection semantics on spill unchanged (still throws after spilling — existing tests assert this).Chained async-vs-async ordering is preserved: a queued write built after a commit sees the bumped seq as its base and commits normally.
Verification
flushSyncre-entrant from inside the async builder'sbuild()— a deterministic interleave. On the pre-fix code it fails exactly as the issue describes (disk holds the stale snapshot); on the fence it passes. Confirmed red→green via stash.fsp.rename(async surface) tofs.renameSync(the new synchronous commit surface) — same injected failures, same assertions, intent unchanged.