Repository navigation
fix(subagents): fence durable batch lease lifecycles - #6764
Conversation
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed against head 102cbb4. The core fencing work is solid: fresh per-pass claim generations fit the String(128) lease_owner column (max 113 chars), worker acks are fenced at the SQL UPDATE, expiry recovery rechecks owner+expiry, and parent-before-item lock order is consistent across claim/control/ack paths, so no new deadlock cycle. The new regressions model real SQLite interleavings rather than mocks, which is the right way to test this. Three findings below: one completeness gap (the claim-path writes are still unfenced on SQLite), one API-shape suggestion (the lease_owner fallback that keeps a process-identity fence reachable), and one semantics nit (cancel_requested now also means lease lost).
| SubagentBatchItemRow.batch_id == batch.id, | ||
| SubagentBatchItemRow.status == "queued", | ||
| SubagentBatchItemRow.cancel_requested_at.is_(None), | ||
| SubagentBatchItemRow.id.not_in(exclude_item_ids), |
There was a problem hiding this comment.
The worker-ack paths are now fenced, but the claim path itself still writes unfenced on SQLite. After this SELECT (FOR UPDATE is a no-op there, and the SQLite driver runs these reads in autocommit), the for item in runnable: loop below mutates ORM objects and lets the final session.commit() flush an unconditional UPDATE ... WHERE id = ?. If a peer's claim pass commits between this worker's read and its flush, the stale flush overwrites the peer's newer claim (owner, attempt += 1, expiry), both workers execute the item, and the peer's fenced finalize later loses — exactly the interleaving class the new tests prove reachable, since they pause a task mid-pass while a peer reclaims. Since _write_owned_item already exists, the claim loop could do the same fenced UPDATE ... WHERE status = 'queued' + rowcount check to confirm it won the claim. This is pre-existing on main, but it is the one remaining unfenced stale write in this file, and this PR is the natural place to close it.
| return batch | ||
|
|
||
| async def _execute_item(self, item: dict[str, Any]) -> None: | ||
| async def _execute_item(self, item: dict[str, Any], *, lease_owner: str | None = None) -> None: |
There was a problem hiding this comment.
lease_owner: str | None = None keeps a silent fallback to the process-identity self._lease_owner — precisely the non-fence this PR removes (run_once's new comment: "A process identity is not a fence for a later claim by that process"). Today the only production caller passes the fresh claim_owner; the default exists only so backend/tests/blocking_io/test_tool_assembly_offloop.py can call _execute_item(item) directly. Making the parameter required (and updating that test to pass a token) would turn a future caller's silent fencing regression into a visible error instead of reintroducing the bug this fixes.
| return {"valid": not cancel_requested, "cancel_requested": cancel_requested} | ||
| valid = await self._write_owned_item(session, item_id, lease_owner, {"lease_expires_at": now + timedelta(seconds=lease_seconds), "updated_at": now}) | ||
| await session.commit() | ||
| return {"valid": valid, "cancel_requested": not valid} |
There was a problem hiding this comment.
Nit: this quietly changes cancel_requested semantics. It used to mean only "a batch/user cancellation was requested"; now lease loss (fenced UPDATE rowcount 0) — and the early return for a missing item at line 401 — are also reported as cancel_requested=True. Current consumers only read valid, so nothing breaks today, but a future caller using this field to distinguish "user cancelled" from "lease lost" (logging, metrics, retry policy) will misreport. Keeping cancel_requested tied to cancel_requested_at is not None / batch status, or adding a separate key, would preserve the old contract.
willem-bd
left a comment
There was a problem hiding this comment.
Follow-up against head b482f8a (one new commit since the review at 102cbb4). All three findings from the previous round are fixed, and the fixes are pinned by real-interleaving regressions I verified both ways (they fail against 102cbb4's code and pass at head; full touched suite green: 26 in tests/test_batch_lease_lifecycle.py, 28 across the three updated test files).
- Unfenced ORM flush in the claim loop (
sql.py) — FIXED. Thefor item in runnable:loop now issues a conditionalUPDATE ... WHERE status = 'queued' AND cancel_requested_at IS NULL AND batch_id IN (<parent runnable subquery>)and only counts a win onrowcount == 1, refreshing the row it reports; the pending-promotion writes got the same fencing. The residualbatch.status = "running"write is now gated onbatch_claimed, i.e. on a successful fenced write already inside this transaction, so a concurrent pause/cancel can no longer be resurrected. Newtest_stale_claim_pass_preserves_peer_claim_or_control(phase x peer_action) pauses a stale pass right after its runnable read and proves a peer's claim, cancel, or pause is never overwritten — it fails on all 6 cases against the prior commit. lease_ownerfallback to process identity (batch_service.py) — FIXED._execute_itemnow requires the keyword (lease_owner: str), every caller and test passes the captured claim token explicitly, andtest_execution_requires_an_explicit_claim_ownerasserts the TypeError a future silent caller would hit.cancel_requestedover-loading inrenew_item_lease(sql.py) — FIXED. The lost-lease and missing/wrong-owner paths now computecancel_requestedfrom fresh item/batch state, so it again means only an explicit item or batch cancellation; pinned bytest_renewal_reports_only_explicit_cancellation(missing/wrong_owner/item_cancelled/batch_cancelled/owned).
The remaining unfenced-in-shape parent write is safe on both backends: PostgreSQL holds the parent row lock from the claim pass's FOR UPDATE batch select, and on SQLite the transaction already owns the writer lock once batch_claimed is true. No new findings.
Why
Durable batch work can lose its lease before it even launches. Tool assembly runs in the real assembly pool and can wait on slow MCP discovery, but renewal previously began only after dispatch. A later scheduler pass performs expiry recovery before checking its local execution registry: with
max_attempts=1, it can fail an item whose original initialization is still running.SQLite also ignores
SELECT FOR UPDATE. If a worker reads its owned item, another worker reclaims it, and the first worker resumes, the ORM commit updates by primary key and can overwrite the new attempt's lease, state or result. The inverse race exists too: recovery can overwrite a heartbeat that renewed after its expiry read.What changed
Public DTOs, schema and retry policy stay the same. This addresses lease supervision and stale writes; the separate post-dispatch exception/child-teardown work in #5253 remains outside this patch.
Surface area
Bug fix verification
backend/tests/test_batch_lease_lifecycle.pyuses real SQLite repositories and the production assembly pool; model execution is replaced with a deterministic test executor.Eight regressions fail on pristine main
65fa321ec18a8e97b5ee60b023afbd95b0eba83band pass here: four stale worker acknowledgements after a real peer reclaim; local recovery during initialization; periodic initialization renewal visible to another worker; fresh claim generations; and an expiry read racing renewal. The stale acknowledgement timestamp deliberately remains inside the old lease so an expiry check cannot hide a missing ownership fence.Two further regressions cover peer cancellation with valid
lease_seconds=10/poll_interval_seconds=60and draining an in-flight heartbeat across repeated caller cancellation. The long-poll regression was observed failing before restoring the lease/3 polling bound.Validation
65fa321(pytest -m "not live" --ignore=tests/blocking_io --splits 4 --group 1..4 --splitting-algorithm least_duration --durations-path=.test_durations tests/ -q), using the branch's own locked Python environment: 29,755 passed, 489 skipped, all four exit codes zero.ruff check .,ruff format --check ., andgit diff --check: passed.Main then advanced to
9c071c7; the three new changes concern cron editing, Feishu notifications and upload snapshots, with no file overlap or batch-lease dependency changes.PostgreSQL integration was not run locally; tests requiring its opt-in URI remain skipped. No paid model calls were used.
AI assistance
Tool(s) used: OpenAI Codex.
How you used it: Codex investigated the production flow, reproduced the races, wrote the implementation and regression tests, and ran validation. A separate Codex reviewer identified the cancellation-latency regression, which was reproduced and fixed; its final read-only review found no remaining introduced Important/Critical findings. Human review of this new patch is pending, so this PR is a draft.