Skip to content

fix: retain subprocess ownership after waiter cancellation - #3308

Merged
vybe merged 4 commits into
Abilityai:devfrom
alex-furman:codex/execution-ownership
Oct 8, 2026
Merged

vybe merged 4 commits into
Abilityai:devfrom
alex-furman:codex/execution-ownership

Conversation

@alex-furman

Copy link
Copy Markdown
Contributor

Description

Cancelling a chat or headless execution waiter can remove a still-running child from ProcessRegistry. If both termination waits time out, the registry also reports success without observing exit. Both defects reproduce on unmodified dev a72271d.

Retain registered live children, their log subscriptions and sweep allowlist entries until exit is observed. After cancellation or outer timeout, executor workers perform eventual cleanup; ordinary success still finalizes the result before stream_end. Chat readers are shielded because their child already exists, while queued headless workers remain cancellable before spawning. Unconfirmed termination now returns an explicit failure and retains the handle.

Architecture documentation and the test registry describe this contract. This does not establish distributed capacity enforcement, pre-Popen ownership, reused-ID safety, descendant-process termination, or server-restart recovery.

Fixes #3307

Journey Impact: extends: J04

Validation and review

  • Stock negative controls: three expected failures for cancelled chat/headless ownership and false termination success.
  • Registry-only mutation: three expected failures demonstrate that executor worker-completion cleanup is required.
  • Outer-timeout mutation: three expected failures caught and verified the timeout counterpart of cancellation cleanup.
  • Final targeted run: 53 passed across ownership, existing lifecycle review guards and headless timeout behavior.
  • Earlier broader run: 102 passed across nine ownership/registry/termination/sweep/executor modules. One unrelated reused-ID test and guard were then removed, leaving 101 retained results from that run; subsequent timeout changes are covered by the final 53-test run. This is reused evidence, not a newly executed combined suite.
  • Independent adversarial review of the final candidate: 13 ownership regressions passed; no remaining blocker for this bounded fix.
  • Whitespace checks pass. Tests use disposable Python children, no model calls, credentials, network or live agent. Cgroup sweeping is stubbed so tests cannot signal unrelated processes.

Reproduce the final focused suite with normal backend test dependencies and the agent source on PYTHONPATH:

PYTHONPATH=src/backend:docker/base-image:. python -m pytest tests/unit/test_runtime_process_ownership.py tests/unit/test_2433_review_fixes.py tests/unit/test_headless_executor_970_timeout.py -q

Tested with Python 3.13 in an isolated Linux container. Real container restart, production dispatch and whole-stack acceptance are not claimed.

Mutation: test_abandoned_waiter_keeps_live_subprocess_owned and test_terminate_does_not_confirm_a_process_that_survived_both_waits fail against stock; the former also fails with stock executors over the corrected registry and without timeout abandonment cleanup.

Checklist

  • Focused bug fix, targeting dev and linked to an issue
  • Tested locally with negative controls and independent review
  • Regression tests execute changed behavior and reference the issue
  • Documentation and test registry updated
  • No private operational data or credentials included
  • All existing tests pass — full repository suite not run

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

⚠️ 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

vybe commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

merge-train: ejected from this batch — rides the next train once fixed. regression diff reports one new failure from this PR's own new test: test_runtime_process_ownership::test_headless_worker_finish_racing_waiter_cancellation_still_cleans_up (run 37567485053). It also conflicts with dev now and needs a merge.

@vybe vybe added the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 7, 2026
trinity-ability and others added 2 commits October 8, 2026 13:57
tests/registry.json rebuilt from the merge stages: dev's entries plus this
branch's one new entry (unit/test_runtime_process_ownership.py).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts the result (Abilityai#3307)

test_headless_worker_finish_racing_waiter_cancellation_still_cleans_up
failed in CI (run 37567485053, Python 3.13.15) with
"500: Task execution error: '<' not supported between instances of
'NoneType' and 'int'" instead of CancelledError.

The test scheduled task.cancel() from the worker thread and let the worker
return immediately, assuming a cancel queued first is always processed before
the worker's result. Since CPython 3.13.12, asyncio's _chain_future copies the
result synchronously when the done-callback is attached on the loop thread to
an already-finished worker future, so run_in_executor hands back a completed
future, the caller never yields, the queued cancel never lands, and the stub
context goes through the real _finalize_headless_result (return_code None).
Product cleanup is correct in that ordering; the test's timing assumption was
not.

The worker now waits until the cancel has been delivered, and the loop is held
until the worker has fully returned, which pins the ordering the test names
(worker never observes the abandoned flag, caller cancelled before its result)
on every interpreter. No sleeps, no product change. Removing the caller's
finally: unregister still fails the test 10/10.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vybe

vybe commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Pushed two commits to clear the merge-train ejection:

  • 14590b581 merges dev (only tests/registry.json conflicted; both sides kept).
  • 3869e2ebb fixes the failing test. It was a test timing assumption, not a product bug; no product code changed.

Root cause. CI failed with 500: Task execution error: '<' not supported between instances of 'NoneType' and 'int' instead of CancelledError: the waiter was never cancelled, received the worker's result, and ran the real finalizer on the stub context. The test queued task.cancel() from the worker thread and returned at once, assuming a cancel queued first always beats the result. Since CPython 3.13.12, asyncio's _chain_future copies the result synchronously when the worker has already finished, so run_in_executor hands back a completed future and the queued cancel lands on a finished task. In that ordering the caller's finally: registry.unregister(...) still runs, so cleanup is correct.

Reproduction, unmodified test, 40 runs: Python 3.13.5: 40 pass. Python 3.13.16: 25 pass, 15 fail with the exact CI error. (3.13.16 stands in for CI's 3.13.15.)

Fix. The worker now waits until the cancel is delivered, and the loop callback holds the loop until the worker has returned, on a dedicated single-thread pool. No sleeps, skips or xfails; the cleanup assertions are unchanged, plus a control assert that the worker returned before the waiter's cleanup. Mutation check: removing the product's finally: unregister fails the fixed test 10/10.

Results. Fixed test 50/50 on both 3.13.16 and 3.13.5; whole file green on 15 random seeds; 56 unit files touching claude_code / headless_executor / process_registry 1281 passed.

@vybe vybe added ui PR touches the frontend UI — triggers Playwright e2e tests and removed status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) labels Oct 8, 2026
…ebuilt, both entries kept

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train PR #3409 (lane C: /validate-pr, /review, /cso --diff; frontend-e2e and prod-image-smoke green on the train). No critical finding. Uncovered runtimes and the failed-write leak are tracked in #3408.

@vybe
vybe merged commit 836c0cb into Abilityai:dev Oct 8, 2026
22 checks passed
vybe pushed a commit that referenced this pull request Oct 8, 2026
…son rebuilt, all entries kept

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants