fix(crew-sync): unblock the outbox on server timeouts; keep retry ord… - #555
Merged
Conversation
…er (H5, H6)
H5 — a head-of-line block that could never dead-letter
A Postgres statement timeout arrives as UploadDataError('… canceling
statement due to statement timeout', '57014'). 57014 is not 22/23/42 and
not PGRST, so it was not terminal; classification then fell through to
the transport-message test, matched \btimeout\b, and returned 'network'.
The network branch consumes no retry, never sets `failed`, and STOPS the
drain — so one server-side timeout pinned the head of the outbox forever
and blocked every later write on the device, while FailedSyncBanner
(which filters on `failed`) showed nothing at all. A crew member could
work a full shift, sync nothing, and only find out at logout.
A DATA error carrying a Postgres/PostgREST code demonstrably REACHED the
server — the server produced the code — so it is now classified terminal
or transient by that code and never falls through to the transport test.
An uncoded timeout message still classifies as network, because nothing
there proves it reached anyone.
Transport failures still never dead-letter: discarding a crew member's
work because their signal is bad would be worse than the bug. But an
invisible retry loop is its own failure, so past STALLED_NETWORK_ATTEMPTS
consecutive transport failures the queue surfaces in an amber "still
trying to sync" notice — no discard affordance, because nothing is lost;
it just stops being invisible.
H6 — "Retry all" resurrecting a superseded write
Dead-lettering let the drain continue (correct — other records must not
block), so later writes for the SAME record pushed on top of a gap.
Clearing `failed` in place then replayed the stale payload as though it
were newest: crew ticks an item, it dead-letters, they realise it isn't
done and un-tick it (which pushes fine), they tap Retry all — and the
server flips back to complete, with the next delta pull erasing the
un-tick from the phone too. Same shape for inventory quantities and
availability.
Dead-lettering now holds back that record's remaining queued mutations,
scoped to (table, targetId) so every other record keeps draining. Ids are
preserved, so a retry replays tick-then-un-tick in the order the crew
member actually performed them. retryAllFailedMutations now reads
orderBy('id') rather than a bare toArray(), so the re-queue order is
deterministic rather than incidental.
Worth recording: the first version of the hold-back was itself wrong. The
drain iterates a snapshot taken before the loop, so it pushed the very
mutations that had just been held back — the tests caught it, not review.
Fixed with a drain-scoped heldBack set; the durable state remains the
row's `failed` flag.
Verification: tsc clean, 2814/2814 (+8), lint 201/202, ui-classes clean,
build succeeds, chokepoints 0, ratchet steady. Both fixes were reverted
independently to confirm the tests fail without them — H5's classification
change breaks 2 tests when reverted, H6's cascade breaks 1.
Not covered here, and unchanged: this is unit-level. The offline paths
still warrant a real device pass before relying on them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013Fu4FzR25EYR7kz64cyp6r
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Contributor
📝 WalkthroughWalkthroughThe PR classifies coded upload errors, exposes stalled network retries, orders failed-mutation retries, and prevents successor mutations from overtaking permanently rejected predecessors. Tests cover classification, retry thresholds, ordering, and unrelated-record draining. ChangesSync reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SyncEngine
participant DexieOutbox
participant Server
SyncEngine->>DexieOutbox: Read ordered mutation snapshot
SyncEngine->>Server: Upload mutation
Server-->>SyncEngine: Return permanent rejection
SyncEngine->>DexieOutbox: Mark later same-record mutations failed
SyncEngine->>SyncEngine: Skip held-back mutation IDs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
unit/dexie/sync-outbox-ordering.test.ts (1)
208-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the stalled-threshold test to exercise the actual drain path.
This test only checks that
STALLED_NETWORK_ATTEMPTSis a small positive number. It does not confirm that repeated network failures in the drain actually raisenetworkRetryCountto that threshold while keeping the row!failed, which is the exact conditionfailed-sync-banner.tsxdepends on to surface the stalled notice. Consider adding a test similar to the existing dead-letter test (lines 120-135) that pushes aTypeError/network failureSTALLED_NETWORK_ATTEMPTStimes and assertsnetworkRetryCount >= STALLED_NETWORK_ATTEMPTSwithfailedstillfalseorundefined.🤖 Prompt for 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. In `@unit/dexie/sync-outbox-ordering.test.ts` around lines 208 - 213, Replace the range-only test in the “stalled-outbox visibility threshold” suite with a drain-path test modeled on the existing dead-letter test: enqueue a row, trigger a TypeError/network failure exactly STALLED_NETWORK_ATTEMPTS times, then assert networkRetryCount >= STALLED_NETWORK_ATTEMPTS while failed remains false or undefined. Preserve the existing setup and use the actual drain behavior rather than testing the constant’s numeric range.
🤖 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.
Nitpick comments:
In `@unit/dexie/sync-outbox-ordering.test.ts`:
- Around line 208-213: Replace the range-only test in the “stalled-outbox
visibility threshold” suite with a drain-path test modeled on the existing
dead-letter test: enqueue a row, trigger a TypeError/network failure exactly
STALLED_NETWORK_ATTEMPTS times, then assert networkRetryCount >=
STALLED_NETWORK_ATTEMPTS while failed remains false or undefined. Preserve the
existing setup and use the actual drain behavior rather than testing the
constant’s numeric range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 35d779ac-de5e-492e-ac4a-3c7467218156
📒 Files selected for processing (5)
app/crew/_components/failed-sync-banner.tsxlib/dexie/helpers.tslib/dexie/net.tslib/dexie/syncService.tsunit/dexie/sync-outbox-ordering.test.ts
This was referenced Aug 4, 2026
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
2 participants
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.
…er (H5, H6)
H5 — a head-of-line block that could never dead-letter A Postgres statement timeout arrives as UploadDataError('… canceling statement due to statement timeout', '57014'). 57014 is not 22/23/42 and not PGRST, so it was not terminal; classification then fell through to the transport-message test, matched \btimeout\b, and returned 'network'. The network branch consumes no retry, never sets
failed, and STOPS the drain — so one server-side timeout pinned the head of the outbox forever and blocked every later write on the device, while FailedSyncBanner (which filters onfailed) showed nothing at all. A crew member could work a full shift, sync nothing, and only find out at logout.A DATA error carrying a Postgres/PostgREST code demonstrably REACHED the server — the server produced the code — so it is now classified terminal or transient by that code and never falls through to the transport test. An uncoded timeout message still classifies as network, because nothing there proves it reached anyone.
Transport failures still never dead-letter: discarding a crew member's work because their signal is bad would be worse than the bug. But an invisible retry loop is its own failure, so past STALLED_NETWORK_ATTEMPTS consecutive transport failures the queue surfaces in an amber "still trying to sync" notice — no discard affordance, because nothing is lost; it just stops being invisible.
H6 — "Retry all" resurrecting a superseded write
Dead-lettering let the drain continue (correct — other records must not block), so later writes for the SAME record pushed on top of a gap. Clearing
failedin place then replayed the stale payload as though it were newest: crew ticks an item, it dead-letters, they realise it isn't done and un-tick it (which pushes fine), they tap Retry all — and the server flips back to complete, with the next delta pull erasing the un-tick from the phone too. Same shape for inventory quantities and availability.Dead-lettering now holds back that record's remaining queued mutations, scoped to (table, targetId) so every other record keeps draining. Ids are preserved, so a retry replays tick-then-un-tick in the order the crew member actually performed them. retryAllFailedMutations now reads orderBy('id') rather than a bare toArray(), so the re-queue order is deterministic rather than incidental.
Worth recording: the first version of the hold-back was itself wrong. The drain iterates a snapshot taken before the loop, so it pushed the very mutations that had just been held back — the tests caught it, not review. Fixed with a drain-scoped heldBack set; the durable state remains the row's
failedflag.Verification: tsc clean, 2814/2814 (+8), lint 201/202, ui-classes clean, build succeeds, chokepoints 0, ratchet steady. Both fixes were reverted independently to confirm the tests fail without them — H5's classification change breaks 2 tests when reverted, H6's cascade breaks 1.
Not covered here, and unchanged: this is unit-level. The offline paths still warrant a real device pass before relying on them.
Claude-Session: https://claude.ai/code/session_013Fu4FzR25EYR7kz64cyp6r
Summary by CodeRabbit