TASK-1175: generation-fenced durable queue leases - #1
Conversation
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
src/store/mod.rs:422(Store::enqueue_v2) —let warning = (duplicate_count > 0).then(|| format!("subject {qualified_id} already has {duplicate_count} active generation(s)"));detects an existing live generation but then continues to allocate and insert another queue entry. This violates TASK-1175's explicit requirement to prevent same-task duplicates: a second delivery with a different idempotency key can create another pending generation for the same qualified task, and because each row has a differentsubject_generation, both can subsequently lease into independent workflows. REQUIRED CHANGE: serialize v2 enqueue byqualified_id(for example with a transaction-scoped advisory lock), check for any live row with thatsubject_qualified_idbefore incrementing the generation allocator, and return a typed collision/rejection response or error instead of inserting; retain idempotent replay only when the same idempotency key identifies the same existing entry. Also add a database-level invariant if the contract permits one (such as a partial unique index over live v2subject_qualified_id) so concurrent callers cannot bypass the application check. Verification: add a real-Postgres integration test that concurrently callsenqueue_v2twice for the same qualified task using two different idempotency keys, asserts exactly one enqueue succeeds and the other reports the subject collision, then callslease_v2(max = 5)and asserts only one fenced entry/workflow is returned for that task. Runcargo test --all-targets; the new concurrency test and the existing 13 tests must pass.
The CAS recovery/renew/completion fencing, explicit expired-lease recovery path, exact repository/head reservation, additive migration, and real-Postgres CI coverage are otherwise thoughtfully structured and align well with the durable restart portion of the task.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
src/store/mod.rs—Store::enqueue_v2:let warning = (duplicate_count > 0).then(|| format!("subject {qualified_id} already has {duplicate_count} active generation(s)"));still permits a second live generation of the same qualified subject to be inserted. The generation allocator serializes allocation, but neither that serialization nor the(subject_qualified_id, subject_generation)unique index prevents two requests with different idempotency keys from creating generations 1 and 2 concurrently/sequentially while generation 1 remains pending/leased/held. That violates the task invariant that one task generation may own only one queue item/workflow/workspace/environment/branch/PR and allows duplicate live work whenever no repository reservation is supplied or the reservations differ. REQUIRED CHANGE: InStore::enqueue_v2, after idempotent replay handling and before allocating a new generation, serialize onqualified_id(for example with a transaction-scoped advisory lock), query for an existing v2 row for that qualified subject inpending,leased, orheld, and return a typed subject-collision error instead of generating/inserting another row. Preserve replay only when the same idempotency key resolves to the same existing entry. Also add a database-level live-subject invariant if migration compatibility permits (for example, a partial unique index onsubject_qualified_idfor live v2 states). Verification: add a real-Postgres concurrency test that launches twoenqueue_v2calls for the same qualified subject with different idempotency keys (and non-colliding/no repository reservations), asserts exactly one succeeds and the other reports the typed subject collision, asserts stats contain exactly one pending row, and assertslease_v2returns only one fenced entry; then runcargo test --all-targets.
The explicit recovery/CAS fencing model, stable workflow identity, repository/head reservation, five-slot lease cap, and real-Postgres CI coverage are otherwise strong and directly address the durable restart requirements.
|
Addressed the requested duplicate-task invariant in a5ba9f7.
|
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
src/store/mod.rs:441(Store::enqueue_v2) —return Ok(QueueEnqueueV2Response { enqueued: false, ... warning: Some(...) })reports a distinct-idempotency-key subject collision using the successful replay response shape. That is wire-incompatible with the pinned protocol contract, whereQueueEnqueueV2Response.enqueued == falseis specifically documented as an idempotent replay andwarningis only a non-fatal advisory. A caller therefore cannot distinguish a valid replay from a rejected competing generation except by parsing free-form text, and may incorrectly treat the existing entry as its own replay. REQUIRED CHANGE: introduce/use a typed queue subject-collision outcome (preferably a dedicatedanimus-queue-protocolJSON-RPC error code with structured data containingqualified_id, existingentry_id, and generation), makeStore::enqueue_v2return a typed collision error for this branch, and map it inhandle_enqueue_v2; reserveQueueEnqueueV2Response { enqueued: false }for a replay of the same idempotency key and entry. Verification: updatev2_concurrent_duplicate_subject_enqueue_creates_one_live_generationto assert exactly one call returnsenqueued: true, the other returns the typed subject-collision error/code (not anOkresponse or parsed warning), then assert stats contain one pending row andlease_v2yields exactly one fenced entry; runcargo test --all-targets.
The qualified-subject advisory lock, partial live-subject unique index, preservation of same-key replay semantics, and real-Postgres concurrent coverage are otherwise strong fixes for the duplicate-generation race. CI is green on a5ba9f75784b75cf9afd7038b8dbd32337b1d9fd.
Implements the Postgres queue slice of REQUIREMENT-076 and TASK-1175 on top of animus-protocol v0.7.0-rc.14.
Key behavior:
Verification:
Dependency: launchapp-dev/animus-protocol v0.7.0-rc.14 at 63c60d573090a98c1a36c8f469e86e0dbcd6c712.