Skip to content

fix(agentos-sidecar): skip close_session teardown wait when the adapter already exited#1640

Draft
khaled-mansour-zid wants to merge 1 commit into
rivet-dev:mainfrom
khaled-mansour-zid:pr/close-session-already-gone
Draft

fix(agentos-sidecar): skip close_session teardown wait when the adapter already exited#1640
khaled-mansour-zid wants to merge 1 commit into
rivet-dev:mainfrom
khaled-mansour-zid:pr/close-session-already-gone

Conversation

@khaled-mansour-zid

Copy link
Copy Markdown

Summary

AcpExtension::close_session can block for ~2× SESSION_CLOSE_TIMEOUT (~10s) when the
adapter process has already exited out-of-band before the client closes the session. During
that block, because close_session runs serially per sidecar, a create_session issued right
afterward (e.g. session recovery) is stalled behind it.

This PR short-circuits the teardown when the process is already gone, reusing the same
"adapter is gone" classification the prompt path already uses.

Root cause — and why the current shape is otherwise correct

close_session does the standard graceful-shutdown dance:

kill_process_wire(SIGTERM)
if !wait_for_process_exit(SESSION_CLOSE_TIMEOUT) {   // 5s
    kill_process_wire(SIGKILL)
    wait_for_process_exit(SESSION_CLOSE_TIMEOUT)     // another 5s
}

That is correct for a live process being asked to shut down: SIGTERM, wait for its exit
event, escalate to SIGKILL if it's stubborn.

The gap is the already-dead case. wait_for_process_exit is purely event-driven — it polls
the event stream for a ProcessExitedEvent and never checks current process state:

async fn wait_for_process_exit(ctx, process_id, timeout) -> bool {
    let deadline = Instant::now() + timeout;
    loop {
        if Instant::now() >= deadline { return false; }
        let event = ctx.poll_event_wire(...).await?;
        if let ProcessExitedEvent(exited) = event.payload {
            if exited.process_id == process_id { return true; }
        }
    }
}

If the adapter exited before close_session (crash / OOM, or a lazily-observed idle-time
crash), its ProcessExited event already fired and was drained by the normal event loop. So this
poll can never observe it → it burns the full timeout for SIGTERM and again after SIGKILL.
Both signals are also no-ops (the PID is gone), so the whole ~10s is wasted.

Notably, the codebase already knows how to detect thisis_adapter_gone_error classifies an
out-of-band-dead adapter (via ADAPTER_EXITED_ERROR_MARKER / ADAPTER_NO_ACTIVE_PROCESS_MARKER)
and the prompt path uses it. close_session just doesn't consult it before waiting.

Real-world impact

The dominant trigger is not a rare crash — it's a returning user after idle eviction. When
an idle session's adapter process is evicted, the user's next turn finds it gone and the host
recovers by recreating the session; the recreate's implicit close of the dead session hits this
stall, so the user eats ~10s on their first message back. Mid-turn OOM recovery hits the same path.

Fix

Capture the SIGTERM result and skip the wait_for_process_exit dance when it reports the process
is already gone — detected via the existing is_adapter_gone_error plus the lower-level
process-table ESRCH / "no such process" the signal path returns for an already-reaped PID:

let already_gone = matches!(&sigterm, Err(error) if is_process_already_gone(error));
if !already_gone && !wait_for_process_exit(...).await {
    kill_process_wire(SIGKILL);
    wait_for_process_exit(...).await;
}

Behavior for a live process is unchanged; only the already-dead path is short-circuited straight to
resource disposal.

Validation

Built the sidecar with and without this change and measured createSession-after-guest-kill (kill
the ACP guest, then time the next createSession, which internally closes the dead session):

sidecar createSession after guest death
without this fix ~10.4s
with this fix ~0.3s

(35× on the recovery critical path.) Reordering the host-side recovery — recreate first, close the
dead session off the critical path — does not help, because the block is inside the sidecar's
teardown, not in host ordering; this sidecar-level fix is required.

Notes

  • Minimal and localized to close_session; reuses the existing gone-adapter classification.
  • A more general alternative would be to make wait_for_process_exit itself state-aware (return
    immediately if the process is no longer tracked) — happy to take that direction instead if you'd
    prefer the fix at that layer.

…er already exited

close_session runs SIGTERM -> wait_for_process_exit -> SIGKILL -> wait. That is
correct for a live process, but wait_for_process_exit only polls the event stream
for a future ProcessExited event and never checks current process state. When the
adapter exited out-of-band before close_session (crash / OOM, or a lazily-observed
idle-time crash), that event already fired and was drained, so both waits burn the
full SESSION_CLOSE_TIMEOUT (~2x total) against a PID that is already gone. Because
close_session is serial per sidecar, this also stalls a create_session issued right
after it (session recovery / a returning user whose idle session was evicted).

Detect the already-gone case from the SIGTERM result — reusing the existing
is_adapter_gone_error classification plus the process-table ESRCH / "no such
process" the signal path returns for a reaped PID — and skip straight to resource
disposal. Live-process behavior is unchanged.

Measured createSession-after-guest-death: ~10.4s without this change, ~0.3s with.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant