docs(ingestion): work-order for remaining phase-3 concurrency fixes

docs/ingestion-concurrency-fix-workorder.md

@@ -0,0 +1,310 @@
+# Ingestion-concurrency phase-3 — remaining fix work-order
+
+Sequenced, operator-applied plan for the state-machine violations that could
+**not** be safely landed from a chat session. Companion to
+`docs/ingestion-state-machine.md` (§6 findings, §8 backlog) and
+`docs/scale-readiness-review.md`.
+
+Author date: 2026-07-08. All facts below were read from the **live** project
+`Clinical KB Database` (`sjrfecxgysukkwxsowpy`) via read-only `execute_sql`;
+nothing here was applied to live. Function bodies are quoted from
+`pg_get_functiondef` so migrations are derived from live truth, not `schema.sql`
+(which is known-drifted — see R24e and `docs/database-drift-detection.md`).
+
+## Already landed (do not redo)
+
+- **R11, R15/R16, R22, R24d-gate** — merged (PR #346).
+- **R11 janitor-side guard** (`scripts/cleanup-storage.ts` +
+  `src/lib/storage-cleanup-safety.ts`) and **R1 lease heartbeat**
+  (`shouldPersistJobProgress` in `src/lib/ingestion.ts`, worker refresh of
+  `locked_at`) — merged (PR #369).
+
+## Global rules for this release
+
+1. **Never raw-SQL against live.** Every DB change is a committed migration
+   (`supabase/migrations/<ts>_*.sql`) + a matching `supabase/schema.sql` edit +
+   `npm run drift:manifest` (Docker) in the same PR, per
+   `docs/database-drift-detection.md`. Operator applies with `supabase db push`
+   after review.
+2. **Expand/contract for RPC signature changes.** New params are added with
+   defaults so existing callers keep resolving; the fence/behavior only engages
+   once the new param is passed. This lets the **DB migration ship first** and
+   the worker/edge deploy follow, with no broken window.
+3. **Deploy order per item is explicit below.** Items marked
+   _coordinated_ must not have their DB half applied without the paired
+   app/worker/edge deploy reaching a state where the old callers are still
+   safe (guaranteed by rule 2).
+4. **Eval gate.** Any item marked _retrieval-affecting_ must show
+   `npm run eval:retrieval:quality` unchanged (currently **36/36**,
+   content_mrr@10 = 0.924, live 2026-07-08) before defaults change.
+5. **Drift-backlog coordination.** Items touching `ingestion_jobs` indexes
+   (R17) or the `ingestion_job_stages` FK (R24e) overlap the open reconciliation
+   backlog in `docs/database-drift-detection.md` (§ "Reconciliation backlog"
+   items #7 and #8). Land them **through that backlog**, not as a competing PR.
+
+---
+
+## R2 — lease fences on complete / fail (root fix, expand/contract)
+
+**Finding (SILENT-CORRUPTION/OPS-CHURN):** no completion/fail write is fenced by
+the lease holder, so after any 45-min reclaim (R1 heartbeat now makes this rare
+but not impossible) a zombie worker's completion/failure clobbers a
+freshly-reclaimed job. Live signatures confirmed to take **no `p_worker_id`**:
+
+```
+complete_ingestion_job(p_job_id uuid, p_document_id uuid, p_batch_id uuid, p_stage text)
+fail_or_retry_ingestion_job(p_job_id uuid, p_document_id uuid, p_batch_id uuid,
+  p_retry boolean, p_document_status text, p_stage text, p_error_message text,
+  p_next_run_at timestamptz)
+```
+
+**Change (backward-compatible):** add trailing `p_worker_id text default null`
+to both. Fence the **job** update with
+`and (p_worker_id is null or locked_by = p_worker_id)`, capture the row count,
+and when a non-null worker id matched **zero** rows, return
+`jsonb_build_object('ok', false, 'reason', 'lease_lost', …)` **without** running
+the sibling force-complete / document write / batch refresh. When `p_worker_id`
+is null the body is byte-for-byte today's behavior.
+
+`complete_ingestion_job` skeleton (derived from the live body):
+
+```sql
+create or replace function public.complete_ingestion_job(
+  p_job_id uuid, p_document_id uuid, p_batch_id uuid default null,
+  p_stage text default 'indexed', p_worker_id text default null)
+returns jsonb language plpgsql set search_path to 'public','extensions','pg_temp' as $$
+declare v_rows int;
+begin
+  update public.ingestion_jobs
+     set status='completed', stage=p_stage, progress=100, error_message=null,
+         locked_at=null, locked_by=null, completed_at=now()
+   where id=p_job_id and document_id=p_document_id
+     and (p_worker_id is null or locked_by = p_worker_id);
+  get diagnostics v_rows = row_count;
+  if p_worker_id is not null and v_rows = 0 then
+    return jsonb_build_object('ok', false, 'reason', 'lease_lost',
+      'job_id', p_job_id, 'document_id', p_document_id);
+  end if;
+  -- (unchanged) sibling force-complete, batch refresh, ok:true return
+  …
+end; $$;
+```
+
+`fail_or_retry_ingestion_job`: same fence on the **job** update; when fenced and
+0 rows, return `ok:false` and **skip the `documents` update** (do the job update
+before the document write on the fenced path so a lost lease can't still demote
+the document). Non-null path unchanged.
+
+**Files:** migration + `schema.sql` (both RPCs); `worker/main.ts` to pass the
+worker id and abort the job when a call returns `ok:false`.
+**Deploy order:** migration first (no-op for the current worker), then worker
+deploy. _Coordinated; not retrieval-affecting._
+**Verify:** `check:drift` green; a worker smoke run (`worker:once`) completing a
+real job returns `ok:true`; a forced stale-id call returns `ok:false`.
+
+---
+
+## R9 — batch-status lost update (self-contained RPC)
+
+**Finding (AVAILABILITY):** two sibling jobs completing in overlapping txns both
+compute `refresh_import_batch_status` counts from pre-commit snapshots → the
+second write is a lost update → batch stuck `processing` forever. Live body
+confirmed: it `select count(*) … from ingestion_jobs where batch_id=…` then
+`update import_batches …`, with no lock serializing concurrent refreshes.
+
+**Change:** lock the batch row first so the count+update are serialized:
+
+```sql
+-- at the top of refresh_import_batch_status, after the null check:
+perform 1 from public.import_batches where id = p_batch_id for update;
+-- then the existing count(*) … and update … run under that row lock
+```
+
+**Files:** migration + `schema.sql`.
+**Deploy order:** DB only; behavior-preserving. _Not coordinated._
+**Verify:** `check:drift` green; concurrency test — two `complete_ingestion_job`
+calls for the last two jobs of a batch, interleaved, leave the batch terminal.
+
+---
+
+## R7 — attempt_count clamp / stuck pending (self-contained RPC)
+
+**Finding (AVAILABILITY):** a stale-snapshot re-pend can leave a `pending` job
+whose `attempt_count = max_attempts`; the claim filter is
+`j.attempt_count < j.max_attempts`, so it is permanently unclaimable and pins
+its batch. Live claim confirmed: filter `attempt_count < max_attempts`,
+increment `attempt_count = attempt_count + 1` at claim.
+
+**Change (pick one, add a targeted test):**
+(a) let the claim filter accept a re-pended maxed job exactly once —
+`(j.status='pending' and j.attempt_count <= j.max_attempts …)` while keeping
+`< ` for the stale-processing arm — so it gets a final claim that then either
+succeeds or is failed terminally; **or**
+(b) clamp every `attempt_count` write to `least(x, max_attempts)` at the retry
+route + recovery script so a maxed job can't be re-pended into limbo.
+Prefer (b) if the retry/recovery resets are the real source (they set
+`attempt_count = 0`, so the stuck case is narrow) — smaller blast radius.
+
+**Files:** migration + `schema.sql` (option a) or app/script only (option b).
+**Deploy order:** DB only (a) / app only (b). _Not coordinated._
+**Verify:** unit test on the chosen path; `check:drift` for (a).
+
+---
+
+## R23 — cleanup delete predicates lack the job-existence guard (self-contained RPC)
+
+**Finding (SILENT-CORRUPTION):** in
+`cleanup_abandoned_document_index_generations(p_document_id, p_limit, p_dry_run)`
+the job-existence guard lives only in the candidate CTE; the 7 per-table counts
+and 7 deletes re-check generation only, each on a fresh READ COMMITTED snapshot,
+so a reindex claimed after selection has its staged rows deleted table-by-table.
+
+**Change:** repeat the candidate guard inside every delete's `where` — i.e.,
+only delete an artifact row whose `document_id` has **no** open
+(`pending`/`processing`) `ingestion_jobs` row, in addition to the existing
+generation-mismatch predicate. Fetch the exact body first:
+`select pg_get_functiondef('public.cleanup_abandoned_document_index_generations(uuid,integer,boolean)'::regprocedure);`
+then add `and not exists (select 1 from public.ingestion_jobs j where
+j.document_id = <artifact>.document_id and j.status in ('pending','processing'))`
+to each delete (and mirror it in the counts for dry-run parity).
+
+**Files:** migration + `schema.sql`.
+**Deploy order:** DB only; strictly narrows what cleanup deletes. _Not coordinated._
+**Verify:** `check:drift`; dry-run before/after on live shows equal-or-fewer
+candidates.
+
+---
+
+## R17 — partial unique index on open jobs (drift-backlog item #7)
+
+**Finding (OPS-CHURN + structural):** no constraint prevents multiple open jobs
+per document; the reindex enqueue race (R13) and duplicate reindex POSTs (R17)
+rely on advisory guards. A partial unique index makes it structural.
+
+**Change:**
+
+```sql
+create unique index concurrently if not exists
+  ingestion_jobs_one_open_per_document_uidx
+  on public.ingestion_jobs (document_id)
+  where status in ('pending','processing');
+```
+
+Live state check (2026-07-08): **0 pending/processing jobs**, so creation will
+not fail on existing duplicates — but the operator must confirm the queue is
+quiet at apply time (`reindex:health` → `jobs_pending/processing = 0`).
+`concurrently` cannot run inside a txn-wrapped migration; apply it as its own
+statement per the repo's concurrent-index precedent.
+
+**Paired app change (required, same release):** the reindex routes must translate
+a `23505` unique violation on job insert into the existing "already queued" 409,
+not a 500. Update `src/app/api/documents/[id]/reindex/route.ts` and
+`src/app/api/documents/bulk/reindex/route.ts` job-insert error handling
+(the competing-job branch already exists — return its 409 on unique violation).
+
+**Files:** migration + `schema.sql` + both reindex routes + a route test.
+**Deploy order:** ship the route change first (it must tolerate the violation),
+then the index. _Coordinated._ **Coordinate with drift-backlog item #7** which
+is already reshaping `ingestion_jobs` indexes.
+**Verify:** `check:drift`; route test that a second concurrent reindex POST gets
+409, not 500.
+
+---
+
+## R24e — remove the phantom `ingestion_job_stages.job_id` FK from schema.sql (drift-backlog item #8)
+
+**Finding (FRESH-ENV-ONLY):** `schema.sql` declares
+`ingestion_job_stages.job_id → ingestion_jobs(id)`, but **live has no such FK**
+(confirmed: the only FK is `ingestion_job_stages_document_id_fkey → documents`).
+On any `schema.sql`-provisioned env the agent's `stageStart` inserts
+`indexing_v3_agent_jobs.id` into `job_id` and dies with FK 23503, burning
+attempts to terminal `failed`.
+
+**Decision (evidence-based):** the drift backlog item #8 proposes _adding_ the FK
+to live, but a prior live scan found ~253 orphan stage rows and 0 rows whose
+`job_id` resolves to an `ingestion_jobs` row — adding + `validate` would fail and
+the column is populated with agent-job ids, not ingestion-job ids. **Remove the
+FK from `schema.sql`** so fresh/preview envs match live. This supersedes backlog
+item #8 — update that doc when landing.
+
+**Files:** `schema.sql` (drop the constraint declaration) + `drift:manifest`
+regen + delete/annotate the matching drift-allowlist entry + update
+`docs/database-drift-detection.md` item #8.
+**Deploy order:** repo-only; **live is untouched** (already correct).
+**Verify:** `check:drift` green; manifest freshness test passes.
+
+---
+
+## R5 — worker metadata full-replace clobbers concurrent edits (coordinated)
+
+**Finding (SILENT-CORRUPTION):** the worker's three full-`documents.metadata`
+replace sites (commit RPC `p_metadata`, `worker/main.ts` final writes) erase
+concurrent bulk-metadata edits, renames, and agent-state patches under any
+reclaim.
+
+**Change:** replace the full-object writes with server-side JSONB deep-merge
+scoped to worker-owned keys (a `jsonb_set`/`||` merge in the commit RPC and a
+merge helper for the worker writes), or fence the writes on `updated_at`.
+This needs the commit RPC body (`commit_document_index_generation`, 9 params —
+fetch with `pg_get_functiondef`) and worker changes.
+
+**Files:** migration + `schema.sql` (commit RPC) + `worker/main.ts`.
+**Deploy order:** _coordinated_ (RPC + worker). **Touches `worker/main.ts`** —
+sequence after / rebased on any open worker PR. _Not retrieval-affecting._
+**Verify:** concurrency test — a rename during a reclaimed commit survives.
+
+---
+
+## deep-memory delete-scoping (R24c/R24d other half) — NOT a small change; needs design
+
+**Finding:** deep-memory's unscoped deletes of `document_index_units` /
+`document_memory_cards` / `document_sections` wipe the enrichment agent's rows
+too. #346's route-gate closes the **concurrent** arm; this is the **sequential**
+arm (a deep-memory pass after an agent pass).
+
+**Why it is not a quick patch — live-verified 2026-07-08:**
+
+- `document_sections` has `UNIQUE (document_id, section_index)`;
+  `document_index_units` / `document_memory_cards` are PK-only.
+- `generated_by` tagging is inconsistent: index_units 111,263 NULL / 149
+  `local-worker` / 579 `indexing-v3-agent`; sections 28,757 NULL / 6,399 agent
+  (no `local-worker` tag at all); cards 29,974 `local-worker` / 16,496 NULL /
+  6,571 agent. So "delete only my rows" cannot key on a single deep-memory tag —
+  the correct scope is **"everything except `indexing-v3-agent`", NULL-safe**.
+- The NULL-safe PostgREST filter is verified working:
+  `.or("metadata->>generated_by.is.null,metadata->>generated_by.neq.indexing-v3-agent")`
+  selects 111,412 non-agent vs 579 agent index_units.
+- **But it is unsafe for `document_sections`:** 0 documents currently hold both
+  agent and non-agent sections — deep-memory's unscoped delete is what prevents
+  the collision. Preserving agent sections and then re-inserting deep-memory's
+  own `section_index` values would hit the unique constraint (23505 crash);
+  scoping only units+cards leaves agent memory-cards detached via
+  `section_id ON DELETE SET NULL`.
+
+**Required work (design, not patch):** decide the section_index ownership model
+between the agent and deep-memory — e.g. give the agent a disjoint index range
+or stop it writing `document_sections`, or make deep-memory's section write an
+`on conflict (document_id, section_index)` upsert with a defined winner. Then
+scope index_units + memory_cards deletes with the NULL-safe filter above.
+_Retrieval-affecting → eval gate. Touches `src/lib/deep-memory.ts` and the edge
+agent._
+
+**Files:** `src/lib/deep-memory.ts`, `supabase/functions/indexing-v3-agent/*`,
+possibly a migration.
+**Deploy order:** _coordinated_ (worker/route + edge agent). **Do not ship the
+naive scope — it can crash live enrichment.**
+
+---
+
+## Suggested landing order
+
+1. **R24e** (repo-only, unblocks fresh/preview envs) — via drift backlog #8.
+2. **R9**, **R23** (self-contained, behavior-preserving RPCs, DB-only).
+3. **R2 fences** (expand/contract: migration, then worker deploy) — the root fix.
+4. **R7** (small; pick option b if retry/recovery is the source).
+5. **R17** (with the reindex-route 409 change) — via drift backlog #7.
+6. **R5** (coordinated worker + commit RPC).
+7. **deep-memory scoping** — only after the section-ownership design is agreed.
+
+Every DB item ends with `npm run check:drift` green and (for retrieval-affecting
+items) `npm run eval:retrieval:quality` unchanged.