diff --git a/data/repo-awareness-snapshot.json b/data/repo-awareness-snapshot.json index 42eb385724..c4ba14cf8d 100644 --- a/data/repo-awareness-snapshot.json +++ b/data/repo-awareness-snapshot.json @@ -2454,6 +2454,11 @@ "section": "decisions", "catalogued": true }, + { + "path": "docs/decisions/2026-10-16-stuck-final-attempt-uses-existing-recovery.md", + "section": "decisions", + "catalogued": true + }, { "path": "docs/developer-area-access.md", "section": "root", @@ -23693,6 +23698,14 @@ "outcome": "P2 findings confirmed: complete review corpus omitted legacy/archive rows; fallback UI counts disagreed with rendered groups. Smallest fixes applied; no additional P0-P2 findings in changed scope.", "checks": "Hosted failure root cause: stale review_state snapshot; focused Vitest first attempt blocked by repository coordinator capacity; local snapshot gate reproduced expected review_state drift." }, + { + "date": "2026-10-16", + "ref": "fix/adversarial-remediation-20261016", + "head": "d5775ec60c00faff2b1a2485c4ee84f53b915a56", + "scope": "Adversarial assessment + parallel fix swarm + post-fix adversarial review across calculators, handover privacy, teaching QR/attendance, EPA magic links, and roster swaps (zero RAG diff)", + "outcome": "All P0/P1/P2 findings fixed and verified; zero RAG files touched", + "checks": "typecheck (10171 files), eslint --max-warnings=0 (33 code files), check:diff-integrity, check:forms-review-sheet, check:source-acquisitions, vitest (14 suites, 344/344 passed)" + }, { "date": "2026-07-31", "ref": "codex/moderate-batch-20260730", @@ -24957,6 +24970,14 @@ "outcome": "merged origin/main; resolved outstanding-issues against #1508 IDs; kept pre-snapshot wording", "checks": "check:outstanding-issues,docs:check-links" }, + { + "date": "2026-10-16", + "ref": "feat/backlog-remediation-20261016", + "head": "e526f8a4d5ad00ad05c5b780262b84a5e347938c", + "scope": "prlanded", + "outcome": "PR #3571 squash-merged cleanly into main (e526f8a4d5ad00ad05c5b780262b84a5e347938c); two-dot tree diff against pushed head 7c00d1f35e960dc9ca92f4d6c4d50ccd1242724e is empty.", + "checks": "gh pr view 3571 (MERGED), git rev-list --parents -n 1 e526f8a4d5ad00ad05c5b780262b84a5e347938c (1 parent 24eeea365e782c651047eebf0ecbe9db87e4fd07), git diff --stat e526f8a4d5ad00ad05c5b780262b84a5e347938c 7c00d1f35e960dc9ca92f4d6c4d50ccd1242724e (empty)" + }, { "date": "2026-07-17", "ref": "main (origin/main)", diff --git a/docs/decisions/2026-10-16-stuck-final-attempt-uses-existing-recovery.md b/docs/decisions/2026-10-16-stuck-final-attempt-uses-existing-recovery.md new file mode 100644 index 0000000000..9b5ad9af2e --- /dev/null +++ b/docs/decisions/2026-10-16-stuck-final-attempt-uses-existing-recovery.md @@ -0,0 +1,8 @@ +# Decision: An upload stuck on its final attempt is cleared by the existing recovery, not a new database fix + +- **Status:** decided 2026-10-16 +- **Source:** `docs/ingestion-state-machine.md`, find "The owner decided on 2026-10-16 not to add" + +An ingestion job whose last attempt ends with an uncertain outcome stays in processing, because the claim step skips jobs that have used all their attempts. The owner chose not to add a database change for this. The 6-hourly ingestion check flags the job when it is among the 25 oldest open jobs, and the existing queue recovery tool resets it. That tool applies at most 20 fixes per run by default, so with a larger backlog it is rerun or given a higher limit. + +If this summary and the source ever differ, the source wins. diff --git a/docs/ingestion-state-machine.md b/docs/ingestion-state-machine.md index 8d9c2905c9..0a3065a869 100644 --- a/docs/ingestion-state-machine.md +++ b/docs/ingestion-state-machine.md @@ -194,6 +194,16 @@ was no lease heartbeat at the time of this snapshot, every worker window below a **Worker (W1)** - C1: crash after claim → job `processing` until stale reclaim; attempt burned. +- C1a (2026-10-16): on a job's final attempt, an uncertain outcome (a crash, a lost + connection, or SQLSTATE 40003) leaves the job `processing` with no stale reclaim, because + `claim_ingestion_jobs` skips jobs at `max_attempts`. The owner decided on 2026-10-16 not to add + an automatic database recovery for this. The 6-hourly ingestion autopilot reads only the 25 + oldest open jobs (`scripts/reindex-health.ts`) and flags a `processing` job among them locked for + more than 30 minutes. A stuck job behind a larger backlog is therefore flagged only once the + older jobs ahead of it clear. Queue recovery (T16) reads every open job but applies at most + `--limit` actions per run (default 20), and changes nothing until confirmed (interactively, or + by `--apply` or `--yes`). With a larger backlog, rerun it or raise `--limit` until the job is + reset. - C2 (non-atomic only): crash after reset → document has zero index rows and `D:processing` until reclaim. Steady-state docs are safe (atomic path skips T3) — but see R15 for how the retry route re-opens this for indexed docs. diff --git a/src/app/api/upload/route.ts b/src/app/api/upload/route.ts index 7bffe0039e..41cfeb72a8 100644 --- a/src/app/api/upload/route.ts +++ b/src/app/api/upload/route.ts @@ -22,6 +22,7 @@ import { assertTrustedDocumentStoragePath, createTrustedDocumentAdmission, dispatchTrustedDocumentAdmission, + releaseSupersededDocumentObject, TrustedDocumentAdmissionDispatchError, TrustedDocumentStorageUploadError, trustedAdmissionDocumentId, @@ -172,6 +173,48 @@ async function duplicateUploadResponse(args: { }); } +// Each cleanup call gets a short deadline so a committed upload is not held open by storage or +// ledger stalls (about 10 s worst case); a timed-out removal falls back to the ledger. +const SUPERSEDED_RELEASE_TIMEOUT_MS = 5_000; + +async function releaseSupersededUpload(args: { + ownerId: string; + documentId: string; + supersededPath: string | null; + committedPath: string; +}) { + if (!args.supersededPath) return; + const supersededPath = args.supersededPath; + const supabase = createAdminClient({ timeoutMs: SUPERSEDED_RELEASE_TIMEOUT_MS }); + // The admission is already durable, so a failure here is logged and never fails the upload. + const release = await releaseSupersededDocumentObject({ + remove: (paths) => supabase.storage.from(env.SUPABASE_DOCUMENT_BUCKET).remove(paths), + queueCleanup: (path) => + supabase.from("storage_cleanup_jobs").insert({ + document_bucket: env.SUPABASE_DOCUMENT_BUCKET, + document_paths: [path], + owner_id: args.ownerId, + status: "pending", + image_bucket: env.SUPABASE_IMAGE_BUCKET, + image_paths: [], + }), + ownerId: args.ownerId, + documentId: args.documentId, + supersededPath, + committedPath: args.committedPath, + }).catch((error: unknown) => ({ + outcome: "unrecorded" as const, + failures: [error instanceof Error ? error.message : "release failed"], + })); + if (release.outcome === "unrecorded") { + logger.error("Superseded upload object cleanup failed; object requires manual reconciliation", { + storagePath: supersededPath, + ownerId: args.ownerId, + failures: release.failures, + }); + } +} + export async function POST(request: Request) { let supabase: ReturnType | null = null; let uploadedPath: string | null = null; @@ -411,6 +454,7 @@ export async function POST(request: Request) { return publicErrorResponse(`Upload is paused. ${health.message}`, 503, { code: "upload_unavailable" }); assertUploadNotAborted(request); + const intendedStoragePath = admissionStoragePath; let storageUpload; try { storageUpload = await uploadTrustedDocumentBytes({ @@ -432,6 +476,9 @@ export async function POST(request: Request) { throw storageError; } uploadedPath = storageUpload.storagePath; + // A collision left an identical object at the intended path; once this request's admission + // commits on the recovery sibling, that object is referenced by nothing. + const supersededStoragePath = storageUpload.collisionRecovered ? intendedStoragePath : null; if (storageUpload.storagePath !== admissionStoragePath) { admissionStoragePath = storageUpload.storagePath; documentPayload = { ...documentPayload, storage_path: admissionStoragePath }; @@ -475,6 +522,12 @@ export async function POST(request: Request) { resourceId: documentId, metadata: { fileType: file.type, fileSize: file.size }, }); + await releaseSupersededUpload({ + ownerId: uploadOwnerId, + documentId, + supersededPath: supersededStoragePath, + committedPath: admissionStoragePath, + }); return NextResponse.json({ document: recovered.document, job: recovered.job }, { status: 201 }); } } catch { @@ -583,6 +636,12 @@ export async function POST(request: Request) { // record, not in the durable audit trail. metadata: { fileType: file.type, fileSize: file.size }, }); + await releaseSupersededUpload({ + ownerId: uploadOwnerId, + documentId, + supersededPath: supersededStoragePath, + committedPath: admissionStoragePath, + }); return NextResponse.json({ document, job }, { status: 201 }); } catch (error) { diff --git a/src/lib/trusted-document-admission.ts b/src/lib/trusted-document-admission.ts index 1db35ff753..678829b33e 100644 --- a/src/lib/trusted-document-admission.ts +++ b/src/lib/trusted-document-admission.ts @@ -345,6 +345,53 @@ export async function uploadTrustedDocumentBytes( return Object.freeze({ storagePath: recoveryPath, collisionRecovered: true }); } +type SupersededObjectRelease = Readonly<{ + outcome: "skipped" | "removed" | "queued" | "unrecorded"; + failures: readonly string[]; +}>; + +function releaseFailureMessage(step: string, error: unknown): string { + const message = error instanceof Error ? error.message : isRecord(error) ? String(error.message ?? "") : ""; + return `${step}: ${message.slice(0, 200) || "unknown error"}`; +} + +/** + * After this request's own admission commits on a recovery sibling, the collided object at the + * intended path is referenced by nothing: the document id is fixed by the correlation id, so no + * other admission for it can commit any more. Remove it, or queue it on the cleanup ledger. + * Never call this before the admission is durable, or for a path the committed row still uses. + */ +export async function releaseSupersededDocumentObject( + args: Readonly<{ + remove(paths: string[]): PromiseLike<{ error: { message: string } | null }>; + queueCleanup(path: string): PromiseLike<{ error: { message: string } | null }>; + ownerId: string; + documentId: string; + supersededPath: string; + committedPath: string; + }>, +): Promise { + const supersededPath = assertTrustedDocumentStoragePath(args.ownerId, args.documentId, args.supersededPath); + const committedPath = assertTrustedDocumentStoragePath(args.ownerId, args.documentId, args.committedPath); + if (supersededPath === committedPath) return { outcome: "skipped", failures: [] }; + const failures: string[] = []; + try { + const { error } = await args.remove([supersededPath]); + if (!error) return { outcome: "removed", failures }; + failures.push(releaseFailureMessage("remove", error)); + } catch (error) { + failures.push(releaseFailureMessage("remove", error)); + } + try { + const { error } = await args.queueCleanup(supersededPath); + if (!error) return { outcome: "queued", failures }; + failures.push(releaseFailureMessage("queue", error)); + } catch (error) { + failures.push(releaseFailureMessage("queue", error)); + } + return { outcome: "unrecorded", failures }; +} + function isRecord(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value); } diff --git a/tests/trusted-document-admission.test.ts b/tests/trusted-document-admission.test.ts index 720f055e04..d303542918 100644 --- a/tests/trusted-document-admission.test.ts +++ b/tests/trusted-document-admission.test.ts @@ -4,6 +4,7 @@ import { describe, expect, it, vi } from "vitest"; import { assertTrustedAdministratorAssurance, createTrustedDocumentAdmission, + releaseSupersededDocumentObject, dispatchTrustedDocumentAdmission, toTrustedDocumentAdmissionRpcPayload, TrustedDocumentAdmissionDispatchError, @@ -526,3 +527,74 @@ describe("trusted document Storage upload recovery", () => { expect(download).not.toHaveBeenCalled(); }); }); + +describe("superseded collision object release", () => { + const intended = `${administratorId}/documents/${documentId}/guideline.pdf`; + const recovery = `${administratorId}/documents/${documentId}/recovery-cccccccc-cccc-4ccc-8ccc-cccccccccccc-guideline.pdf`; + const ok: { error: null } = { error: null }; + const failed = { error: { message: "storage unavailable" } }; + type StorageResult = { error: { message: string } | null }; + const release = ( + remove: (paths: string[]) => PromiseLike, + queueCleanup: (path: string) => PromiseLike, + supersededPath = intended, + ) => + releaseSupersededDocumentObject({ + remove, + queueCleanup, + ownerId: administratorId, + documentId, + supersededPath, + committedPath: recovery, + }); + + it("removes the collided object once the recovery sibling is committed", async () => { + const remove = vi.fn(async () => ok); + const queueCleanup = vi.fn(); + await expect(release(remove, queueCleanup)).resolves.toEqual({ outcome: "removed", failures: [] }); + expect(remove).toHaveBeenCalledWith([intended]); + expect(queueCleanup).not.toHaveBeenCalled(); + }); + + it("queues the object on the cleanup ledger when removal fails or throws", async () => { + for (const remove of [vi.fn(async () => failed), vi.fn(async () => Promise.reject(new Error("network")))]) { + const queueCleanup = vi.fn(async () => ok); + await expect(release(remove, queueCleanup)).resolves.toMatchObject({ outcome: "queued" }); + expect(queueCleanup).toHaveBeenCalledWith(intended); + } + }); + + it("reports an unrecorded object, with both failure reasons, when removal and the ledger fail", async () => { + await expect( + release( + vi.fn(async () => failed), + vi.fn(async () => failed), + ), + ).resolves.toEqual({ + outcome: "unrecorded", + failures: ["remove: storage unavailable", "queue: storage unavailable"], + }); + await expect( + release( + vi.fn(async () => failed), + vi.fn(async () => Promise.reject(new Error("db"))), + ), + ).resolves.toEqual({ outcome: "unrecorded", failures: ["remove: storage unavailable", "queue: db"] }); + }); + + it("never touches the path the committed document still uses", async () => { + const remove = vi.fn(); + const queueCleanup = vi.fn(); + await expect(release(remove, queueCleanup, recovery)).resolves.toEqual({ outcome: "skipped", failures: [] }); + expect(remove).not.toHaveBeenCalled(); + expect(queueCleanup).not.toHaveBeenCalled(); + }); + + it("refuses a path outside this document's own storage prefix", async () => { + const remove = vi.fn(); + await expect( + release(remove, vi.fn(), `${administratorId}/documents/dddddddd-dddd-4ddd-8ddd-dddddddddddd/guideline.pdf`), + ).rejects.toThrow(); + expect(remove).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/upload-duplicate-cleanup-ledger.test.ts b/tests/upload-duplicate-cleanup-ledger.test.ts index e4771bf24d..61618ccf1c 100644 --- a/tests/upload-duplicate-cleanup-ledger.test.ts +++ b/tests/upload-duplicate-cleanup-ledger.test.ts @@ -11,3 +11,21 @@ describe("duplicate upload cleanup ledger", () => { expect(uploadRoute).toMatch(/if \(cleanupLedgerError\) \{[\s\S]*throw new Error\(/); }); }); + +describe("superseded collision object cleanup", () => { + it("releases the collided intended-path object only after this request's admission is durable", () => { + const calls = [...uploadRoute.matchAll(/await releaseSupersededUpload\(/g)].map((match) => match.index ?? -1); + expect(calls).toHaveLength(2); + for (const call of calls) { + const durable = uploadRoute.lastIndexOf("durableAdmission = true;", call); + expect(durable, "durable admission precedes the release").toBeGreaterThan(-1); + expect(uploadRoute.slice(durable, call)).not.toContain("catch"); + } + expect(uploadRoute).toContain("storageUpload.collisionRecovered ? intendedStoragePath : null"); + for (const call of calls) { + const callArgs = uploadRoute.slice(call, uploadRoute.indexOf("});", call)); + expect(callArgs).toContain("supersededPath: supersededStoragePath,"); + expect(callArgs).toContain("committedPath: admissionStoragePath,"); + } + }); +});