fix(subagents): tear down dispatched child before batch retry

[Zsanz3]

Fixes #5240

Problem

After execute_async() starts a child, a bookkeeping failure (mark_item_running / lease renew) could finalize and requeue the durable item without cancelling that child. The service then dropped ownership while the previous execution kept running, so a retry could overlap it.

Fix

On supervisor/bookkeeping failure after a child was dispatched: retain ownership, request cancellation, await teardown, then release for retry. Lease fencing and user-cancel paths are unchanged beyond this local supervision order.

Tests

Added a bookkeeping-failure overlap regression in test_subagent_batch_service.py (schema-valid batch config bounds).

Scope

Only batch_service.py, its AGENTS note, and the regression test. Does not change #5116.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ shoyann
✅ Zsanz3
❌ cursoragent
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[willem-bd]

Reviewed at c9851204b (refs/auto-review/5253-c9851204). The supervision order is the right shape — retain ownership, request cancellation, wait, then finalize — and the new regression test passes (5 passed). Two real gaps remain on this head: the teardown helper can only ever return True, so the refusal branch is dead and the wait is an unbounded, lease-renewing loop that can wedge an item forever (reproduced locally), and on current main execution_done_event does not exist while terminal status is set before _aexecute's finally teardown, so "teardown confirmed" can still release the item during the previous child's cleanup. Two smaller items cover the lost-lease path and the unprotected renewal inside the wait loop.

[Zsanz3]

Thanks for the review at c9851204.

Kept the retain → cancel → wait → finalize order and closed the gaps:

• _await_dispatched_execution_teardown() can now return False. The wait is bounded to a few lease periods; unconfirmed teardown no longer takes the ordinary immediate-retry path.
• Teardown is confirmed only by SubagentResult.execution_teardown_event (set after run_with_timeout() returns) or a vanished registry entry — not by CANCELLED/FAILED/completed_at.
• valid: False / cancel_requested stops the renew/poll loop and returns False instead of finalizing without a held lease.
• Renewal exceptions are caught, logged with item_id, and treated as the lease-expire path.

Regressions cover lease-lost, unconfirmed teardown, and renewal failure. HEAD is 909a53d547f81f24640d7cf3741ceeffb6ef80e7.

[willem-bd]

Re-reviewed at 0ca802a1 (refs/local-review/5253-0ca802a1). All four findings from the c9851204 round are addressed, and I checked each against the head code rather than the diff: the supervised wait is bounded to three lease periods and _await_dispatched_execution_teardown() can now return False; teardown is confirmed only by SubagentResult.execution_teardown_event (or a vanished registry entry), never by terminal status/completed_at; valid: False and cancel_requested stop the loop and refuse the ordinary retry; and a renewal exception is caught, logged with item_id, and takes the lease-expiry path instead of escaping. I ran both suites at this head in a pinned worktree (150 passed) and confirmed the four new regressions fail against c9851204's batch_service.py/executor.py, so they have real teeth. Nothing blocking; one suggestion below on pinning the ordering the fix depends on.

[WillemJiang]

@Zsanz3 please click the CLA Assiatant button to sign the CLA before we merge you code.

[WillemJiang]

@Zsanz3 please click the CLA assistant button to sign the CLA and resolve the conflict with the main branch before we start the merge process.

[willem-bd]

Verified at the rewritten head d209757c (refs/auto-review/5253-d209757c; the branch was rebased, so I re-checked at head rather than relying on the 0ca802a1/20e9b0d rounds). All five prior findings remain fixed there: the supervised wait is bounded to three lease periods and _await_dispatched_execution_teardown() returns a real bool (the refusal branch is reachable); teardown is confirmed only by execution_teardown_event or a vanished registry entry, never terminal status/completed_at; valid: False/cancel_requested stops the wait and refuses the ordinary retry; a renewal exception is caught and logged with item_id, taking the lease-expiry path; and the executor regression now pins order == ["release", "teardown_event"] with a production-shaped finally in the patched _aexecute stand-in — that closes the round-3 test-strength thread. I also reviewed the delta since 20e9b0d — the rebase onto current main (pre-launch lease revalidation and off-loop tool assembly in _execute_item), the AGENTS.md restore, and the test-only adaptation making the first renewal the pre-launch revalidation (both regressions now assert lease_renewals >= 2, correctly distinguishing the wait-loop renew from the pre-launch one) — and found no regressions: run_coroutine_threadsafe guarantees the future (and forget_future) cannot resolve before run_with_timeout's finally, so the sticky event cannot fire early. Ran both suites at the pinned head: test_subagent_batch_service.py 10 passed, executor teardown tests 2 passed. Nothing outstanding from my side.

[Zsanz3]

Follow-up on 078d0da6: the teardown regression now pins the event after the production finally path (sandbox/holder release), instead of only while a patched _aexecute body is still sleeping. Happy to adjust further if anything still looks off.

[WillemJiang]

@Zsanz3 Please make sure the git commit author email is added in your GitHub profile email settings.

@Zsanz3 please click the CLA Assiatant button to sign the CLA before we merge you code.

[shoyann]

I found a cancellation-specific gap on d42a748b: request_cancel_background_task() calls cancel() on the cross-thread submission Future, which invokes forget_future() before the isolated-loop task has finished draining. That callback currently sets execution_teardown_event, so the teardown check can pass while resource cleanup is still blocked.

I opened a focused follow-up against your branch: Zsanz3#4 (one commit, implementation + two regressions + the lifecycle note).

The new tests use the real submission/cancellation path and cover both cancellation during blocked cleanup and cancellation before loop dispatch. They fail on this head and pass with the follow-up; the executor/batch/capacity/runtime selection passes all 185 tests. The PR also reports the full Windows baseline comparison, including outstanding failures — it does not claim an all-green full suite.

The change leaves teardown confirmation in the execution wrapper's finally and keeps the Future callback responsible only for forgetting the submission handle. It does not change the lease-expiry recovery policy or ordinary FAILED-result retries.

[Zsanz3]

Thanks @shoyann — I reviewed the follow-up and merged it into this branch (f86b7662, via Zsanz3#4).

Teardown confirmation now stays in the execution wrapper's finally after cleanup drains; the Future callback only forgets the submission handle. The two new cancellation regressions are on this head.

Happy to adjust further if anything still looks off.

[WillemJiang]

@copilot resolve the merge conflicts in this pull request

[willem-bd]

Read-only static review of pinned current revision; verified introduced findings below. No PR code or tests executed.

[willem-bd]

[P2] Exclude retained teardown items from lease-expiry claims

After a bookkeeping failure, one transient renewal exception stops all subsequent renewals but leaves this item in _executions until teardown or the three-lease-period deadline. With a spare service slot, the same worker's run_once keeps calling claim_items: when this lease expires, the repository requeues and commits a new claim with attempt += 1, then run_once discards that claim because the old item task is still mapped. For example, with legal 10-second leases and max_attempts=3, cleanup lasting 25 seconds allows claims at roughly 10 and 20 seconds to consume attempts 2 and 3 without dispatching either retry; the next expiry terminalizes the item as failed after only one actual execution. Prevent atomic recovery/claim from consuming attempts for locally retained teardown items, or return to the lease-expiry path immediately once renewal fails instead of retaining a nonrenewing item task. Cover this with the real repository and polling during blocked teardown.