Close race window in StoreService.patch() between snapshot and write-back#5385
Conversation
![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: a3298f3af0
ℹ️ 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".
Preview deploymentsHost Test Results 1 files 1 suites 2h 57m 27s ⏱️ Results for commit 1b97c4d. Realm Server Test Results 1 files ± 0 1 suites ±0 11m 29s ⏱️ + 1m 54s Results for commit 1b97c4d. ± Comparison against earlier commit 96f3e98. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrency hazard in StoreService.patch() where a long-running linked-relationship resolution step could occur after taking the serialized snapshot used for merge + write-back, allowing concurrent writes to the same instance to be reverted by the stale snapshot.
Changes:
- Reorders
StoreService.patch()to resolve linked-card relationships before snapshotting/serializing the target instance, shrinking the race window that could clobber concurrent field writes. - Adds an integration test that forces
patch()to pause during relationship resolution, performs a concurrent write to another field, and asserts that the write survives the patch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/host/app/services/store.ts | Reorders relationship resolution to happen before the snapshot/serialize step in StoreService.patch(). |
| packages/host/tests/integration/store-test.gts | Adds a regression test covering a concurrent field write during store.patch() while relationship resolution is artificially delayed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
can you provide more context for the reason of this PR? why do we need to load links when we are doing a document patch? linked fields are not part of the patch payload, these are side loaded data. the document being patched should be able to be patched without loading links at all. (a patch should not touch the |
…rite-back patch() snapshotted the instance's full document before resolving linked relationships (a real network gap), then reapplied that snapshot wholesale afterward. Any field written on the same live instance by other code during that gap was silently reverted when the stale snapshot landed. Resolving linked relationships first, then snapshotting immediately before the write-back, closes the window. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…atch() The write ran synchronously right after invoking patch(), before the function had actually reached the relationship-load step it awaits on — patch() yields at its own initial instance lookup first. That let the write land before either the old or new snapshot point, so the test could pass without the fix under test. Wait for a signal fired from inside the stub once relationship loading is genuinely paused, then write. Also restore the literal original loadPatchedInstances reference in the finally block instead of a rebound copy of it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sure @habdelra There is a catalog listing create command that creates a listing card, then kicks off about nine background tasks concurrently against that same card to auto-populate it — name, summary, tags, categories, license, examples and specs get set directly, while a screenshot and thumbnail get attached via PatchCardInstanceCommand → store.patch(). I was seeing fields silently disappear after listing creation — a tag or category set by one task would vanish once another task's patch landed later. I traced it to store.patch(): it snapshots the instance, resolves the patch's relationship target (a network fetch), then reapplies the snapshot wholesale — so whatever another task wrote to the same instance during that fetch gets reverted when the stale snapshot writes back. That's what this PR fixes: resolving the relationship before taking the snapshot instead of after, so the snapshot can't go stale. On why patch() resolves the relationship at all — you're right it's not needed to persist the patch, the server only needs the pointer. It's there because LinksTo.deserialize()'s lazy fallback (cache → included[] → not-loaded sentinel) assumes a live UI template that re-renders once the real value arrives, but most patch() callers are Commands — plain code with no equivalent "wait for it." Without the eager load, a patched relationship would come back as an unresolved placeholder for any caller that doesn't already have the target cached, with no automatic recovery. I intentionally kept this PR scoped to the concurrency bug rather than also removing that eager load — patch()'s relationships path has other callers (the AI chat edit tools, code-mode field editing, submission-retry logic) I haven't audited for whether they assume synchronous hydration |
richardhjtan
force-pushed
the
cs-11609-listing-create-race-condition-bug
branch
from
96f3e98 to
1b97c4d
Compare
Summary
There's a catalog listing-create command that creates a listing card, then kicks off about nine background tasks concurrently against that same card to auto-populate it — name, summary, tags, categories, license, examples, and specs get set directly, while a screenshot and thumbnail get attached via
PatchCardInstanceCommand→store.patch(). Fields were silently disappearing after listing creation — a tag or category set by one task would vanish once another task's patch landed later.The cause is in
StoreService.patch(): it snapshots the instance, resolves the patch's relationship target (a network fetch), then reapplies the snapshot wholesale — so whatever another task wrote to the same instance during that fetch gets reverted when the stale snapshot writes back.This reorders the method to resolve the relationship before taking the snapshot instead of after, so the snapshot can't go stale. No behavior change for the non-concurrent case.
Why
patch()resolves relationships at allThis isn't required to persist the patch — the server only needs the pointer, not the target's content. It's there because
LinksTo.deserialize()'s lazy fallback (cache →included[]→not-loadedsentinel) assumes a live UI template that re-renders once the real value arrives, but mostpatch()callers are Commands — plain code with no equivalent "wait for it." Without the eager load, a patched relationship would come back as an unresolved placeholder for any caller whose target isn't already cached, with no automatic recovery.This PR is intentionally scoped to the concurrency bug rather than also removing that eager load —
patch()'s relationships path has other callers (the AI chat edit tools, code-mode field editing, submission-retry logic) that haven't been audited for whether they assume synchronous hydration.Test plan
Deferred, mutates an unrelated field on the same live instance whilepatch()is in flight, and asserts that field survives.store.tsfails the test (actual: "Hassan",expected: "Race Winner"); with the reorder in place, it passes.