Repository navigation
Keep NAT's Guardian calls off the event loop - #484
bar-capsule wants to merge 1 commit into
Conversation
Round trips run in worker threads, and lifecycle hooks go out in order from one sender thread, so a slow Guardian does not stall the workflow. Signed-off-by: Bar Kaduri <bar@capsule.security>
drk-ram
left a comment
There was a problem hiding this comment.
Reviewed commit 02888ed962ece3968b5702f51c4cdf01e087b98a against its stated stacked base.
P2: drain the queued lifecycle work in the existing emission fixtures.
The off-loop transport and ordered sender are a good direction, but _queue_lifecycle_hook now returns before delivery. tests/integrations/nat/test_emission.py:104-175 still sleeps for 0.3 seconds before inspecting the four lifecycle records, and its directly constructed middleware instances are never closed.
At this commit, the focused command below produced 11 passed, 2 failed: test_lifecycle_observer_emits_session_hooks and test_sequential_session_invariants. The same four emission tests passed at the parent commit 81841cc44e1d72c879793bc84fda7e70e364d562. Pending sends also outlive fixture teardown and encounter restored secrets or a stopped Guardian.
Please register mw.close as cleanup for every fixture-created middleware, before environment/Guardian teardown, and replace the fixed sleeps with a delivery barrier or explicit drain before assertions. In a diagnostic run, replacing the two sleeps with mw.close() made all four emission tests pass. The new event-loop tests themselves pass; the regression is in the existing fixtures' assumptions, not a request to put network calls back on the loop.
Local validation (from adapters, Python 3.11):
python -m pytest tests/integrations/nat/test_event_loop.py tests/integrations/nat/test_emission.py -q --tb=short
Result: 2 failed, 11 passed, 22 warnings, 6 subtests passed in 23.70s
|
Closing this slice because PR #22 (the reference adapters for Claude Code, Cursor and NAT) merged into integration as 2630ec2, and that merge carries every slice in this stack. The top of the stack had the same files as PR #22's final commit, so nothing in this slice was left behind. Thanks, Bar, for cutting the adapters into pieces this size, because it made the review possible. |
Stacked on #483. Next: #485. The full stack is listed in #22.
What changed
build_acs_middlewareopens the session in a worker thread before it yields the middleware, unlessACS_DISABLED=1, and every call reuses that decision, a refusal or no answer included. A middleware built directly opens its session on the first call, off the loop and under a lock.pre_invokeandpost_invokerun their Guardian round trips withasyncio.to_thread.Ariel's findings this answers
import asyncioand twoasyncio.to_threadsuggestions.build_acs_middleware.Where this differs from Ariel's proposal
asyncio.to_thread, so they keep their order.Which issue does this implement
Part of #134
Base branch
Stacked on #483, not on
integration. It lands through #22; see the stack in #22.Type of change
I tested this
At this slice's top (
02888ed9), withclauderemoved from PATH andACS_LIVE_CLAUDEandACS_SPEC_DIRunset: 451 passed and 3 expected skips with the NAT extra, 392 passed and 3 expected skips without it. The skips are the two opt-in live Claude Code tests and the manual Cursor procedure. CI passes on this PR, including the installed-wheel check on Python 3.11 and 3.12.Checklist
git commit -s(required by the DCO)Security
Unchanged decisions; NAT's Guardian calls run off the event loop.