fix(capacity): make slot admission atomic across workers

[alex-furman]

Problem and change

Concurrent backend workers can each observe spare capacity and authorize dispatch past max_parallel_tasks. A repeated execution ID can also authorize another dispatch and overwrite the original metadata. This was reproduced on unmodified dev a72271d with six independent processes against Redis7.

SlotService now uses WATCH/MULTI/EXEC to serialize the capacity/membership decision and slot metadata writes. Duplicate IDs return False. Metadata type validation is inside the watched decision because Redis transactions provide isolation, not rollback on command errors. A lost EXEC response never authorizes dispatch, although an already-committed reservation may remain for reconciliation.

The existing backlog sentinel handoff is preserved: a competing admission can win the release/reacquire gap, and BacklogService returns the claimed row to its queue. Documentation and the test registry describe the boundary and how to run the real-Redis cases.

Validation and review

• Stock reproduction: six admissions at capacity1 and capacity3; repeated execution IDs also authorized all six. Always-on independent-client regressions fail on stock too.
• 53 passed: atomic admission, CapacityManager, per-slot TTL and renewal, including real-Redis multiprocess cases.
• 55 passed in the earlier focused/backlog/physical-meter run; unchanged broader paths reused after the final metadata-error correction.
• 207 passed, 2 skipped: canary invariants. The two PostgreSQL-only cases were then run against disposable PostgreSQL17: one passed; test_pg_h01_fires_on_a_blind_roster failed because its fake Docker containers provide no readable pull environment. That exact failure also reproduces with the unchanged official slot_service.py; it is a pre-existing fixture failure, not counted as passing.
• Independent adversarial review found the malformed-metadata partial-transaction case; it was reproduced, fixed and independently retested. Final reviewer run:12 passed, including real Redis, with no remaining blocking findings.
• Test module-isolation lint and diff whitespace checks pass.

Reproduce the focused suite using the normal test dependencies plus a disposable Redis:

TEST_SLOT_REDIS_URL=redis://127.0.0.1:6379/0 python -m pytest tests/unit/test_atomic_slot_admission.py tests/unit/test_slot_per_slot_ttl.py tests/unit/test_2433_slot_renew.py tests/unit/test_capacity_manager.py -q

Tested with Python3.13, redis-py5.2.1, fakeredis2.39.0 and Redis7. Without TEST_SLOT_REDIS_URL, three process-contention cases skip; concurrent independent-client tests still run. No model calls or live agents are required.

Boundary

This fixes atomic admission, not complete physical-execution exclusion. It does not change stale-slot cleanup, remote termination, Redis-loss recovery or uncertain-result handling. Those lifecycle concerns remain separate; this PR must not be represented as resolving them.

Fixes #3305

Journey Impact: extends: J04

Mutation: test_independent_clients_never_over_admit, test_concurrent_processes_never_over_admit and test_duplicate_cannot_authorize_dispatch_or_replace_metadata fail against stock. The wrong-type metadata regression also caught a partial transaction in the initial candidate.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[vybe]

merge-train (mechanical): merged origin/dev into this branch (15a7660) to resolve the tests/registry.json conflict — kept both sides, this PR's test_atomic_slot_admission.py entry first. No code changed. Validation: READY.

One non-blocking note for a follow-up: acquire_slot now returns False for an already-held id, which CapacityManager reads as capacity-full (queue_persistent would re-queue a running row; reject would fail it). No current caller acquires the same id twice, so it's latent — a distinct "already held" signal would close it.

[vybe]

merge-train: ejected after the batch gate — rides the next train once fixed.

On the train (and on this branch alone, after the dev merge), regression diff reports three new failures from the edge-case suites #3327 added to dev yesterday:

