docs(03): gap-closure plans 03-09..03-12 for write-path review findings
This commit is contained in:
Lucas Berger
@@ -0,0 +1,158 @@
|
||||
---
|
||||
phase: 03-event-write-back-pwa-install
|
||||
plan: 11
|
||||
type: tdd
|
||||
wave: 3
|
||||
depends_on: ["03-10"]
|
||||
gap_closure: true
|
||||
autonomous: true
|
||||
requirements: [CAL-05, CAL-06]
|
||||
files_modified:
|
||||
- apps/api/src/broker/outboxWorker.ts
|
||||
- apps/api/tests/broker/outboxWorker.test.ts
|
||||
must_haves:
|
||||
truths:
|
||||
- "An edit-as-move delete row never dispatches until its paired create row has reached status='done' — durably, across separate drain cycles"
|
||||
- "Two overlapping drain cycles never both dispatch the same outbox row"
|
||||
- "A same-calendar update re-reads the freshest etag from calendarEvents just before PUT, so rapid successive edits do not spuriously 412"
|
||||
artifacts:
|
||||
- path: apps/api/src/broker/outboxWorker.ts
|
||||
provides: "Durable create-before-delete gating, drain concurrency guard, fresh-etag-before-PUT"
|
||||
contains: "isDraining"
|
||||
key_links:
|
||||
- from: "runOutboxDrain"
|
||||
to: "calendarOutbox status machine"
|
||||
via: "in-flight claim / blocked-delete gate persisted in DB, not an in-memory Set"
|
||||
pattern: "isDraining|processing|blocked"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Close the outbox durability and concurrency holes. The create-before-delete
|
||||
ordering for edit-as-move (D-04) is enforced only by an in-memory `Set` that holds
|
||||
within a single drain batch — a move pair straddling batches can delete the original
|
||||
before the new copy is confirmed (CR-04, the exact "lost event" D-04 forbids). There
|
||||
is also no guard against overlapping 15s drain cycles double-dispatching the same
|
||||
still-`pending` row (CR-05), and same-calendar updates trust a stale enqueue-time etag
|
||||
that guarantees a spurious 412 on a second quick edit (WR-02).
|
||||
|
||||
Purpose: the outbox is durable and non-duplicating under real timing.
|
||||
Output: a worker whose ordering and exactly-once guarantees survive across drain cycles.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/PROJECT.md
|
||||
@.planning/STATE.md
|
||||
@.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md
|
||||
@.planning/phases/03-event-write-back-pwa-install/03-10-SUMMARY.md
|
||||
@apps/api/src/broker/outboxWorker.ts
|
||||
@apps/api/src/db/schema.ts
|
||||
@apps/api/src/broker/sync.ts
|
||||
</context>
|
||||
|
||||
<artifacts_this_phase_produces>
|
||||
No new exported symbols. Adds a module-level `isDraining` guard in outboxWorker.ts
|
||||
and durable status gating for paired delete rows (reusing the existing `calendarOutbox`
|
||||
`status` enum and `groupId` column — no schema migration required: a paired delete is
|
||||
gated by querying its sibling create's status, not enqueued as a new enum value).
|
||||
</artifacts_this_phase_produces>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 1: RED+GREEN — durable create-before-delete + drain concurrency guard (CR-04, CR-05)</name>
|
||||
<files>apps/api/src/broker/outboxWorker.ts, apps/api/tests/broker/outboxWorker.test.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/broker/outboxWorker.ts (failedCreateGroups Set lines 271-296; per-batch sort lines 263-269; startOutboxWorker schedule lines 359-365; runOutboxDrain entry line 247)
|
||||
- apps/api/src/db/schema.ts (calendarOutbox: status enum pending|done|failed|dead, groupId, lines 125-154)
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (CR-04, CR-05)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- RED (CR-04 cross-batch): simulate a move pair where the create row and the delete row are returned in SEPARATE drain calls. Drain 1 returns only the delete row (create not yet done). Assert the delete is NOT dispatched (deleteCalendarEvent not called) because the paired create (queried by groupId) is not `done`. Drain 2, after the create has reached `done`, dispatches the delete. Fails today (the in-memory Set is empty in drain 1, so the delete proceeds).
|
||||
- RED (CR-05): invoke `runOutboxDrain` twice concurrently (do not await the first before starting the second) against the same pending row; assert the dispatch wrapper (createCalendarEvent) is invoked exactly once across both calls.
|
||||
</behavior>
|
||||
<action>
|
||||
CR-04 — make the ordering durable. For a `delete` row that has a `groupId`, before dispatching,
|
||||
query calendarOutbox for the sibling row with the same `groupId` and `operation='create'`:
|
||||
- if that sibling is not yet `done`, SKIP this delete this cycle (leave it `pending` for a later
|
||||
drain) — do not rely on `failedCreateGroups` co-occurring in the batch;
|
||||
- if the sibling create is `failed`/`dead`, skip the delete permanently per D-04 (mark it `failed`
|
||||
with a "paired create did not succeed" lastError so the original event is preserved).
|
||||
Keep the within-batch create-before-delete sort as a fast path, but the DB sibling-status query is
|
||||
the authoritative gate. Remove reliance on `failedCreateGroups` as the sole cross-cycle mechanism.
|
||||
|
||||
CR-05 — add a module-level `let isDraining = false`. At the top of `runOutboxDrain`, if `isDraining`
|
||||
return immediately; else set `isDraining = true` and wrap the whole drain body in try/finally that
|
||||
resets `isDraining = false`. The 15s scheduler in `startOutboxWorker` already calls runOutboxDrain;
|
||||
the guard makes an overlapping invocation a no-op. (Document that a more robust DB row-claim is a
|
||||
future option, but the in-process guard is sufficient for the single-worker two-user deployment.)
|
||||
|
||||
Commit RED then GREEN.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- behavior: a delete whose paired create is not `done` is not dispatched, even when they arrive in different drain cycles.
|
||||
- behavior: a paired create that ends failed/dead causes the delete to be skipped (original event preserved).
|
||||
- behavior: two overlapping runOutboxDrain calls dispatch each row exactly once.
|
||||
- source: `grep -c 'isDraining' apps/api/src/broker/outboxWorker.ts` returns >= 2.
|
||||
- test-command: `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts` passes.
|
||||
</acceptance_criteria>
|
||||
<done>The create-before-delete invariant holds across drain cycles and overlapping cycles never double-apply a row.</done>
|
||||
</task>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 2: RED+GREEN — re-read freshest etag before PUT to avoid spurious 412 (WR-02)</name>
|
||||
<files>apps/api/src/broker/outboxWorker.ts, apps/api/tests/broker/outboxWorker.test.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/broker/outboxWorker.ts (update dispatch lines 158-173; the etag comes from row.etag captured at enqueue time)
|
||||
- apps/api/src/db/schema.ts (calendarEvents.etag line 95; calendarEvents.uid line 94)
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (WR-02)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- RED: an update row carries a stale `etag` ('old-etag'), but calendarEvents has been re-synced to 'new-etag'. Assert updateCalendarEvent is called with 'new-etag' (the freshest value read from calendarEvents at dispatch time), not the row's stale 'old-etag'. Fails today (row.etag is used verbatim).
|
||||
</behavior>
|
||||
<action>
|
||||
In the `operation === 'update'` branch of `dispatchRow`, before calling `updateCalendarEvent`,
|
||||
re-read the freshest etag for this object from `calendarEvents` (select `etag` where
|
||||
`uid = row.uid`, taking the row whose calendar matches `row.calendarUrl` if needed). Use that
|
||||
fresh etag for the If-Match instead of `row.etag` when present; fall back to `row.etag` if the
|
||||
DB read returns nothing. This coalesces rapid successive same-uid edits against the latest
|
||||
server state rather than the enqueue-time snapshot, preventing the guaranteed-412-on-second-edit
|
||||
described in WR-02. Do NOT weaken conflict detection for genuine third-party changes — the fresh
|
||||
etag still reflects the last synced server state, so a real external edit still 412s (D-08 intact).
|
||||
|
||||
Commit RED then GREEN.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- behavior: the update PUT uses the freshest calendarEvents.etag, not the stale enqueue-time etag.
|
||||
- behavior: when calendarEvents has no row for the uid, the worker falls back to row.etag.
|
||||
- source: the update branch reads calendarEvents.etag at dispatch time (grep for a select against calendarEvents inside the update path).
|
||||
- test-command: `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts` passes.
|
||||
</acceptance_criteria>
|
||||
<done>Rapid successive same-calendar edits no longer fire a spurious conflict toast; genuine external changes still 412 (D-08 preserved).</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
- `cd apps/api && npx vitest run tests/broker/` green.
|
||||
- `cd apps/api && npm run build` succeeds.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
The outbox is durable (create-before-delete across cycles), non-duplicating (concurrency guard),
|
||||
and avoids spurious conflicts (fresh-etag). CR-04, CR-05, WR-02 closed.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/03-event-write-back-pwa-install/03-11-SUMMARY.md` when done.
|
||||
</output>
|
||||
Reference in New Issue
Block a user