Claim the sequential queue read gate for deferred writes - #99815
Claim the sequential queue read gate for deferred writes#99815Abdukhamid000 wants to merge 2 commits into
Conversation
`API.write()` claims the read gate synchronously: `push()` calls `setIsReadyPromisePending()` before its first await, so a READ firing on the next line parks behind the write. `writeWhenReady()` never did, so deferring a write silently dropped that ordering - the queue stayed empty for the length of the deferral, and a destination screen fetching the same data raced ahead and repopulated itself from pre-write server state. `claimReadGateForDeferredWrite()` claims the gate on behalf of a write that is not on the queue yet. `push()` adopts the claim when the write lands, so the handover leaves no gap, and the queue drain resolves it. `flush()` also had to learn about it: its empty-queue branch resolved the gate unconditionally, which during a deferral means "the write hasn't been pushed yet", not "nothing is coming". The follower branch still resolves, since a tab that never processes the queue would otherwise park READs forever. No-op while offline, matching `push()` and `flush()`, neither of which parks READs behind a queue that isn't running.
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![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: 7214f06594
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| setIsReadyPromisePending(); | ||
| deferredWriteGateClaims += 1; |
There was a problem hiding this comment.
When writeWhenReady() is called while another write is already being processed, this reuses that write's pending isReadyPromise. If the active queue finishes before the deferred barrier settles, the queue-drain path at flush() lines 602–621 resolves the shared promise without checking deferredWriteGateClaims, so subsequent reads proceed while the deferred write is still absent from the queue and can restore pre-write server data. The claim count must be honored by every online gate-resolution path, not only the newly guarded empty-queue branch.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!settleOnce()) { | ||
| return; | ||
| } | ||
| // A later write may have opened a new gate since; resolving that one would let READs through |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should read as plain, natural sentences. This comment joins two independent clauses with a semicolon, which the style guide asks you to write as two separate sentences instead.
Split the clauses into two sentences:
// A later write may have opened a new gate since. Resolving that one would let READs through
// while somebody else's write is still pending.
if (isReadyPromise !== claimedPromise) {
return;
}Reviewed at: 7214f06 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| waitForIdle: jest.fn(() => Promise.resolve()), | ||
| // Called by the network layer on init; stub so advancing fake timers doesn't hit a missing export. | ||
| flush: jest.fn(), | ||
| // The claim writeWhenReady settles once its write reaches (or fails to reach) the queue; the real |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should read as plain, natural sentences. This comment uses a semicolon to join two independent sentences; the style guide asks you to write them as two separate sentences.
Split into two sentences:
// The claim writeWhenReady settles once its write reaches (or fails to reach) the queue. The real
// gate is covered in SequentialQueueReadGateTest.
claimReadGateForDeferredWrite: jest.fn(() => ({handOff: jest.fn(), release: jest.fn()})),Reviewed at: 7214f06 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Nothing imports the type - consumers use the inferred return type of claimReadGateForDeferredWrite - and exporting it added a knip finding.
1 participant
cc @JakubKorytko -
writeWhenReadyis yours, and the design question at the bottom is really for you.Explanation of Change
API.write()claims the sequential queue's read gate synchronously:push()callssetIsReadyPromisePending()before its first await, with a comment saying exactly why - "so any READ that fires on the next synchronous line viawaitForIdle()correctly parks behind this write".writeWhenReady()never did. So deferring a write silently dropped the read-after-write ordering that everywrite()caller gets for free: the queue stays empty for the length of the deferral,waitForWrites()resolves immediately, and a destination screen that fetches the same data races ahead of the write and repopulates itself from pre-write server state.That is what caused #99805. Nothing in
writeWhenReady's docblock warned about it - it coversconflictResolver, barrier-rejection semantics, cross-call ordering and crash-loss durability, but not this - so the next caller to defer a write would have walked into the same trap.The fix.
claimReadGateForDeferredWrite()claims the gate for a write that is not on the queue yet.push()marks the gate pending again idempotently once the write lands, so it adopts the claim rather than opening a second one, and the queue drain resolves it - the handover leaves no gap for a READ to slip through. The claim is two-phase:handOff()when the write reaches the queue,release()when it never does.flush()had to learn about it too. Its empty-queue branch resolved the gate unconditionally, and during a deferral the queue is empty - so any unrelated flush in that window would have released the claim and the fix would have evaporated. It now skips that resolve while a deferred claim is outstanding. The follower branch still resolves: a tab that never processes the queue would otherwise park READs forever. There is a test that fails without the guard.Offline is a no-op, matching
push()(which returns before claiming the gate in that state) andflush()(which resolves it) - neither parks READs behind a queue that isn't running. If the app goes offline mid-deferral, the claim is handed back rather than parking every READ until reconnect.One design call worth a second opinion, @JakubKorytko: this is safe by default with no opt-out, so every
writeWhenReadycaller now holds the gate, and READs wait out the barrier on top of the write's own round trip. There are no production callers today, so nothing regresses - but if you'd rather have an escape hatch for a long deferral whose destination genuinely doesn't refetch, say so and I'll add the option.I also documented a footgun this introduces: a barrier must not await a READ of its own, or it deadlocks until
safetyTimeoutMsbreaks it.Fixed Issues
$ #99805
PROPOSAL: N/A - root-cause follow-up to the revert in #99814
Tests
This is a primitive with no production callers, so the behavior is covered by unit tests rather than a UI flow:
npx jest tests/unit/SequentialQueueReadGateTest.ts- 6 tests over the gate itself: a READ parks behind a claim; a hand off leaves the gate to the queue drain; aflush()that finds the queue empty mid-deferral does not release it; a second claim adopts the first rather than opening a second gate; offline is a no-op; a stale release cannot resolve a gate a later write openednpx jest tests/unit/APIWriteWhenReadyTest.ts- the wiring: the gate is claimed synchronously before the barrier settles, handed off when the write executes online, and given back when it executes offlinenpx jest tests/unit/SequentialQueueTest.ts tests/unit/APITest.ts tests/unit/NetworkTest.tsx tests/unit/MiddlewareTest.ts tests/unit/resolveWriteBarrierTest.ts tests/unit/pendingSearchWriteTest.ts tests/unit/pendingSubmitWriteTest.tsAll 179 tests across those 9 suites pass locally, along with ESLint, typecheck and cspell.
Offline steps
Covered by the "is a no-op while offline" test in
SequentialQueueReadGateTestand the "hands the gate back when the write executes while offline" test inAPIWriteWhenReadyTest. There is no user-facing flow to exercise, since nothing in production callswriteWhenReadyon this branch.QA steps
No user-facing change:
writeWhenReadyhas no production callers, so this alters no flow QA can reach. A regression here would surface as READs hanging, so a general smoke test of any list that loads from the server (Inbox, Reports, Spend > Expenses) is enough to confirm nothing is parked.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A - no user-facing change; this PR touches only the API/queue primitive and its unit tests.
Android: mWeb Chrome
N/A - no user-facing change; this PR touches only the API/queue primitive and its unit tests.
iOS: Native
N/A - no user-facing change; this PR touches only the API/queue primitive and its unit tests.
iOS: mWeb Safari
N/A - no user-facing change; this PR touches only the API/queue primitive and its unit tests.
MacOS: Chrome / Safari
N/A - no user-facing change; this PR touches only the API/queue primitive and its unit tests.
MacOS: Desktop
N/A - no user-facing change; this PR touches only the API/queue primitive and its unit tests.