• test_ec_capacity_slots_edges::test_r11_reacquire_same_id_under_cap_does_not_double_count — contract conflict: dev pins that re-acquiring an already-held id under the cap is idempotent (True, counted once); this PR returns False. That's the validation note above (CapacityManager reads False as capacity-full).
• test_ec_capacity_slots_properties::test_p1_slot_accounting_matches_set_oracle — same contract (('acquire', 'e0', {'e0'}, 2)).
• test_ec_capacity_slots_edges::test_r14_concurrent_acquire_across_workers_never_overshoots_cap — XPASS(strict): this PR fixes the bug it records (bug: acquire_slot checks capacity and then adds the slot in two calls, so two workers can exceed an agent's cap #3318). Drop the xfail marker and add Fixes #3318 (or close it as a duplicate of bug: concurrent slot admission can exceed configured capacity #3305).

Which held-id semantics is right is the author's call: either keep acquire_slot idempotent for a held id (and still atomic), or return a distinct "already held" result and update r11/p1 to the new contract on purpose.

[vybe]

Pushed two commits to clear the merge-train ejection:

• fbd43e7c3 merges dev (only tests/registry.json conflicted; both sides kept).
• 99585ef74 makes re-acquiring an already-held id idempotent under the cap (True, counted once, slot not renewed, metadata not overwritten). At the cap it still returns False. The held path goes through the same WATCH/MULTI/EXEC step as a new admission, so the cross-worker atomicity this PR adds is unchanged.

@alex-furman this reverses a choice you made on purpose (duplicate id returns False because True authorizes a dispatch). I took the option that keeps the contract dev already pins in r11 and p1, because CapacityManager reads False as capacity-full and would re-queue or fail a running row. If you would rather have a distinct already-held result, say so and r11/p1 can move to that contract deliberately.

Also: r14 (#3318) no longer needs its strict xfail. Its hook patched redis.zcard, which the new code never calls, so it is moved to the pipeline read and asserts that it fired.

Local results: the three ejected files 78 passed; test_atomic_slot_admission.py 12 passed against a real Redis; all 68 unit files touching slot_service / capacity_manager 1982 passed. Not run: the randomized seed matrix and integration suite, which CI covers.

[vybe]

merge-train: one more commit pushed, a55241a32, after an independent validation of this branch. It corrects my own earlier commit and closes one hang.

1. A held id now takes the same write as a new admission (corrects 99585ef74). My previous commit returned True for a held id without writing. dev re-ZADDs (score re-anchored to now) and rewrites the metadata and its TTL; without that, a re-dispatch runs on the lease of the original admission and the slot can be reaped mid-run. The separate held branch is gone: acquire_slot is now dev's logic inside WATCH/MULTI/EXEC. Under the cap a held id is refreshed and counted once; at the cap it is refused like any other id, as on dev.

2. The retry loop is bounded. redis-py reports a connection or timeout error on a watched pipeline as WatchError, which the while True swallowed. With WATCH succeeding and EXEC failing every time, the loop span synchronously on the event loop and never returned (26,279 EXEC attempts in 5s in a probe). It now gives up after MAX_ADMISSION_ATTEMPTS = 32 and raises SlotAdmissionError (a redis.RedisError). Exhaustion never returns True or False.

3. The connection-loss test and doc sentence now say what happens. The old test replaced Pipeline.execute wholesale and asserted a ConnectionError production cannot raise from there. The fault is now injected at the connection's send, through the real transaction path. execution.md is reworded accordingly.

@alex-furman two consequences worth your eye, neither changed here:

• With the last free seat, if EXEC commits and its response is lost, the retry sees the id held at the cap and returns False: the caller is told capacity-full while its own id holds the slot until release or stale reaping.
• In backlog_service.drain_next, a raise at the real-id re-acquire leaves the claimed row running, since the requeue runs only on a False return. dev behaves the same on any Redis error at that point.

Results: the three slot files 82 passed; 76 unit files touching slot_service / capacity_manager / backlog_service 2155 passed. Mutations: an unbounded loop fails both bound tests; a held id that returns without writing fails four. Not run on this commit: the real-Redis multi-process contention cases (no Redis available locally; they passed on the previous commit and are skipped in CI). Under real contention, losing 32 rounds in a row would raise rather than return False.

[vybe]

merge-train: batch validated on train PR #3409. Admission is dev's logic made atomic under WATCH/MULTI/EXEC, with a bounded retry; mutation testing confirmed the atomicity. Open notes for the author are in the two merge-train comments above.