feat(coding-agent): keep ACP daemon sessions resident - #805
Closed
sethkarten wants to merge 8 commits into
Closed
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d4de117. Configure here.
snimu
reviewed
Aug 10, 2026
snimu
reviewed
Aug 10, 2026
snimu
reviewed
Aug 10, 2026
Two reviewers independently caught that `clientOwned: appMode !== "acp"` made every ACP session resident, including `--no-session`. A no-session run has no session file, so nothing can later reattach to it. Marking it resident means dispose() sends detach rather than completing the session, and the worker survives with no way to reclaim it -- a leak per ACP invocation. Interactive mode already passes `clientOwned: parsed.noSession` for this reason. ACP now requests resident only when it has a session to reattach to, which is the path this branch exists to enable; `--no-session` keeps the previous client-owned lifetime.
`launchEnv` was gated on `clientOwned`, so making ACP resident would have started its worker without the caller's environment. That gate was harmless while every ACP session was client-owned. It is not harmless now: the daemon still launches the worker for a resident session, and an embedder passes what the worker needs through that environment. The verifiers ACP harness supplies the model endpoint, its bearer token, and proxy settings exactly this way, so a resident worker would come up unable to reach the model and every rollout would fail with a connection error that looks like a provider outage. `collectDaemonLaunchEnv()` already strips the `PRIME_AGENT_INTERNAL_` role variables, which is the reason the gate existed, so forwarding it unconditionally is safe for both lifecycles. Found by tracing a live verifiers E2E failure rather than by review: the released 0.7.0 that CI installs still uses client-owned ACP, so this would only have bitten after this branch shipped.
The supervisor discarded launchEnv unless the session had an owner client:
const launchEnv =
ownerClientId || existing?.descriptor.ownerClientId
? (command.launchEnv ?? existing?.launchEnv)
: undefined;
A resident session has no owner client, so a resident worker launched without the
caller's environment. Making the client always send launchEnv (earlier on this
branch) was therefore not enough: the two halves disagreed and the worker still
came up with no model endpoint, no bearer token, and no proxy settings, which
surfaces as a provider connection error rather than a configuration fault.
Ownership governs worker LIFETIME, not whether the launch environment is honored,
so launchEnv is now kept whenever the command supplies it, retaining the fallback
to the existing worker's env on recovery.
The added test asserts the resident worker PROCESS actually observes the env --
an extension writes the received value to disk -- rather than checking the field
was passed along.
Note: daemon-supervisor-process.test.ts already fails 8/14 locally on macOS
(these spawn real supervisors over unix sockets under /var/folders); the same
baseline failures occur without this change, so CI on Linux is the signal here.
sethkarten
force-pushed
the
feat/acp-resident-lifecycle
branch
from
August 10, 2026 17:42
72266c8 to
0626d45
Compare
| extensionPath, | ||
| [ | ||
| "import { appendFileSync } from 'node:fs';", | ||
| `export default function() { appendFileSync(${JSON.stringify(markerPath)}, process.pid + ':' + process.env.PRIME_AGENT_TRACES_BASE_URL + ':' + (process.env.PRIME_AGENT_TEST_CREDENTIAL ?? '') + '\\n'); }`, |
This was referenced Aug 11, 2026
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Why
ACP hardcoded a client-owned daemon session, so the worker — and with it the live IPython kernel — stopped when the ACP client disconnected. Any ACP consumer that wants state to survive a turn boundary therefore had to keep one long-lived client process alive for the whole rollout, and build keepalive, transport and teardown machinery around it.
residentwas already on the wire (daemon-protocol.ts:DaemonSessionLifecycle = "resident" | "client_owned"), so ACP simply never asked for it.What changed
clientOwned: appMode !== "acp"at the daemon-client call site. ACP gets a resident worker;rpc,printandjsonkeep client-owned lifetimes unchanged.No protocol or schema change, and no new capability. An earlier draft of this branch bumped
DAEMON_PROTOCOL_VERSIONand negotiated anacp_resident_sessionscapability; both were removed. The bump was actively harmful — startup matches version and schema exactly, so protocol-7 daemons would go stale before any fallback could engage, letting a busy old daemon block ACP startup. A silent fallback to client-owned was also rejected: it would quietly destroy the persistence this change exists to provide.Re-attach
A resident worker is only useful if a later client can get back to the session. Prime Agent already supports this: relaunching ACP with
--continueresolves the resident session by canonical session file and attaches to it. Nosession/loadand no new ACP method are required.Validation
npm run checktest/acp-resident-lifecycle.test.tsandtest/daemon-protocol.test.ts: 14 passed, 1 skipped (the skipped one iskernel-heavy, run separately below)object()marker, disconnects ACP stdio, relaunches with--continue, and asserts live namespace identity —object()cannot survive a dill snapshot, so this proves the same interpreter, not a revived one.clientOwned: truefails that test (65s, no kernel survives); restoring passes (12s). Reverting protocol to 8 fails the schema/version assertions.Two pre-existing failures in
acp-features(goal timeout, post-close new session) reproduce onmainand are unrelated.Note
High Risk
Changes daemon session lifecycle, worker recovery, and on-disk descriptors plus credential handling; mistakes could leak secrets, break ACP/embedder model access, or leave workers in the wrong lifecycle after disconnect.
Overview
ACP daemon lifecycle now treats session-file ACP runs as resident workers (survive client disconnect;
--continuecan reattach), while ACP--no-sessionand other modes stay client-owned.isClientOwnedDaemonSessionencodes that rule at the daemon create call site.Launch environment is always sent on create (
launchEnv: collectDaemonLaunchEnv()for both lifecycles) so the first worker spawn gets caller endpoint/credentials/proxy settings. Only non-secret keys are written to resident worker descriptors viafilterPersistedDaemonLaunchEnv/DAEMON_PERSISTED_LAUNCH_ENV_KEYS; on load, descriptors are sanitized and rewritten if needed. Supervisor recovery relaunches resident workers from the persisted allowlist (not ambient supervisor secrets); client-owned workers keep transient env from the reconnecting owner. Promoting owned→resident clearslaunchEnvon the descriptor.Tests cover lifecycle negotiation, allowlist filtering, supervisor restart + worker recovery without credential persistence, and a kernel-heavy ACP disconnect/
--continuereattach integration test.Reviewed by Cursor Bugbot for commit 0626d45. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Keep ACP daemon sessions resident across supervisor restarts
isClientOwnedDaemonSessionutility;--no-sessionACP invocations remain client-owned.DAEMON_PERSISTED_LAUNCH_ENV_KEYS) across supervisor restarts, while client credentials and secrets are excluded.process.envto avoid leaking ambient secrets from the previous worker process.launchEnvfrom the descriptor so transient credentials are not persisted.launchEnvfield; existing descriptors are sanitized on load to remove any non-allowlisted keys.Macroscope summarized 0626d45.