diff --git a/app/crew/_components/failed-sync-banner.tsx b/app/crew/_components/failed-sync-banner.tsx index 3c10900b..ca4b0984 100644 --- a/app/crew/_components/failed-sync-banner.tsx +++ b/app/crew/_components/failed-sync-banner.tsx @@ -24,6 +24,7 @@ import { createClient } from '@/lib/supabase/client' import type { MutationTable } from '@/lib/dexie/schema' import { retryAllFailedMutations, discardFailedMutation } from '@/lib/dexie/helpers' import { retryFailedPhotoUploads, discardPendingPhoto } from '@/lib/dexie/photo-sync' +import { STALLED_NETWORK_ATTEMPTS } from '@/lib/dexie/net' import { Button } from '@/components/ui/Button' import { Badge } from '@/components/ui/Badge' import { Dialog } from '@/components/ui/Dialog' @@ -67,6 +68,21 @@ export function FailedSyncBanner({ userId }: Readonly<{ userId: string }>) { [], ) ?? [] + // Transport failures deliberately never dead-letter — losing a crew + // member's work because their signal is bad would be worse than the bug + // this surfaces. But the drain STOPS at a blocked head, so every later + // change on the device queues behind it. Previously that state was + // completely invisible: `failed` is never set on the network path, this + // banner filters on `failed`, and the only trace anywhere was the pending + // count in the logout dialog. A crew member could work a whole shift, sync + // nothing, and find out at logout. + const stalledMutations = useLiveQuery( + () => db.mutations + .filter((m) => !m.failed && (m.networkRetryCount ?? 0) >= STALLED_NETWORK_ATTEMPTS) + .toArray(), + [], + ) ?? [] + const entries: FailedEntry[] = [ ...failedMutations.map((m) => ({ key: `mutation-${m.id}`, @@ -82,7 +98,34 @@ export function FailedSyncBanner({ userId }: Readonly<{ userId: string }>) { })), ] - if (entries.length === 0) return null + if (entries.length === 0 && stalledMutations.length === 0) return null + + // A stalled queue is NOT a failure — the work is intact and still retrying, + // so it gets its own amber notice with no discard affordance rather than + // being folded into the red "didn't sync" list. + const stalledNotice = stalledMutations.length > 0 && ( +
+
+ +
+

+ {stalledMutations.length} change{stalledMutations.length !== 1 ? 's' : ''} still trying to sync +

+

+ Your work is saved on this phone and will keep retrying on its own. + If this stays here, move somewhere with better signal before you + finish for the day. +

+
+
+
+ ) + + if (entries.length === 0) return <>{stalledNotice} const retryAll = async () => { setRetrying(true) @@ -96,6 +139,7 @@ export function FailedSyncBanner({ userId }: Readonly<{ userId: string }>) { return ( <> + {stalledNotice}
{ const db = getDexieDb(userId) - const failed = (await db.mutations.toArray()).filter((m) => !!m.failed) + // orderBy('id') — NOT a bare toArray(). The drain replays in id order, and + // SyncEngine.holdBackSuccessors() deliberately dead-letters a record's whole + // remaining sequence so a retry re-applies it in the order the crew member + // performed it. Clearing the flags in an unspecified order would leave that + // sequence intact but re-queue it non-deterministically. + const failed = (await db.mutations.orderBy('id').toArray()).filter((m) => !!m.failed) for (const mutation of failed) { await db.mutations.update(mutation.id!, { diff --git a/lib/dexie/net.ts b/lib/dexie/net.ts index ac8e32ae..6a03b29a 100644 --- a/lib/dexie/net.ts +++ b/lib/dexie/net.ts @@ -110,12 +110,46 @@ function isTerminalDataCode(code: string): boolean { export function classifyUploadFailure(err: unknown): UploadFailureKind { if (!isOnline()) return 'network' if (err instanceof UploadHttpError) return classifyHttpStatus(err.status) - if (err instanceof UploadDataError && err.code && isTerminalDataCode(err.code)) return 'terminal' + + // A DATA error carrying a Postgres/PostgREST code demonstrably REACHED the + // server — the server is what produced the code. It is therefore terminal or + // transient, and must never fall through to the transport-message test + // below. + // + // That fall-through was a real trap. 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; the message + // then matched \btimeout\b and it was classified 'network'. The network + // branch in SyncEngine.handleFailure never consumes a 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. + // + // Classified 'transient': it may well succeed on replay, but it now spends + // the retry budget and eventually dead-letters into a surface a crew member + // can see. + if (err instanceof UploadDataError && err.code) { + return isTerminalDataCode(err.code) ? 'terminal' : 'transient' + } + if (err instanceof TypeError) return 'network' if (NETWORK_MESSAGE_PATTERN.test(messageOf(err))) return 'network' return 'transient' } +/** + * How many consecutive transport failures — all while the device reported + * itself ONLINE — before the outbox is treated as stalled and surfaced. + * + * Transport failures deliberately never dead-letter: discarding a crew + * member's work because their connection is bad would be far worse than the + * bug this bounds. But an unbounded, silent retry loop is its own failure — + * the drain stops at the blocked head, so every later write on the device + * queues behind it invisibly. Past this threshold the mutation stays queued + * and keeps retrying; it simply stops being invisible. + */ +export const STALLED_NETWORK_ATTEMPTS = 5 + // navigator.locks is unavailable in non-secure contexts, in workers on some // engines, and in the jsdom/node test environment. Declared narrowly here // rather than relying on the ambient DOM lib so the fallback path is diff --git a/lib/dexie/syncService.ts b/lib/dexie/syncService.ts index 0cdbfc57..80f50872 100644 --- a/lib/dexie/syncService.ts +++ b/lib/dexie/syncService.ts @@ -41,6 +41,11 @@ export class SyncEngine { // a shared IndexedDB, so two tabs each pass their own guard — withTabLock() // in processOutbox() is what actually serializes the drain across tabs. private isProcessing = false + // Ids held back mid-drain by holdBackSuccessors(). Scoped to a single + // drain (cleared at its start) — the durable state is the row's `failed` + // flag; this only stops the current loop's pre-computed snapshot from + // pushing a row that was marked failed after the snapshot was taken. + private readonly heldBack = new Set() private disposed = false // Single pending wake-up for a drain that stopped on a not-yet-due // mutation. One handle only — scheduleRetry() clears any previous timer @@ -142,10 +147,18 @@ export class SyncEngine { const db = getDexieDb(this.userId) const pending = (await db.mutations.orderBy('id').toArray()).filter((m) => !m.failed) + // `pending` is a SNAPSHOT taken before the loop. holdBackSuccessors() can + // mark rows failed part-way through it, and the loop would otherwise push + // them anyway — sending exactly the write that was just held back to + // preserve ordering, which defeats the whole point. + this.heldBack.clear() + for (const mutation of pending) { // Auto-incrementing key — always populated once read back from the table. const id = mutation.id as number + if (this.heldBack.has(id)) continue + // Backoff gate: a mutation still inside its retry window stops the // drain entirely (never skip-and-continue — later mutations against // the same record must not jump ahead). Resume when it comes due. @@ -187,6 +200,49 @@ export class SyncEngine { } } + /** + * Marks every still-queued mutation for the SAME record as failed, once one + * of them dead-letters. + * + * Without this, dead-lettering silently drops one write out of the middle of + * a record's sequence: the drain moves on, later writes for that record land + * on the server, and a subsequent "Retry all" replays the stale one on top + * of them. Holding the successors back keeps the sequence intact so a retry + * re-applies it in the order the crew member performed it. + * + * They carry a distinct lastError so the banner does not tell a crew member + * that five separate things failed when one did. + */ + private async holdBackSuccessors( + db: FieldStayDexie, + mutation: MutationRow, + failedId: number, + ): Promise { + const successors = (await db.mutations.orderBy('id').toArray()).filter( + (m) => + !m.failed && + (m.id as number) > failedId && + m.table === mutation.table && + m.targetId === mutation.targetId, + ) + + if (successors.length === 0) return + + console.warn( + `[SyncEngine] holding back ${successors.length} later change(s) to ` + + `${mutation.table}/${mutation.targetId} so the retry order is preserved` + ) + + for (const successor of successors) { + const successorId = successor.id as number + await db.mutations.update(successorId, { + failed: true, + lastError: 'Held back so earlier changes to this item retry in order', + }) + this.heldBack.add(successorId) + } + } + /** * Records one push failure. Returns true when the drain must stop (so * later mutations against the same record can't jump ahead of this one), @@ -237,8 +293,27 @@ export class SyncEngine { failed: true, lastError: describeFailure(err), }) - // A mutation that will never succeed must not block every later write - // against other records — it is finished, so the drain continues. + + // Dead-lettering breaks this record's mutation ORDER, and nothing used + // to put it back. The drain continues past the dead letter (correctly — + // other records must not be blocked), so later mutations for the SAME + // record push successfully on top of a gap. Then "Retry all" clears the + // failed flag in place, the row keeps its original low id, and the drain + // replays the stale payload as though it were the newest write. + // + // Concretely: crew ticks a checklist item (#10) → 500s five times → + // dead-letters. They realise it isn't done and un-tick it (#11) → pushes + // fine, server now false. They tap Retry all → #10 replays + // is_completed = true → the server flips BACK to complete, and the next + // delta pull overwrites Dexie too, so the un-tick disappears from the + // phone as well. Same shape for inventory_items.current_quantity (an old + // count overwriting a corrected one) and crew_availability. + // + // So: hold back this record's remaining queued mutations too. They keep + // their ids, so the sequence is preserved and a retry replays + // tick-then-un-tick in the order the crew member actually performed + // them. Scoped to (table, targetId) — every other record keeps draining. + await this.holdBackSuccessors(db, mutation, id) return false } diff --git a/unit/dexie/sync-outbox-ordering.test.ts b/unit/dexie/sync-outbox-ordering.test.ts new file mode 100644 index 00000000..95787e9d --- /dev/null +++ b/unit/dexie/sync-outbox-ordering.test.ts @@ -0,0 +1,213 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { makeFakeDexieDb, makeFakeSupabase, type FakeDexieDb } from './fake-dexie' +import type { MutationRow } from '@/lib/dexie/schema' + +/** + * Two defects from the 2026-08-03 audit, both of which lose crew work silently. + * + * H5 — a head-of-line block that can never dead-letter. A Postgres statement + * timeout arrives as UploadDataError('… statement timeout', '57014'). + * 57014 is not 22/23/42 and not PGRST, so it was not terminal; the + * message then matched \btimeout\b and it was classified 'network'. The + * network branch consumes no retry, never sets `failed`, and STOPS the + * drain — so one server-side timeout pinned the outbox head forever, + * blocked every later write on the device, and showed nothing in + * FailedSyncBanner (which filters on `failed`). + * + * H6 — "Retry all" resurrecting a superseded write. Dead-lettering let the + * drain continue, 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. Tick → dead-letter → un-tick → Retry all → the + * server flips back to ticked. + */ + +const holder = vi.hoisted(() => ({ + db: null as unknown, + supabase: null as unknown, +})) + +vi.mock('@/lib/dexie/schema', () => ({ + getDexieDb: () => holder.db, + isDexieShutdown: () => false, +})) + +vi.mock('@/lib/supabase/client', () => ({ + createClient: () => ({ + from: (table: string) => (holder.supabase as ReturnType).from(table), + }), +})) + +import { SyncEngine } from '@/lib/dexie/syncService' +import { classifyUploadFailure, UploadDataError, STALLED_NETWORK_ATTEMPTS } from '@/lib/dexie/net' + +const NOW = Date.parse('2026-08-03T09:00:00.000Z') + +function db(): FakeDexieDb { return holder.db as FakeDexieDb } +function supabaseCalls() { return (holder.supabase as ReturnType).calls } +function mutationRow(id: number) { + return db().mutations.get(id) as Promise +} + +function setOnline(value: boolean): void { + Object.defineProperty(globalThis, 'navigator', { + configurable: true, + value: { onLine: value }, + }) +} + +// A Postgres statement timeout: the server DID respond, and said "57014". +const STATEMENT_TIMEOUT = { + error: { message: 'canceling statement due to statement timeout', code: '57014' }, +} +const UPLOAD_OK = { data: [{ id: 'x' }], error: null } + +async function seed(overrides: Partial = {}): Promise { + const id = await db().mutations.add({ + table: 'checklist_instance_items', + targetId: 'item_1', + op: 'PATCH', + payload: { is_completed: true }, + createdAt: new Date(NOW).toISOString(), + retryCount: 0, + ...overrides, + }) + return id as number +} + +describe('classifyUploadFailure — a coded error reached the server', () => { + beforeEach(() => setOnline(true)) + + it('classifies a statement timeout as transient, NOT network', () => { + const err = new UploadDataError( + 'checklist_instance_items upload failed: canceling statement due to statement timeout', + '57014', + ) + // 'network' would mean: never consume a retry, never dead-letter, block + // the drain forever. + expect(classifyUploadFailure(err)).toBe('transient') + }) + + it('still classifies a genuine transport failure as network', () => { + expect(classifyUploadFailure(new TypeError('Failed to fetch'))).toBe('network') + expect(classifyUploadFailure(new Error('The network connection was lost'))).toBe('network') + }) + + it('still classifies an RLS/constraint rejection as terminal', () => { + expect(classifyUploadFailure(new UploadDataError('denied', '42501'))).toBe('terminal') + expect(classifyUploadFailure(new UploadDataError('dupe', '23505'))).toBe('terminal') + }) + + it('classifies an uncoded timeout message as network (transport still wins without a code)', () => { + // No code => nothing proves it reached the server. + expect(classifyUploadFailure(new Error('request timed out'))).toBe('network') + }) +}) + +describe('outbox — a server timeout no longer blocks the queue forever', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(NOW) + holder.db = makeFakeDexieDb() + setOnline(true) + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => { + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('spends the retry budget and eventually dead-letters into a visible surface', async () => { + const id = await seed() + holder.supabase = makeFakeSupabase({ + checklist_instance_items: Array.from({ length: 12 }, () => STATEMENT_TIMEOUT), + }) + + const engine = new SyncEngine('u1') + for (let i = 0; i < 12; i++) { + vi.setSystemTime(NOW + i * 600_000) // step past each backoff window + await engine.processOutbox() + } + + const row = await mutationRow(id) + expect(row?.retryCount, 'a server-side timeout must consume the retry budget').toBeGreaterThan(0) + expect(row?.failed, 'and must eventually become visible to the crew member').toBe(true) + }) +}) + +describe('outbox — dead-lettering preserves a record\'s mutation order (H6)', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(NOW) + holder.db = makeFakeDexieDb() + setOnline(true) + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => { + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('holds back later changes to the SAME record when one dead-letters', async () => { + // The audit's exact scenario: tick, then un-tick, with the tick failing. + const tickId = await seed({ payload: { is_completed: true } }) + const untickId = await seed({ payload: { is_completed: false } }) + + holder.supabase = makeFakeSupabase({ + checklist_instance_items: Array.from({ length: 20 }, () => ({ + error: { message: 'denied by policy', code: '42501' }, // terminal + })), + }) + + const engine = new SyncEngine('u1') + await engine.processOutbox() + + expect((await mutationRow(tickId))?.failed, 'the tick dead-letters').toBe(true) + expect( + (await mutationRow(untickId))?.failed, + 'the un-tick must be held back, or Retry all replays the tick ON TOP of it', + ).toBe(true) + + // The un-tick never reached the server, so there is no newer server state + // for a later Retry-all replay of the tick to clobber. (`calls` records + // every chain method, not one entry per push — so assert on the PAYLOAD + // rather than the call count.) + const sentPayloads = supabaseCalls() + .filter((c) => c.method === 'update') + .map((c) => c.args[0] as Record) + + expect( + sentPayloads.some((p) => p.is_completed === false), + 'the held-back un-tick must not be pushed', + ).toBe(false) + }) + + it('does NOT hold back changes to a different record', async () => { + const failing = await seed({ targetId: 'item_1' }) + const other = await seed({ targetId: 'item_2' }) + + holder.supabase = makeFakeSupabase({ + checklist_instance_items: [ + { error: { message: 'denied by policy', code: '42501' } }, // item_1 → terminal + UPLOAD_OK, // item_2 → succeeds + ], + }) + + const engine = new SyncEngine('u1') + await engine.processOutbox() + + expect((await mutationRow(failing))?.failed).toBe(true) + expect( + await mutationRow(other), + 'an unrelated record must still drain — blocking everything was the bug this replaced', + ).toBeUndefined() + }) +}) + +describe('stalled-outbox visibility threshold', () => { + it('is a small positive number so a stuck queue surfaces within a shift', () => { + expect(STALLED_NETWORK_ATTEMPTS).toBeGreaterThan(0) + expect(STALLED_NETWORK_ATTEMPTS).toBeLessThanOrEqual(10) + }) +})