fix(session): release session-scoped resources on every path that ends a session - #634
Conversation
…s a session Closes agentrust-io#625. `rebind_session` rotated the session and its call logs but never called `aclose()`, which had no call sites anywhere in the repo. The stdio child, the pooled HTTP clients and the provenance/drift caches all survived a close and were reused by the next session, which is the cross-session contamination docs/spec/stdio-transport.md names as the reason children are scoped to one session rather than pooled. Three paths end a session, not one. Close and graceful shutdown were the two missing `aclose()` call sites. `POST /sessions/{id}/reset` retires a session id and opens a successor too, so it leaked the same resources for the same reason. Close now stops admission, drains admitted calls to a configurable deadline, records every terminal outcome including cancellations, signs, then releases resources and rotates. Reset drains and releases before recording its boundary, so a failed cleanup leaves the session untouched and the request retryable. A reset naming an already-rotated session is rejected before draining, so it cannot cancel the successor's in-flight calls. Cleanup promises only what it can keep. A child that fails to close is retained and the failure propagates, because the process is still live and a retry can reap it. An HTTP client cannot offer that: `AsyncClient` marks itself closed and HTTPcore empties its pool before the underlying streams are released, so a failed close leaves connections no retry reaches through any public API. Such a client is dropped and the failure logged. A call arriving during a transition waits for it and is admitted to the successor, but that wait is bounded. A transition that has already failed is lifted only by a close retry or operator action, so waiting on it indefinitely turned a failed close into an unreachable gateway that gave no signal. Verified on Python 3.11.16, 3.12.13 and 3.14.7. The close and reset regressions were confirmed to fail on the unfixed code path, asserting on the child's PID rather than on the absence of an error. The lifecycle invariants were also checked out of tree, across 500 randomized interleavings of calls, close, reset and shutdown under injected failure against real child processes. That harness was validated by mutation and is deliberately not part of the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yatsuiii <battyrises@gmail.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
carloshvp
left a comment
There was a problem hiding this comment.
Reviewed exact head 57245c2. No blocking findings in the session resource-lifetime fix.
Close drains admitted calls and records terminal outcomes before signing. Failed drain, terminal persistence, partial close, and retryable child cleanup preserve the sealed state needed to prevent work escaping an already-finalized claim. Close/reset release session resources, stale transitions are rejected, and graceful shutdown excludes new admission and first-use spawning. The documented HTTP cleanup limitation remains best-effort disposal, not a guarantee that failed socket cleanup can be retried.
Independent isolated macOS/Python 3.13 validation: full suite 1,774 passed, 14 skipped; Ruff, mypy, Bandit and whitespace checks passed. Six additional close/reset races, in both request orders and at three stagger intervals, each produced one successful boundary and one stale-session response, terminated the original real subprocess, and served the successor from a fresh child. The real-child close/reset regressions both fail when the proxy/server implementations are replaced with main's versions.
On the policy question raised in #625: preserving refusal after the identity's kill switch trips is appropriate for this change. Automatically substituting a different identity would evade the configured block. Availability and operator recovery limits are documented; no durable-recovery guarantee is implied.
The current merge simulation against freshly fetched main is conflict-free and the commit is signed off. #633 overlaps this lifecycle code; whichever lands second needs fresh integration validation, including discovery-cache reset. Approval covers this exact standalone head. Local tests use software test configuration and do not establish hardware attestation; repository gates remain separate.
Preserve the admission, drain, and retryable cleanup semantics from agentrust-io#634 while invalidating shared discovery and comparison caches together. Add public-call pagination/drain regressions across HTTP JSON, SSE, and stdio, plus failed-cleanup cache tests. Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
Closes #625.
Session close rotated the session and call logs but retained stdio children,
HTTP clients, and provenance/drift caches. The application also lacked
cleanup on graceful shutdown, and
POST /sessions/{id}/resetleaked the sameresources for the same reason: it retires a session id and opens a successor,
so the child, the pooled clients, and the caches outlived the session that
owned them.
Scope against the agreed contract
record terminal outcomes including cancellations, sign, release resources,
then rotate. Server pointers advance only after proxy rebind succeeds.
acquisition, drains active calls, and coordinates spawning with cleanup.
The spawn lock is retained here because shutdown uses it to exclude an
unfinished first-use spawn from its cleanup snapshot.
POST /sessions/{id}/resetends a session and opens a successor,so it now drains admitted calls and releases the same resources close does.
Cleanup precedes the recorded boundary. A resource-close failure leaves the
session ID and audit boundary unchanged and admission sealed, so retry can
close the retained resource before recording reset. Reset's sensitivity and
reset_countsemantics are unchanged.retained and the failure propagates, because the process is still live and a
retry can still reap it. A pooled HTTP client cannot offer that:
AsyncClient.aclose()marks the client closed and HTTPcore empties its poolbefore the underlying streams are released, so a failed close leaves
connections that no retry reaches through any public API. Rather than
advertise a recovery that does not exist, such a client is dropped and the
failure logged. Dropping it is what the successor needs regardless: it is
reuse, not the socket, that Session close retains stdio children and per-session upstream caches #625 is about.
is admitted to the successor. That wait is now bounded. A transition that has
already failed is lifted only by a close retry or operator action, so waiting
on one indefinitely turned a failed close into an unreachable gateway that
gave no signal at all. Past the bound the caller gets the reason instead.
an already-rotated session returns 404 instead of cancelling the successor's
in-flight calls.
no longer exists, caches clear, and the successor starts with fresh state.
The cancellation regression checks the real close endpoint's signed claim.
The failure handling below preserves those operations under retries and
concurrent close/reset requests. This does not implement execution correlation,
change reset's state-store semantics, or add durable recovery infrastructure.
Draining is what makes reset's cleanup safe rather than a new bug: closing a
child while an admitted call still held it would break that call.
Failure and recovery behavior
Drain waits up to
CMCP_SESSION_CLOSE_DRAIN_SECONDS(default 30 seconds),then requests cancellation and allows five seconds to unwind. An incomplete
drain keeps admission sealed; a close retry must finish draining first.
Shutdown remains terminal and reports an incomplete drain, or a child it could
not close, as failure rather than success. A pooled client it could not close is
logged instead, for the reason given below.
Transition ownership is separate from sealed admission, allowing serialized
retries. Completed claims are reused if successor creation or cleanup fails.
Failed cleanup retains resources, including the child process handle, for retry.
Reset cannot mutate a session awaiting drain or close recovery.
Hydration is inside the admitted call's terminal-finalization boundary. A failed
terminal audit write blocks signing, rotation, reset, and new call admission,
even after all call tasks have stopped. Restoring the writer does not recover
the missing outcome; this PR provides no repair operation for it. Shutdown can
still release resources without issuing a claim. Process-death tests use a
non-destructive Windows process wait and a POSIX PID probe.
Failure policy for a partial close. Once close bookkeeping starts, a
failure before the claim is stored leaves that session unavailable: new tool
calls stay blocked, and neither a close retry nor a reset resumes it. This is
not a preference.
close_session()records the session's outcomes into thekill-switch window and consumes a claim-sequence number before it signs, so
retrying a close that already partially ran would double-count both. Refusing
that retry is the rule stated on the issue, that a claim missing a call it made
costs more than a slow close, applied to a claim that would otherwise count one
twice. The cost is availability for that session even when the original failure
was transient. No repair operation is provided, because durable recovery is a
larger design than this change; restarting does not reconstruct the interrupted
claim either. Recovery is a new session.
The new exceptions currently reach the existing generic HTTP 500 handler;
detailed failure causes are logged.
Verification
suite on upstream
0db343a, on Linux/Python 3.14.7: 1772 passed, onebaseline failure, 14 skipped.
Linux/Python 3.11.16 (the declared floor) and 3.12.13: 1729 passed on each,
the same one baseline failure, 6 skipped (one integration module excluded for
a missing optional dependency in those throwaway environments, which are not
lock-faithful installs).
asyncio.wait_foraround
Condition.wait_for, was checked directly on 3.11, 3.12 and 3.14: thecondition lock is re-acquired before the timeout propagates, so the logic
that runs in the timeout branch holds it.
3.13 and every Windows run remain pending hosted CI; the Windows
process-death assertion in particular has never executed on Windows.
test_startup_fails_on_unknown_tee_provider_name, also occurson clean upstream. It patches the software-only provider while leaving
hardware auto-detection enabled, so this host selects its TPM instead.
terminal persistence during cancellation and ordinary failure, and real
child-process cleanup.
path before the change, asserting on the child's PID rather than on the
absence of an error.
aclose()as a coroutine, matchingthe real interface that reset awaits.
live HTTPcore pool on both
httpx==0.27.0(the floorpyproject.tomldeclares) and
httpx==0.28.1(what CI resolves), a client holding one pooledAsyncHTTPConnectionreports closed afteraclose()and leaves zeroconnections in its pool, so a retry through the client reaches nothing.
once against real httpx machinery (a transport whose
aclose()raises),asserting the client is dropped, the transition still succeeds, and the
failure is logged.
successor creation fails, after which a call is answered rather than held.
assumed: with no transition in flight it costs about 0.45us per call on 3.11
and 0.31us on 3.14, against the documented p50 budget of 1ms. The deadline is
only set up when a transition is actually in flight.
review here lived in combinations, so the combinations were generated and
checked out of tree. A harness drove calls, close, reset and shutdown against
each other in 500 randomized orderings, with injected child-close,
client-close and successor-creation failures, against 776 real stdio children
and sub-100ms drain deadlines. After every ordering it asserted that a 200
close or reset left no child of the retired session alive, that no call was
left counted or running, that admission was sealed only in a state the design
declares absorbing, and that every 200 close stored a claim. No violations,
no stray processes.
close cleanup, reverting the reset cleanup, and restoring the unbounded
admission wait each make it report the failure. It is a local verification
tool and is deliberately not added to the suite, since its value here was
finding the bugs rather than guarding against them, and its timing profile is
tuned to one platform.
git diff --check: passed.Limits
The drain deadline does not bound total request time, signing, transition-lock
acquisition, or arbitrary transport cleanup. Cancellation is cooperative;
a failed drain does not prove that a call was stopped. Hard process termination
cannot run lifespan cleanup. Pending recovery state is in memory only, and the
successor kept for a retried close is held until that retry lands.
Decision: a kill-switch trip at close takes the gateway out of service.
close_session()evaluates the kill switch andcreate_session()rejects onit, and successor creation runs after the close is committed. A session whose
own close trips the kill switch therefore signs and stores its claim and then
cannot produce a successor: the gateway stops admitting work until an operator
unblocks that identity and the close is retried. This PR refuses rather than
failing over, for two reasons. The identity is blocked by the operator's own
policy, so serving it a fresh session would route around that policy. And
before this PR the gateway kept serving the already-closed session, which put
those calls outside a signed claim, which is the failure the drain contract
exists to prevent. The refusal is explicit and bounded: callers get an error
naming the cause rather than blocking. The cost is the whole gateway rather
than the one session, so if you would rather it create a successor that is not
bound to the blocked identity, say so and I will change it here.
Deployment note.
shutdown()waits for the transition lock, so a closealready draining can hold it for
CMCP_SESSION_CLOSE_DRAIN_SECONDSplus thefive-second cancellation grace, after which shutdown runs its own drain on the
same budget. With the defaults that is up to 70 seconds before cleanup starts.
A container grace period shorter than that (30 seconds is a common default)
turns graceful shutdown into SIGKILL, and the lifespan cleanup this PR adds
never runs. Size the grace period above
2 * (drain + 5s), or lower the draindeadline.
LIMITATIONS.mdrecords the limits this change introduces, alongside theexisting ones, rather than leaving them only in this description. The
parity-checked shared block is untouched.
A failed HTTP client close leaks whatever connections it held. No public API
exposes them once
aclose()has run, and reaching them would mean depending onhttpx/HTTPcore private attributes, which was judged too fragile for the
guarantee it buys. The client is dropped, so the successor cannot reuse it, and
the leak is logged for the operator.
SessionDrainIncompletedeclareshttp_status = 503, but nothing in theruntime reads
http_status; like every other error class it surfaces throughthe generic 500 handler. Left as-is rather than changed in this PR.