feat(worker): At most one work item per storage config runs at a time

Problem

A Worker runs one work item at a time, but the claim does not look at what else is running: it takes the oldest queued item with FOR UPDATE SKIP LOCKED (EfWorkItemStore.ClaimAsync). When two Workers serve the same storage config, for example a per-user Worker beside an all-user Worker or a second replica, items of that config run at the same time on the same files: a crawl beside a burst detection, or beside a purge (#47).

#47 relies on this not happening. Its purge deletes a soft-deleted asset's proxy files, and the proxy folder derives from the config and the content hash, so a crawl of the same config that ingests the same content as New at that moment would lose its freshly written proxies, and the asset would have no thumbnail for good.

Desired behaviour

At most one item per storage config runs at any time, across all Workers. A Worker does not claim a scoped item while another item with the same StorageConfigId is Running; it claims the next eligible item instead, so other configs' work and unscoped work are not held up.

Unscoped items (TokenCleanup, WorkItemRetention, ProxySweep, an unscoped ProxyDeletion) are not serialised against anything: they do not touch originals, and the proxy sweep is already safe beside a crawl (one-hour age floor, re-check before delete).
Not a global one-item-at-a-time rule: per-user Workers mount different folders and must not wait for each other (decided by the maintainer during the spec: See soft-deleted assets and purge them #47 grilling, 2026-10-28).
Parallelism stays inside a Worker (Worker:MaxConcurrentFiles), not across Workers on the same files.

Acceptance

Two Workers never run two items of the same storage config at once (integration test on real PostgreSQL, concurrent claimers, beside the existing contract tests).
An item of another config, and an unscoped item, are claimed while a config's item runs.
The design note settles the window in which a requeued item's old attempt is still running (lease expired, the old handler not yet cancelled by its fenced heartbeat), and what the rule promises there.
ADR 0008 records the rule.
scripts/mutation-changed.sh at 100% (the claim keeps its // Stryker disable all and ADR 0005 row).

First caller

The claim itself: the config's Crawl and BurstDetection items are serialised by it from the first deploy. #47's purge is the caller that depends on it and is blocked by this issue.

Design

Design note: #337 (comment), as amended by the maintainer's decision (2026-10-28): #337 (comment)

🤖 Generated with Claude Code

Design note

Problem and constraints:

Rule: at most one Running item per StorageConfigId, across all Workers. Unscoped items are not serialised. Decided in the spec: See soft-deleted assets and purge them #47 grilling (2026-10-28).
The claim is one UPDATE … WHERE "Id" = (SELECT … ORDER BY "RequestedAt" LIMIT 1 FOR UPDATE SKIP LOCKED) under READ COMMITTED (EfWorkItemStore.ClaimAsync). A NOT EXISTS (running item of the same config) filter on its own is not enough: two Workers claiming at the same moment each take a different queued item of config C. SKIP LOCKED locks only the row each one picked, and neither sees the other's uncommitted Running. The rule needs something that serialises those two claims, or rejects the second.
The zombie window is wider than feat(worker): At most one work item per storage config runs at a time #337 assumed. WorkExecutor.HeartbeatAsync logs a failed heartbeat and carries on, and the handler stops only on Lost, which needs a database answer. A Worker cut off from PostgreSQL therefore keeps running its handler, including file writes and deletes, after its lease has expired. Meanwhile another Worker's RecoverExpiredLeasesAsync requeues the item, and a new claim starts work on the same config. Database writes are fenced by (Id, Attempt); file side effects are not. Any per-config rule only holds inside the database unless the old attempt also stops itself.
The claim and the other transitions are PostgreSQL-only and stay // Stryker disable all (ADR 0005), covered by integration contract tests (ADR 0008).

Options:

A. NOT EXISTS in the claim, plus a partial unique index ("StorageConfigId") WHERE "Status" = 'Running' AND "StorageConfigId" IS NOT NULL.
- NOT EXISTS makes the claim pass over a config that is busy and take the next eligible item, so other configs and unscoped work are not held up.
- The index makes the rule a schema invariant, which also binds any future path that sets Running. It shows up in docs/schema.sql.
- In a race, the loser's UPDATE fails with a unique violation on that index. The claim recognises it by index name (a PostgresException overload next to IsUniqueViolation) and returns null, so the Worker tries again at the next poll (5 s).
- Cost: a migration and the violation handling.
- Migration risk: if two items of one config are Running when the index is created, CREATE UNIQUE INDEX fails and so does host startup. That only happens if two Workers are already serving one config. The migration first requeues all but the earliest-started running item of each config. Their old attempts see Lost at their next heartbeat and stop.
B. NOT EXISTS, serialised by a transaction-scoped advisory lock. pg_advisory_xact_lock(<claim key>) runs as its own statement in an explicit transaction before the UPDATE. It has to be a separate statement, because under READ COMMITTED a statement's snapshot is taken before a lock inside it is granted. This makes every claim across all Workers take turns; claims are tiny and run every 5 s, so that costs nothing measurable. Cost: the rule lives only in the claim's code, with no schema backing, and a second statement and transaction on every claim.
C. Change the pattern: hold a per-config session advisory lock for the whole run (pg_try_advisory_lock(config) on a connection kept open while the handler runs). Cost: one held connection per running item. It also doesn't close the zombie window, because the server releases the lock only once it notices the dead connection, and the cut-off Worker carries on regardless.

Whichever of these is chosen, the old attempt has to stop itself: the executor cancels the handler once half a lease (30 s by default) has passed since the last heartbeat that succeeded. The deadline must not depend on a heartbeat call returning. store.HeartbeatAsync runs with CancellationToken.None, and against an unreachable database it can hang for Npgsql's connect and command timeouts (15 s + 30 s), or longer on a dead TCP connection, which would use up the whole margin. So the deadline is a timer of its own, running.CancelAfter(lease / 2) through the TimeProvider, re-armed after each successful beat. Each heartbeat call also gets its own timeout of one heartbeat interval, so a hung call doesn't hold up the next one. Recovery needs the full lease (60 s) to expire, so an attempt has stopped, or been asked to stop, at least half a lease before anyone else can claim work for its config. That margin covers clock skew between Workers and the time a handler takes to honour cancellation. The cancelled attempt can't write its outcome either, so lease recovery requeues the item and counts a stall, as it does today. Handlers already honour their token between steps (the handler contract).

Recommendation: A, plus the self-stop.

The rule becomes something the database holds, which is how this model already enforces "one active asset per content", "one proxy per type" and "one queued item per dedup key" (QueuedDedupKeyUnique, the closest precedent).
B would leave the rule in code that has // Stryker disable all.
C costs connections and still has the zombie window.
Without the self-stop, no option delivers the rule spec: See soft-deleted assets and purge them #47's purge relies on: no file side effects from two attempts on one config at once.

First caller: the claim. From the first deploy it serialises a config's Crawl, BurstDetection and scoped ProxyDeletion; #342's purge is the caller that depends on it. Tests:

Integration, WorkItemStoreTests/WorkScopeTests, real PostgreSQL:
- two concurrent claimers with two queued items of one config → exactly one Running;
- while C's item runs, a claimer takes D's item and an unscoped item;
- a second Running for C violates the index;
- the migration's clean-up leaves one running item per config.
Unit, WorkExecutorTests with Timeline: heartbeats fail continuously → the handler's token is cancelled at half a lease, not before; one heartbeat that succeeds resets the clock; a heartbeat call that never completes still cancels the handler at half a lease.

Behaviour change to confirm: a database outage longer than half a lease now cancels every running item, a long first crawl included. Lease recovery then requeues it and counts a stall, and the third such outage fails it as stalled (WorkMaxStalls). Nothing can be saved while the database is down anyway, so the work lost is only what was done since the outage began. The maintainer hasn't decided this yet; it was surfaced on 2026-10-28.

Hard to reverse: little. The index can be dropped in a later migration. The self-stop changes how the executor reacts to a database outage: a long outage now cancels running work rather than letting it finish blind. That is the intended behaviour. ADR 0008 gets a line covering both.

🤖 Generated with Claude Code

Decision

Decided by the maintainer on 2026-10-28, on the design note.

Rule: at most one Running item per StorageConfigId, across all Workers. Unscoped items are not serialised.
Option A. The claim passes over a busy config (NOT EXISTS), and a partial unique index ("StorageConfigId") WHERE "Status" = 'Running' AND "StorageConfigId" IS NOT NULL rejects a racing second claim. The migration only creates the index, with no clean-up of existing rows: the product has never been deployed.
Losing a race: the claim recognises the violation by index name, returns null, and the Worker waits for the next poll. No immediate retry.
Self-stop: accepted. An attempt cancels its handler half a lease (30 s by default) after its last heartbeat that succeeded, on a timer of its own. Each heartbeat call is limited to one heartbeat interval. The maintainer accepts the behaviour change: an outage longer than half a lease restarts running work (a long first crawl included), counts a stall, and the WorkMaxStallsth such stall fails the item.
A self-stopped attempt writes nothing, as a fenced attempt does. It is logged, and lease recovery requeues the item and counts a stall. It must never reach FailAsync: today, a handler cancelled while stopping is not set falls through to FailAsync, which would mark the item Failed for good once the database is back.
Records: a dated amendment to ADR 0008 covering the rule and the self-stop, made in the implementation PR. No new ADR.

Corrections to the note for the implementer

The deadline and the per-heartbeat timeout must be built from timeProvider (for example a CancellationTokenSource created with the TimeProvider, linked into the handler's token). running comes from CreateLinkedTokenSource, so running.CancelAfter(...) runs on the system timer and a Timeline test cannot drive it.
The concurrent-claimers integration test must force the overlap: hold one claim's UPDATE open in an explicit transaction, run the second claim so it blocks on the index, commit the first, and assert the second returns null. Without that, NOT EXISTS alone usually wins on timing and the test proves nothing about the index.
Recognise the violation with the store's existing private IsQueuedDedupViolation, made to take the index name, rather than adding a PostgresException overload to Core (it has no other caller).
The note's migration clean-up and its integration test are dropped (point 2).
Add a unit test (WorkExecutorTests, Timeline): heartbeats fail until the self-stop, then the database answers again → no Fail, nothing written, and the attempt is logged as self-stopped.

🤖 Generated with Claude Code

Design review

Architect review of the design note by a session other than its author, checked against EfWorkItemStore.cs and WorkExecutor.cs on master (31a5583). It was done on 2026-10-28, before the maintainer's decision, and settled there.

Holds:

The race is real. With NOT EXISTS plus FOR UPDATE SKIP LOCKED under READ COMMITTED, two claimers can each take a different queued item of one config. Something has to serialise the two claims or reject the second.
The zombie window is real. HeartbeatAsync logs a failed heartbeat and carries on, and the handler stops only on Lost.
The options are genuine, and changing the pattern was weighed (C). A follows from the constraints and the ix_workitems_dedupkey_queued precedent.
The self-stop margin holds for long steps, because SystemProcessLauncher kills the FFmpeg process tree on cancellation.

Findings, and how they were settled:

#	Finding	Settled
1	A self-stopped handler throws `OperationCanceledException` while `stopping` is not set, so it falls through to `FailAsync`. If the database is back by then, the item ends `Failed` with no retry. The note's "can't write its outcome" holds only while the database stays down.	Decision 5: it writes nothing, as a fenced attempt does, and lease recovery requeues the item. A test was added.
2	The migration clean-up could collide with `ix_workitems_dedupkey_queued` and requeue cancel-requested items.	Moot: decision 2 drops the clean-up, since the product was never deployed.
3	`running.CancelAfter(...)` on a linked CTS runs on the system timer, so a `Timeline` test cannot drive it.	Implementer note: build the deadline and the heartbeat timeout from `timeProvider`.
4	The concurrent-claimers test can pass on timing alone, because `NOT EXISTS` usually wins.	Implementer note: force the overlap through a transaction held open.
5	The violation check doesn't need a new Core overload.	Implementer note: reuse the store's private `IsQueuedDedupViolation`, made to take an index name.
6	The note had an open maintainer question under a non-template heading ("Behaviour change to confirm").	Decided by the maintainer: decision 4.
7	A race loser idles one poll, and an old-binary Worker would log `QueueFailed` during a rolling deploy.	Decision 3: it waits for the next poll. The rolling-deploy noise is moot (never deployed).

Design approved, as amended by the decision: A plus the self-stop is the only combination that keeps file side effects of two attempts on one config from overlapping, which is the guarantee #342's purge depends on.

🤖 Generated with Claude Code

feat(worker): At most one work item per storage config runs at a time (…

Navigation Menu

Repository navigation

feat(worker): At most one work item per storage config runs at a time #337

Problem

Desired behaviour

Acceptance

First caller

Design

Design note

Decision

Corrections to the note for the implementer

Design review

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

feat(worker): At most one work item per storage config runs at a time #337

Description

Problem

Desired behaviour

Acceptance

First caller

Design

Activity

bitbiter-dev commented on Oct 28, 2026

Design note

bitbiter-dev commented on Oct 28, 2026

Decision

Corrections to the note for the implementer

bitbiter-dev commented on Oct 28, 2026

Design review

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions