Harden retry/reindex rollback guards against overlapping queue mutations#170
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11993cc19c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .eq("status", "queued") | ||
| .is("error_message", null) | ||
| .eq("page_count", 0) | ||
| .eq("chunk_count", 0) | ||
| .eq("image_count", 0); |
There was a problem hiding this comment.
Use a per-attempt guard for reindex rollback
When the job insert fails after this document reset, a second full reindex can still pass checkIngestionMutationSafety because there is no pending/processing job yet, then write the same queued/null/zero state and successfully insert its job. This rollback still matches that second request's state and restores the first request's old failed counts/status over it, leaving a newly queued job paired with a document that no longer appears queued until a worker claims it; the same copied guard in the bulk route has the same race. Add a per-attempt marker/token or make the reset+insert atomic rather than relying only on status/counts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 928c1feb84
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .eq("status", "queued") | ||
| .is("error_message", null) | ||
| .eq("page_count", 0) | ||
| .eq("chunk_count", 0) | ||
| .eq("image_count", 0); |
There was a problem hiding this comment.
Make reindex rollback attempt-specific
Fresh evidence in this commit is that the new rollback guard still keys only on the shared queued/null/zero reset state. If request A resets a failed document and its job insert later fails, request B can pass the safety check before any pending job exists, reset the same document to the identical state, and insert a job; A's rollback then matches these predicates and restores the old failed status/counts over B's queued document. Use an attempt-specific marker or make reset+insert atomic; the copied bulk guard has the same race.
Useful? React with 👍 / 👎.
| // job is NOT processing, OR its lock is already stale, OR it has no lock. | ||
| const staleThreshold = new Date(Date.now() - env.WORKER_STALE_AFTER_MINUTES * 60_000).toISOString(); | ||
|
|
||
| const retryScheduledAt = new Date().toISOString(); |
There was a problem hiding this comment.
Use a collision-proof retry marker
When two retry requests for the same job are handled in the same millisecond, Date.toISOString() can give both resets the same next_run_at. If the first request's document update then fails after the second reset succeeds, its rollback guard still matches the second request's pending row and restores the old failed job state, leaving the document queued but with no pending retry for the worker to claim. Guard the rollback with a value that changes per update, such as the returned updated_at/a token, rather than this millisecond timestamp alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aece631fb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .update({ | ||
| status: job.status, | ||
| stage: job.stage, |
There was a problem hiding this comment.
Prevent stale retry rollback from clearing a newer enqueue
Fresh evidence in this version is that the reset guard still allows rows already reset to pending via status.neq.processing, so two overlapping retry requests for the same failed job can interleave as: B successfully queues the job/document, A then resets the same pending row, and if A's document update fails this rollback restores A's stale failed snapshot. That leaves the document queued from B but removes the pending job the worker would claim; guard the reset/rollback with the pre-reset row version/state or an attempt-specific token, or make the retry update atomic.
Useful? React with 👍 / 👎.
| const response = await POST(authenticatedRequest(`/api/ingestion/jobs/job-1/retry`, { method: "POST" }), { | ||
| params: Promise.resolve({ id: "job-1" }), |
There was a problem hiding this comment.
Use a valid UUID in retry rollback test
This new regression test passes job-1 as the route param, but the retry route validates id with z.string().uuid() before any Supabase calls. The handler therefore returns Invalid ingestion job id. without issuing the reset/rollback updates, so the added test fails instead of covering the rollback path; use a UUID-shaped job id here and in the matching params.
Useful? React with 👍 / 👎.
| expect(response.status).toBe(400); | ||
| expect(await payload(response)).toEqual({ error: "Request could not be completed." }); |
There was a problem hiding this comment.
Update rollback test to expect the 500 envelope
When the mocked job insert fails, this route throws a plain Error and the outer catch calls jsonError(error) with its default 500 status, yielding { error: "Request failed." }. This new assertion expects the 400 envelope instead, so the regression test fails after the rollback path runs; align it with the route's existing internal-failure contract unless the route behavior is intentionally changed.
Useful? React with 👍 / 👎.
Two fixes for CI failures after main moved: - use a valid UUID job id so the retry test reaches the rollback path instead of being rejected by route param validation (400 Invalid ingestion job id.) - expect 500 / 'Request failed.' for downstream update failures, matching the jsonError contract from the API-semantics fix on main Also merges origin/main into the branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@copilot resolve the merge conflicts in this pull request |
Head branch was pushed to by a user without write access
|
Closing as superseded by #146 (merged to main in 151e4e3). #146 landed the equivalent fix across the same files — the retry/reindex/bulk-reindex rollbacks are now fenced on a per-request stamp (documents.updated_at for the reindex routes, next_run_at for retry) so a stale rollback matches zero rows instead of clobbering newer queued/processing work. This PR had also gone into merge conflict against that change. No unique work is being dropped. |
…1609) * docs: replace the search-bar handoff with a durable decisions record `docs/handoff-search-bar.md` shipped to main in #1555. It existed to carry one unverified commit across a session boundary, and its instructions are now false: it tells the reader that `6917e732` is unverified and that no PR should be opened on it, when #1555 merged exactly that work. Leaving it in the repo means the next person to read it acts on stale gate status. Its durable content — results-bar anatomy, why the filter shelf covers two modes rather than eight, and the two things deliberately not done (the library button stays until nav can preserve the query; Sort does not move into the phone sheet from the shared band) — moves to docs/search-results-bar-decisions.md, verified against current main rather than copied forward: `appliedFilters`/`onClearFilters` still have exactly the two production consumers the doc claims, and the `Open source library` control is still there. Also records the PR-policy body defect that #1555's handoff flagged but never captured: ci.yml's body-sync job reads PR_POLICY_BODY.md from the PR head, so committing that scratch file to main (#1546) replaced every open PR's description, and pr-policy.mjs parses the body as merge-gating input. #1548 deleted the file; nothing stops the next branch adding one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the search-bar decisions-doc review Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: fix review findings on search-bar decisions record Reconcile the twelve results-band modes with shelf scope, name the three sheetless Sort consumers, tighten #230 to heads that contain PR_POLICY_BODY.md, and update #170 so documents/therapy sheets match code. * docs(ledger): record review-fix verification at tip --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
docs/outstanding-issues.md #170 conflicted because both branches rewrote that row's Detail cell — the file deliberately carries no merge driver (union was tried and removed, #133) so overlapping edits fail loudly rather than concatenate. Neither side was a superset: main uniquely explained the closed one-of-N/many-of-N defect and the "unlike #1847" stop nuance, this branch uniquely carried the formulation facet work. Resolved by taking main's file and re-applying a combined row through npm run issues:update rather than hand-editing or taking one side wholesale. result-filter-control.tsx auto-merged cleanly and both changes survive: PR #1857's two aria-label fixes (the "All8" concatenation) and this branch's exported ResultFilterFacetChips plus the narrowed builder return type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
…#1885) * feat(therapy-compass): converge filter sheet onto the shared contract Retires the bespoke TherapyFilterTrigger/TherapyFilterSheet in favour of ResultFilterTrigger/ResultFilterSheet/ResultFilterFacetChips, the shared components every other converged mode already uses. Two facet groups (Topics, Availability) now feed both the desktop rail and the phone sheet from one source, closing the last item in /issues #170. Two decisions this PR resolves rather than silently changes, both stated here per docs/filter-contract.md's own rollout convention: - onClearAll vs the query: the old sheet's Clear all wiped the search query too (phone-only, deliberate). That contradicted filter-contract.md section 6 ("onClearAll never touches the query"). The phone-only justification no longer holds — the shared composer already renders its own always-visible clear-query control at every viewport (master-search-header.tsx) — so onClearAll now wires to clearSearchFilters (filters only), matching the invariant. No contract change needed. - AND-within-group tags: therapy's Topics predicate (select.ts's wantTags.every) narrows rather than widens, unlike the contract's OR-within-group default for facets. This is mode-owned filtering logic the shared component's kind type doesn't dictate, so it stays unchanged; only the UI layer converges. Hint counts are computed by re-running the real predicate with each candidate added, which stays honest either way. Also updates the Playwright and vitest coverage that pinned the retired component's exact shape, and regenerates the design-system adoption manifest. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TtjsdU7PRAKYHV98XJRMF6 * chore(ledger): record review for PR #1885 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TtjsdU7PRAKYHV98XJRMF6 * fix(therapy): make query clearing route-owned * test(therapy): cover deep-link query clearing * fix(therapy-compass): use shared control recipe for clear search * style(therapy-compass): format search screen * fix(therapy): preserve filters when clearing query --------- Co-authored-by: Claude <noreply@anthropic.com>
…onent Deletes the ~500-line bespoke DocumentFilterPanel/DocumentFilterTrigger and rebuilds documents' filter sheet on ResultFilterSheet/ResultFilterTrigger (src/components/clinical-dashboard/result-filter-control.tsx), reusing the dense tier (find-a-filter, collapse-by-default, disclosure headers) that PR C already generalized from documents' own >3-groups rule. This is the last mode in the docs/filter-contract.md rollout. Three small additive extensions to the shared component, all optional and inert for the six modes that adopted earlier: - ResultFilterSheet gains meterContent (the "N of M documents shown" bar, rendered first in the body). - ResultFilterSheet gains footerOverride (replaces the default footer entirely, for documents' "Show N documents" + "Browse all sources"). - resultFilterGroup() gains an optional note (the "one only" annotation on the source-type lens, now that it shares a sheet with facet groups for the first time). toggleTagFacet narrows from SmartDocumentTagFacet to its key. Two testids move from literal strings to the shared component's own derivation (document-filter-clear -> document-filter-panel-clear, document-filter-find -> document-filter-panel-find); document-filter-done and document-filter-browse-library stay unchanged as custom footerOverride JSX. The dead-end facet's sr-only reason text changed from documents' bespoke copy to the shared component's generic message; the guard mechanics (disabled, focusable, click-blocked) are unchanged. decoration-on-text contract checks move to result-filter-control.tsx along with the markup they guard. docs/filter-contract.md's Rollout section is updated to close out; a new /issues follow-up (#312) notes services has the same lens-beside-facets shape without the annotation, and /issues #170 records documents as done. Stacked on claude/artifact-build-ygfit8 (PR C, #1882, not yet merged) — this branch's diff includes PR C's commits until #1882 merges.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
… band The Codex reviewer on PR #1925 is right and my closure was wrong. Verified on main 2d27039: result-filter-control.tsx computes const dense = facetGroups.length > 3 || totalFacetOptions > 20 so formulation — one facet group of nine derived domains, the exact case that opened #309 — evaluates dense=false and still renders a wrapping chip row. The 6-20 full-width tier with its right-aligned count column does not exist, and neither does the nine-option DOM assertion the row asked for. What PR F (#1910) delivered is the upper tier: find-a-filter and collapse for >3 groups or >20 options, which is what documents needed. Section 5 has two thresholds; I conflated them and would have archived the row whose specific band is unbuilt. Replaces the done request with an update recording the partial delivery, what remains, and an explicit stop rule against closing on the strength of the upper tier. #170 is unaffected — mode adoption did complete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
…bt rows (#1942) * refactor(tokens): re-land the --shadow-tight retirement onto --e1 PR #1803 retired the --shadow-tight role alias in favour of the --e1 elevation tier across 49 files and squash-merged as 9d8370a on 2026-08-10. The acf78bf merge on 2026-08-11 silently reverted it, along with six other PRs. This re-applies the retirement against current main: 130 call sites across 67 files, plus both declarations. The alias was a pure pass-through -- `--shadow-tight: var(--e1)` in the light and dark role blocks -- so the substitution is value-preserving. Confirmed for forced-colors too rather than assumed: the `@media (forced-colors: active)` block scopes `:root, .dark`, the same `html` element the alias is declared on, so `--shadow-tight` already resolved through the flattened `--e1: none` there. The .ckb-v2 redeclaration hazard does not bite for the same reason -- .ckb-v2 sits on <html> and .ckb-v2.ckb-v2 outspecifies :root, so both spellings substitute against the winning v2 tier. Two comments survived acf78bf while the code they describe did not: the globals.css note that "the resting-hairline role is gone", and the token test's "unlike the --shadow-tight assertion above". Both are accurate again. The token contract test now sweeps the tracked src tree for both spellings (declaration and var() consumer) instead of only asserting the declaration. A declaration-only check would have caught this particular revert, but only because the declarations happened to come back with the call sites; sweeping both makes the gate independent of which half of a bad merge lands. Mutation-verified in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz * chore(design-system): re-pin the contract ratchets to their measured values `scripts/design-system-contract-baseline.json` is a ceiling, so paying debt down leaves silent headroom behind. Ledger #302 records that pattern: legacyShadowAliases was pinned at 220 against a measured 193, 27 units of unguarded slack, up from 3 units on 2026-08-10. With the previous commit's --shadow-tight retirement applied the gap is wider still -- 220 pinned against 119 measured -- because the reland pays down the debt the acf78bf revert had re-hidden. Four other ratchets had accumulated slack from unrelated work in the same window. legacyShadowAliases 220 -> 119 edgeOwnershipConflicts 27 -> 25 rawPaddingLiterals 67 -> 63 rawGapLiterals 34 -> 32 layoutTransitionExceptions 12 -> 11 Regenerated with --print-debt-baseline rather than hand-edited, so the per-path debtByPath counts move with the totals -- those are what findDebtPathRegressions compares, and the retirement moved them wholesale. Every metric in the diff decreases; nothing is absorbed upward. This is not the baseline refresh #262 warns against. That stop rule forbids refreshing to hide the movement; this pins the movement in so it cannot silently drift back a second time. Mutation-verified: reintroducing one alias in button.tsx now fails at both the total (119 -> 120) and the per-path level. Under the old 220 ceiling the same addition passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz * refactor(tokens): hold the search-band count bubble in a spacing token The active-filter badge sized itself with a raw `h-[1.0625rem] min-w-[1.0625rem]` pair. Ledger #275 tracks that value as leaked debt: it had reached five files, so the fix has always been to tokenise once rather than edit a call site. Re-measured on merged main, the badge role is down to a single call site. #170's convergence landed in the meantime -- document-search- results.tsx now renders the shared control and therapy-compass/ filter-sheet.tsx was deleted outright -- so the leak this row was written about has already been reabsorbed by the extraction. Holding the value in @theme is what stops it leaving again. Two arbitrary values in the same component are deliberately left raw: pr-[0.6875rem] and min-[414px]:max-[429px] -- the repo defines no --breakpoint-* tokens at all, and eight peer sites use the same raw min-[]/max-[] form (359px, 389px, 414px). Naming one window while the peers stay raw is the same drift #275 warns about on another axis, and Tailwind named breakpoints would add variants across the whole utility surface. That belongs in a repo-wide decision, filed separately. The three remaining 1.0625rem hits in mode-nav.tsx and nav-slot-ink.tsx are NOT this token. They size <Icon> glyphs -- a 17px icon against a 12/14/16/20/24 --spacing-icon-* scale -- so folding them under a badge token would merge two roles that only happen to share a number. check:icon-scale deliberately does not flag arbitrary h-[Nrem], so they are a real but separate finding, filed rather than guessed at. The token is also registered in CLINICAL_TWMERGE_THEME.spacing, which tests/tailwind-merge-config.test.ts asserts against the @theme block -- without it `cn()` cannot resolve a conflict on the new utility. Safe by that file's own `tap` reasoning: the single call site is a static string carrying no competing h-*/min-w-* class and never passes through `cn()`, so there is no same-variant pair for declaration to hand to the later class. The entry is protective for future use, not load-bearing today. Value-preserving, and proven rather than inferred: compiling globals.css through @tailwindcss/postcss emits .h-search-band-badge { height: var(--spacing-search-band-badge) } .min-w-search-band-badge { min-width: var(--spacing-search-band-badge) } No ratchet moved, so the ceilings pinned in the previous commit still sit at zero slack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz * docs(design-system): close out DS Track A3 and refresh the stale gate rows Track A3 is `#262`. Its three parts are now all settled, each checked against code rather than against the row that describes it. Part 1 is the --shadow-tight retirement re-landed earlier in this PR. Part 3 shipped in PR #1780 per `#301`: rawPaddingLiterals, rawRadiusLiterals and rawLineHeightLiterals are live baseline keys enforced over both the class and CSS-declaration spellings, plus rawGapLiterals beyond the original ask. Part 2 needs no work, and that had already been adjudicated -- GATES.md section 3 records it, which is why nothing here builds it. The decidable half of step selection shipped on 9 Aug inside check:design-system- contract: a declared @theme step no production surface selects fails the build. The remaining half -- which existing step a component picks -- is documented there as something "nothing mechanical can" gate, being a judgement about the rendered design rather than a property of the source, with a standing instruction not to duplicate the arbitrary-value check check:type-scale already ships. Reading `#262` alone would have sent a session to build it; that is the `#301` failure mode, so the closure record says so explicitly. Section 3's live status rows carried numbers this PR moved. `#301`'s lesson is that a row understating shipped work is a duplicate-work generator, so they are corrected in the same change: legacyShadowAliases 224 -> 119, and the alias is now retired outright rather than "224 left to retire" edgeOwnershipConflicts 27 -> 25 rawPaddingLiterals 67 -> 63 rawGapLiterals 34 -> 32 layoutTransitionExceptions 12 -> 11 Section 5 is left alone deliberately: it is a dated record measured against 8db1e53, not a live status surface, and rewriting its figures would destroy the provenance it exists to hold. Ledger records are queued as immutable inbox requests: `#262`, `#302` and `#275` closed; two carve-outs split out of `#275` filed as their own rows (the repo-wide breakpoint-token decision, and three 17px mode-nav icon glyphs that sit off the --spacing-icon-* scale with no gate covering them). The queued re-land request 210e3db5 is cancelled rather than reconciled -- its headline "67 files on main still use the retired alias" is false as of this branch, so it would open a row wrong on arrival. The request file and the cancellation both survive as provenance for the acf78bf merge loss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz * chore(ledger): record the design-token relands review Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz * fix(issues): retarget the reland record after main reconciled it mid-flight CI failed `docs:check-links` on this branch with Error: cancel request 2e791c01... targets missing pending request 210e3db5... `check-docs-links.mjs` replays the inbox batch to resolve link targets, so an unresolvable request fails it. The cause was a race, not a bad record: PR #1936 reconciled 75 queued requests -- 210e3db5 among them -- while this branch was already in flight. Reconciling moves the request file into `docs/outstanding-issues-inbox/applied/` and allocates it a canonical row, so by the time this branch merged main there was no pending request left for the cancellation to name. Cancelling was the right call against a pending request and is the wrong one against a reconciled row. The cancel is dropped and replaced with a `done` against `#319`, the row 210e3db5 became. That is also the better record: the work is finished rather than withdrawn, so the ledger should carry its outcome and its guard, which a cancellation would have thrown away. Also merges origin/main (this branch was 3 behind) and files two findings the PR preflight surfaced, both deliberately not fixed here: - `check:medication-lexicon-report` has been failing on main for every local `verify:pr-local`, and no CI job runs it -- a grep over .github/workflows finds nothing. It is the last step of the local chain, so it fails preflights while CI stays green. The stale file is a clinical-facing generated document; regenerating it inside a CSS-token PR would bundle a clinical-risk artefact with unrelated chores. - Claude Code web containers can ship Node 22 with no node_modules, which fails `npm ci` on engine-strict before any repo script can run. Re-verified after the merge: the tracked tree still holds zero `--shadow-tight` references, and every pinned ratchet still measures exactly its baseline, so the merge moved no metric and the pins stay honest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz * docs(design): retire shadow-tight guidance * docs(design): retire shadow-tight guidance * docs(design): retire shadow-tight guidance * docs(design): retire shadow-tight guidance --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
This addresses Codex review findings on non-atomic rollback paths in retry and reindex flows that could overwrite newer queued/processing work under overlap. The update constrains rollback writes to the exact state mutated by the current request and adds regression coverage for those race windows.
Retry rollback isolation
pending/queued, unlocked,attempt_count=0, and persistednext_run_atfrom the applied reset).Single-document full reindex rollback hardening
status/error/counts).status=queued,error_message IS NULL, counts0).Bulk full reindex rollback hardening
Regression coverage
Verification
npm run verify:cheapnpm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changednpm run verify:releasebefore release or handoff confidence claimsnpm run format:checknpm run check:production-readinesswhen clinical workflow, privacy, environment, Supabase, source governance, or deployment behavior changednpm run check:deployment-readinesswhen deployment startup, hosting, or rollout behavior changedClinical Governance Preflight
Complete this section when the change touches ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output.
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes