Skip to content

Keep NAT's Guardian calls off the event loop - #484

Closed
bar-capsule wants to merge 1 commit into
refimpl/adapters-08-session-statefrom
refimpl/adapters-09-nat-event-loop
Closed

bar-capsule wants to merge 1 commit into
refimpl/adapters-08-session-statefrom
refimpl/adapters-09-nat-event-loop

Conversation

@bar-capsule

@bar-capsule bar-capsule commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #483. Next: #485. The full stack is listed in #22.

What changed

  • build_acs_middleware opens the session in a worker thread before it yields the middleware, unless ACS_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_invoke and post_invoke run their Guardian round trips with asyncio.to_thread.
  • Lifecycle hooks go to one thread that sends them in the order NAT reports them. Each step waits until the hooks reported before it have gone out. When NAT shuts the middleware down, every accepted hook goes out first, in order, including one reported during the shutdown; a hook after that is audited, never sent from NAT's thread.

Ariel's findings this answers

Where this differs from Ariel's proposal

  • The lifecycle hooks, which his comment does not mention, move off the loop too, through one ordered worker rather than asyncio.to_thread, so they keep their order.
  • The async client and async transport stay the follow-up he proposed; this slice keeps the synchronous client and runs it in threads.

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

  • Reference implementation or adapter

I tested this

At this slice's top (02888ed9), with claude removed from PATH and ACS_LIVE_CLAUDE and ACS_SPEC_DIR unset: 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

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

Unchanged decisions; NAT's Guardian calls run off the event loop.

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 drk-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@rocklambros

Copy link
Copy Markdown
Collaborator

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.

@rocklambros rocklambros closed this Nov 5, 2026
@github-project-automation github-project-automation Bot moved this from In progress to Done in ACS Project Tracker Nov 5, 2026
@bar-capsule
bar-capsule deleted the refimpl/adapters-09-nat-event-loop branch November 8, 2026 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants