One walk at a time, and a claim that survives a concurrent restore

[ergofobe]

Two concurrency bugs on the same seam: the boot-time walks and the ingest claim they share a queue with.

#92 — a restart during a walk started a second one alongside it

scheduleFaceRepair and scheduleContentHashBackfill enqueued unconditionally. A restart mid-walk leaves the chain's {after: X} job queued — reclaimStale only ever looks at running rows — so boot put a fresh {} beside it and both ran. The face repair is the expensive one: the fresh walk re-runs ONNX detection over the whole library from the start.

JobQueue.enqueueUnique is the guard, under a per-name advisory lock rather than a bare check-then-insert — the rows being counted do not exist yet, so there is nothing to take a row lock on, and two servers booting together would both read an empty queue. A chain's own re-enqueue stays on enqueue: it runs while its own row is running and would otherwise refuse to continue.

On the "head-origin chain" gate that was rejected before. That flag could not discriminate, because a resumed chain descends from a head chain too — the ancestry is the same. This does not look at ancestry at all. It asks a question about the present: is there a walk in the queue right now? One chain exists or none does, and {after: X} and {} answer identically. The scheme record is then honest for free: with a single chain, reaching a short batch means the library really has been walked.

That one-chain rule has a consequence the review caught, and the fix is in this PR: a chain pages by id, so it is only resumable under the rule it started with. An upgrade is the one event that both bumps CONTENT_HASH_SCHEME and restarts the process, so resuming the interrupted chain would cover the tail of the library and then record the new scheme as walked over a head it never read. The chain now carries the scheme it is walking and starts again from the beginning when that is no longer current. A payload with no scheme cannot name the rule it began under and is treated the same way: a needless pass is the cheap mistake, a stale hash the expensive one.

#93 — a lost restore race returned undefined and the INSERT tripped the unique index

claimExistingAsset selected a trashed row and restored it under a guard on deletedAt, so a row somebody else had restored in between read exactly like a row the sweep had destroyed: no match, undefined, and the caller fell through to an INSERT that tripped assets_owner_checksum_key — a 500 where the answer was duplicate: true.

The guard was redundant for the case it was written for. sweepTrash deletes the row, so its absence already says the photograph is gone; keying the restore on the id alone reads that the same way (empty RETURNING) and stops conflating it with a concurrent restore. The row lock settles the rest: the sweep's own delete is guarded on isDoomed, so whichever side commits first, the other re-evaluates and finds the row restored or gone rather than both proceeding. restored now means "trashed when this claim looked, live now", which is what it is for — it drives faces.refreshFor, and that recount is owed either way.

api/uploads.ts does not take the ingest advisory lock, deliberately. Three reasons. It inserts no asset, so it has no unique index to trip — the lock in claimIdentity guards an insert, and there is none on this path. The explicit restore route does not hold that lock either, so the claim has to be right without it; a lock could only narrow the window, not close it. And putting every resumable begin behind one per-owner key is exactly the contention that drove FaceService off a per-owner lock after a live import logged thousands of canceling statement due to lock timeout.

Tests

Every one was written first and watched fail.

• A restart during a repair or backfill walk starts a second one alongside it #92 — four scheduler tests ({after: X} queued, and the same row flipped to running) plus four on the primitive in queue.test.ts. All four schedulers returned true and enqueued a second walk before the fix.
• A lost restore race returns undefined and the INSERT then trips the unique index #93 — two real interleaved transactions, made deterministic by a row lock rather than a sleep. heldOpen applies a change in its own transaction and leaves it uncommitted, so the row lock is still held when it returns. The claim's SELECT reads the pre-commit snapshot and sees the row trashed; its UPDATE then blocks on that lock instead of racing for it, and the test decides when the other side commits. Nothing depends on which side wins a timer. Before the fix the restore case failed with duplicate key value violates unique constraint "assets_owner_checksum_key" → HTTP 500. The companion test holds a DELETE open the same way and proves the swept row still stores the photograph afresh.

Found, not fixed

• The maintenance chores do not recur. scheduleMaintenance enqueues SWEEP_TRASH_JOB, PRUNE_UPLOADS_JOB and PRUNE_JOBS_JOB 60 seconds after boot and nothing re-enqueues any of them. On a long-lived server the trash is swept once per boot and never again, and reclaimStale runs only at boot and once a minute later. Pre-existing, and larger than this PR.
• Consequence this PR trades into. With one chain instead of two, a walk whose worker dies mid-batch sits running inside the 15-minute stale window, boot declines to start a second chain, and the pass waits for a restart landing more than that window after the crash — where before it was replaced by a duplicate walk. Narrowly worse after a crash, much better on the ordinary restart A restart during a repair or backfill walk starts a second one alongside it #92 is about. A recurring reclaimStale closes it; I tried making the prune chore self-chaining and review found that cure worse than the disease (it is the only recurring caller of reclaimStale, so it becomes the one chain with no rescuer, and a death between its self-enqueue and its done write forks it in two for ever). Backed out in cd69e13 — it belongs with the recurrence bug above, not here.
• AdminService.startRepair keeps its own check-then-enqueue rather than moving to enqueueUnique. Its comment says the looseness is deliberate; tightening it is a separate decision.
• Nit: the dedup predicate name = $1 and status in ('queued','running') has no supporting index — jobs_claim_idx is partial on status = 'queued'. It runs twice per boot, so it is cosmetic today.

Review

code-review at medium on origin/main...HEAD, three iterations. Iteration 1 found a blocker (the resumed chain recording a scheme it had not walked) and two nits; iteration 2 found the stranded-walk regression; iteration 3's findings were all in the maintenance change, which is backed out above. The remaining diff reviewed clean, with media/identity.ts explicitly cleared — the reviewer reinstated the old guard and confirmed the new test fails with the 500.

Left open rather than self-merged: a significant is deferred (the stranded-walk delay, above), and the review cap is three iterations.

Verify: bun install && bun run verify && bun run --filter '@imogen/web' build — 953 pass, 0 fail, web bundle built. No bun.lock diff. SDK pin untouched at v0.5.1.

Closes #92
Closes #93

🤖 Generated with Claude Code