Repository navigation
Kill switch trip returns success before a delegated worker thread's protected side effect commits #834
Description
I wrote a test that shows POST /kill-switch/trip returning 200 {"status": "tripped"} while an in-flight tool call (delegated to a worker thread via run_in_executor) commits its protected side effect 0.5 ms after the response is returned. The coroutine is cancelled by the drain, but the worker thread is not, so the effect lands after the trip has already returned success.
Reproduction
Commit: 053b5e6
Python: 3.13.5, pytest 9.1.1
Test command: PYTHONPATH=src python -m pytest tests/unit/test_inflight_trip_worker_thread_boundary.py -v -s
The test blocks the worker for drain_timeout + 2s (32 s with the default _session_close_drain_s=30). The trip returns at 30.020 s. The worker completes 0.509 ms later. The effect marker file exists.
Diagnostic output:
drain_timeout_s: 30.000
worker_block_for_s: 32.000
trip_status: 200
trip_elapsed_s: 30.020
trip_returned_at: 32387.864753
worker_completed: True
worker_completed_at: 32387.865262
effect_marker_exists: True
XFAIL
Question
The trip handler docstring says "calls already running finish or are cancelled at the close drain deadline." That is truthful for coroutine-based tools. It is not truthful for tools delegated to a worker thread, because Python cannot cancel the worker. Is this behavior intended and should it be documented in the docstring and LIMITATIONS.md, or is it a defect?
I have a branch with the two tests (test_inflight_trip_worker_thread.py for the safe case and test_inflight_trip_worker_thread_boundary.py for the drain-timeout case) ready to open as a PR once I am vouched. Happy to adjust the framing or add a docstring change if that is the preferred fix.
Activity
dinakarjs commentedon Oct 30, 2026
Follow-up on the existing implementation rather than a competing patch: main 1fbc1b2 already includes #823's LIMITATIONS.md section and tests/unit/test_worker_drain_outcome.py. I reran the two cases in a hash-locked Python 3.12 environment: both pass. They deliberately hold a worker until after the trip response, show the later effect completion, and confirm subsequent admission is refused. This documents/tests the admission-versus-worker-completion boundary; it does not forcibly stop the worker or establish protected-effect quiescence. Could the maintainers disposition whether this existing contract addresses the question here or whether a separately scoped stronger executor-stop guarantee is desired? I have not closed the issue or inferred that stronger guarantee. Prepared with OpenAI Codex.
carloshvp commentedon Oct 30, 2026
Current main
1fbc1b2bc04ca8135fc06252b58f98dfe347f017already contains the documentation and paired regression tests from #823, following #820. The trip handler docstring now says cancellation is requested at the deadline and explicitly states that cancelling an awaiter cannot terminate a delegated worker or roll back a remote effect. LIMITATIONS.md describes that boundary.I independently reran
tests/unit/test_worker_drain_outcome.pyat that main revision: both cases pass. They observe a worker completing before the trip and a deliberately released worker completing after the response, while later admission is refused. This is a software marker-file test, not proof of remote-effect quiescence.The current contract is blocked admission plus bounded cooperative draining. A successful trip does not prove that every admitted effect stopped; an unobserved outcome remains unknown and must not trigger an automatic retry. That is the expected boundary for this implementation. The documentation/test request here is already addressed, so I am closing this duplicate follow-up. A stronger executor/commit-point enforcement guarantee needs its own agreed design and acceptance cases; #736 tracks the related runtime boundary. No vouch or new execution guarantee is implied.
Reviewed with OpenAI Codex.
a1k7 commentedon Oct 30, 2026
Thanks for the redirect — verified locally that 1fbc1b2 adds test_worker_drain_outcome.py covering the same boundary. Closing note understood. I'll follow #736 for the stronger guarantee.