fix(board): complete board-detail arbitration after local writes and keep a current 403 authoritative - #2603
Conversation
Labels are part of the board-detail fan-out, so a read that captured the pre-write label set could replace a completed local label write when it resolved after it. Successful create, update and delete now advance the epoch the fan-out is arbitrated against.
Board settings writes now advance the board-detail mutation epoch, so a detail fan-out that captured the pre-save board cannot replace the saved update when it resolves after the write. A background read whose payload is dropped because a local mutation completed mid-flight now queues one successor read. The initiating client receives its own realtime event before the mutation response, so a card move could invalidate its own refresh and leave server-side sibling reordering unrepaired until another event arrived. The queue is single-slot, so repeated invalidations coalesce into that one read and no parallel fan-out is opened. A background HTTP 403 that is current by request generation and was not cancelled now stays authoritative even when a local mutation advanced the data epoch while the read was open. Authorization freshness is a separate rule from board-payload commits: the epoch only says a payload is too old to install, not that the reader still has access. Payload commits and non-authorization explicit failures stay epoch-gated.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review gate (Codex credits exhausted, SC-9): one fresh-context adversarial reviewer on head e7594d5 (merge base 2803355). Verdict: SHIP. Confirmed: every epoch bump (updateBoard, the three label writes, and the existing card and column writes) sits immediately after the API await and before the synchronous local patch with no await between, so a failed write cannot bump and no older fan-out can commit between the patch and the bump; no optimistic local writes exist in the card, column or label stores, so a successor payload can never revert an unacknowledged edit; the successor snapshots the epoch after the invalidating bump and terminates, chains are strictly serial and never loop on errors; isCurrentGeneration implies ownership of the active-fetch slot, so the queue-then-drain handoff cannot start a successor for a superseded read; route change settles the queue then aborts and bumps, unmount settles the slot before nulling the active fetch, and the reverse interleaving is caught by the generation guard; the 403 matrix holds (background 403 after a moved epoch surfaces as the board mutation error; stale-by-generation and cancelled are suppressed; an older background 403 cannot clobber a newer explicit success); #1736's explicit-over-realtime authority is intact; both skins go through BoardView's single arbitration; once-queues are fully consumed per test; facade specs assert through useBoardStore(). Findings by bin:
Evidence at the reviewed head (implementation run): 55 files / 1011 targeted plus 13 further board specs / 253, typecheck, ESLint, build, diff check; red-first for the label-epoch, updateBoard epoch, successor, bound, route-discard, both 403-ordering and both facade specs. Not verified: Playwright, manual browser session. Merge after the fix commit ages and ci-required is green at the new head. |
…egatives Review round 1, no behaviour change. The explicit failure gate is reached with only generation currency, non-cancellation and explicit intent still true, so it covers every status including 403, not only non-authorization failures. Say so, and record why: the view owns an explicit read's outcome and error surface (#2434), while the authorization carve-out above is scoped by #2435 to the background 403. Name the real mechanism behind the successor bound. Repeated events cannot open a parallel read because a background request arriving during an open read joins its promise, and the successor check runs at most once per read. The retitled spec now claims what it proves rather than queue coalescing. Add the missing failure-path negatives for updateLabel and deleteLabel, matching the one createLabel already had.
|
Round 2 (head 9adf800): comment and spec text only, no behaviour change, verified by coordinator read of the worker's report. The explicit-failure gate comment now says explicit reads stay epoch-gated at every status, 403 included, because the view that started the read owns its outcome (#2434), with #2435's authorization carve-out scoped to the background 403; the PR body says the same and states that an explicit Retry that 403s after a local write advanced the epoch returns false silently, unchanged from the merge base. The bound spec is retitled to what it proves (one successor read and no parallel fan-out for repeated mutation events during one background read) with its assertions naming the join branch and the once-per-read check, the rationale comment above queueSuccessorForInvalidatedRead names those as the mechanism, and updateLabel / deleteLabel gain the failure-path negatives createLabel already had. Verified at this head: 55 files / 1013 (1011 at round 1), 13 neighbours / 253, typecheck, scoped ESLint, diff check; npm run build stands at round 1. Round count: 2. Merge after ci-required is green at 9adf800 and the head has aged. |
1 participant
Summary
PR #2433 arbitrated board-detail reads by request generation, cancellation and a per-board data
mutation epoch. Two paths were left incomplete and this PR closes them, plus the authorization
residual recorded on 2026-09-03.
Arbitration before this change.
startBoardFetchstamps every board-detail fan-out with amonotonic request generation and snapshots the board's mutation epoch. A payload commits only when
both still hold (
isCommitEligible). Card and column writes advanced the epoch on success. Boardsettings writes and label writes did not, so a fan-out that captured the pre-save board or label set
could still commit over a completed save, which is the Retry-overlaps-a-save case in the review
thread. A background read invalidated by the epoch simply returned false, so the server-side sibling
reordering that a local card move triggers was never re-read until another realtime event arrived.
On the failure path a single guard,
!isCommitEligible() || axios.isCancel(e), gated every outcome,so a background 403 was swallowed whenever a local mutation had advanced the epoch while that read
was open.
Arbitration after this change. Board settings and label writes advance the epoch on success, so the
whole board-detail payload is covered by one arbitration key. When a background read's payload is
dropped because the epoch advanced under it, and the read is still the current generation, it queues
exactly one successor background read through the existing single-slot queue, which now drains after
any read settles rather than only after an explicit one. Repeated invalidating events during that
window produce exactly one successor, so at most one detail fan-out is ever in flight and the
successor chain ends when mutations stop. On the failure path the gates are now split: generation
currency and cancellation decide whether a failure is reported at all, a background 403 is then
surfaced as the access-revoked outcome regardless of the epoch, and the epoch gate still applies to
payload commits and to explicit failures at every status, 403 included, because the view that started
an explicit read owns its outcome and its error surface (#2434). #2435 scopes the authorization
carve-out to the background 403 only, so an explicit Retry that 403s after a local write advanced the
epoch returns false silently, exactly as it did at the merge base.
Explicit reads deliberately do not queue a successor. Their outcome is owned by the view that
started them, and the view-local error surface is tracked separately by #2434, which this PR does not
touch. #1736's rule that an explicit Retry stays authoritative over realtime refresh is unchanged:
fetchBoardstill settles a queued background read when an explicit intent starts.Closes #2435
Changes
frontend/taskdeck-web/src/store/board/labelStore.ts— successfulcreateLabel,updateLabelanddeleteLabeladvance the board-detail mutation epoch.frontend/taskdeck-web/src/store/board/boardCrudStore.ts— successfulupdateBoardadvances theepoch; a background read invalidated by the epoch queues one successor read; the queue
drains after any read settles; the failure path separates authorization freshness (generation and
cancellation only) from payload commit eligibility (generation, cancellation and epoch).
src/tests/store/board/labelStore.spec.ts,src/tests/store/board/boardCrudStore.spec.ts,src/tests/store/boardStore.spec.ts.No view, composable, router, locale or backend file is touched. Board realtime stays per-board.
Test plan
Verified, from
frontend/taskdeck-webafternpm ci:npx vitest --run --maxWorkers=2 src/tests/store src/tests/views/BoardView.spec.ts src/tests/views/paper/PaperBoardView.spec.ts src/tests/views/paper/PaperBoardManagement.spec.ts src/tests/composables/useBoardRealtime.spec.ts src/tests/resilience— 55 files, 1011 tests passed.npx vitest --run --maxWorkers=2 src/tests/views/BoardView.coverage.spec.ts src/tests/views/BoardView.keyboardRouting.spec.ts src/tests/views/BoardsListView.spec.ts src/tests/views/BoardAccessView.spec.ts src/tests/views/paper/boardMutationCapabilityParity.spec.ts src/tests/property/storeResilience.spec.ts src/tests/composables/useActivityQuery.spec.ts src/tests/composables/useStarterPackCatalog.spec.ts src/tests/composables/useStarterPackImport.spec.ts src/tests/guards/primaryActionGuards.spec.ts src/tests/router/workspaceRouteStability.spec.ts src/tests/components/StarterPackCatalogModal.spec.ts src/tests/api/http.spec.ts— 13 files, 253 tests passed.npm run typecheck— clean.npx eslint src/store/board/boardCrudStore.ts src/store/board/labelStore.ts src/tests/store/board/boardCrudStore.spec.ts src/tests/store/board/labelStore.spec.ts src/tests/store/boardStore.spec.ts— clean.npm run build— succeeded.git diff --check— clean.New regressions are deterministic: deferred promises and explicit epoch bumps, no fake or wall-clock
timers. Red first, confirmed against the unmodified source: the three label-epoch specs, the
updateBoardepoch spec, the successor-read spec, the successor-bound spec, the route-changediscard spec, both 403-after-mutation ordering specs, and both facade-level specs
(
rejects a delayed fan-out after a successful board settings write commits locallyand... after a successful label write commits locally, re-run red with the two epoch calls stubbedout, then restored). The unmount-discard spec was green before the change through a different code
path and is a guard rather than a red-first regression; the explicit-retry spec is a
characterization of behaviour #2433 already had.
Coverage by acceptance point: board settings and label writes advance the epoch (unit plus facade
specs); a background read invalidated by a local mutation queues one successor that commits the
repaired ordering; three invalidating events during the window produce exactly one successor and
never a parallel fan-out; a route load and an unmount both discard the queued successor; a current
background 403 stays authoritative in both mutation-then-rejection and rejection-then-mutation
orderings, and a failed read queues no successor.
NOT verified: Playwright end-to-end (no running stack); no manual browser session; backend untouched
and not run; the full
vitest --runsuite was not run in one pass (it OOMs on this box), only thetargeted board-related files listed above.
Boundaries and risks
boardLoadErroris out of scope, which is why aninvalidated explicit read still queues nothing.
useBoardRealtime.tsis unchanged. Its ownrefreshInFlightguard plusthe store returning the active read to a matching background request keeps the in-flight count at
one.
have committed. That is the intended fix, and for background reads the queued successor re-reads
immediately; for an explicit read the local state is kept and the user's own write is preserved.
server failure cannot produce a retry loop.
.worktrees/codex-2435-board-403-authorityholds only gitignoredfrontend/taskdeck-web/node_modules/and
frontend/taskdeck-web/dist/, both regenerable bynpm ciandnpm run build. Nothing wascopied out; the tree is ready for plain
git worktree remove.Round 2
Review verdict was SHIP with no CRITICAL or HIGH. One batch of comment and test corrections, no
behaviour change. Head
9adf800fd24c612931714f7cab932c848ccf9385.boardCrudStore.tsis reached with onlygeneration currency, non-cancellation and explicit intent still true, so it covers every status
including 403, not just non-authorization failures as the comment claimed. The comment now says
explicit reads deliberately stay epoch-gated at every status because the view owns their outcome
and error surface (Clear stale board-load errors after background recovery #2434), and that Complete board detail arbitration after local writes #2435 scopes the carve-out to the background 403. The Summary
sentence above is corrected the same way. An explicit Retry that 403s after a local write advanced
the epoch returns false silently, unchanged from the merge base.
background requests return the same in-flight promise, and
queueSuccessorForInvalidatedReadisreachable once per read. It is retitled to what it proves, "produces exactly one successor read and
no parallel fan-out for repeated mutation events during one background read", its inline assertions
name the two mechanisms, and the rationale comment in
boardCrudStore.tsnames them too.updateLabelanddeleteLabelasserting the epochdoes not advance when the write fails, matching the one
createLabelalready had.without passing through BoardView's realtime wrapper, so a stale refresh banner stays until an
explicit Retry.
Verified at this head, from
frontend/taskdeck-web:npx vitest --run --maxWorkers=2 src/tests/store src/tests/views/BoardView.spec.ts src/tests/views/paper/PaperBoardView.spec.ts src/tests/views/paper/PaperBoardManagement.spec.ts src/tests/composables/useBoardRealtime.spec.ts src/tests/resilience— 55 files, 1013 tests passed (1011 before, plus the two new label negatives).npm run typecheckclean;npx eslintover the same five changed files clean;git diff --checkclean.NOT verified in round 2:
npm run buildwas not re-run (round 1 build passed and this round changesonly comments and specs); Playwright still not run.