WIP: superseded ACP producer correlation candidate (#805 → #806) - #1183
Draft
sethkarten wants to merge 21 commits into
Draft
WIP: superseded ACP producer correlation candidate (#805 → #806)#1183sethkarten wants to merge 21 commits into
sethkarten wants to merge 21 commits into
Conversation
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.
The roster fix added coverage for a snapshot failure during session/new, but not for one at emission time. Those are separate call sites, and only the first was guarded: re-adding a swallowing `.catch(() => undefined)` on the emission read still passed the suite. That is the failure mode the metadata exists to prevent. Degrading to an empty roster reports `outstandingSubagents: 0` while children are still running, so a consumer scores a turn that has not finished -- wrong, and silently so. The new test drives the emission read to fail and asserts the prompt rejects rather than answering a clean end_turn with fabricated quiescence. Re-adding the swallow now fails it.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Comment on lines
+497
to
498
| if (abort.signal.aborted) return { stopReason: "cancelled" satisfies AcpStopReason }; | ||
| await connection.promptAndWait(text, images.length > 0 ? { images } : undefined); |
There was a problem hiding this comment.
🟡 Medium acp/acp-mode.ts:497
Two cancellation branches in session/prompt return { stopReason: "cancelled" } without awaiting entry.producer.drain(), unlike every other cancellation branch. If session/cancel arrives while getMessages() or getInitialSnapshot() is pending and subscription events have already queued producer notifications, the prompt response is sent before those updates are delivered, so they appear after the terminal prompt response and break the producer's update-before-response ordering. The missing await entry.producer.drain() calls are on the early return after connection.getMessages() and on the final pre-terminal return after the live snapshot read. Add await entry.producer.drain() before both returns.
if (abort.signal.aborted) {
+ await entry.producer.drain();
return { stopReason: "cancelled" satisfies AcpStopReason };
}
await connection.promptAndWait(text, images.length > 0 ? { images } : undefined);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/acp/acp-mode.ts around lines 497-498:
Two cancellation branches in `session/prompt` return `{ stopReason: "cancelled" }` without awaiting `entry.producer.drain()`, unlike every other cancellation branch. If `session/cancel` arrives while `getMessages()` or `getInitialSnapshot()` is pending and subscription events have already queued producer notifications, the prompt response is sent before those updates are delivered, so they appear after the terminal prompt response and break the producer's update-before-response ordering. The missing `await entry.producer.drain()` calls are on the early return after `connection.getMessages()` and on the final pre-terminal return after the live snapshot read. Add `await entry.producer.drain()` before both returns.
| } | ||
| const previousDescriptor = worker.descriptor; | ||
| worker.descriptor = { ...previousDescriptor, ownerClientId: undefined }; | ||
| worker.descriptor = { ...previousDescriptor, ownerClientId: undefined, launchEnv: undefined }; |
There was a problem hiding this comment.
🟠 High daemon/daemon-supervisor.ts:2092
promoteOwnedWorker clears launchEnv entirely when promoting a client-owned worker to resident, so a crash/restart after promotion loses the owner's persisted allowlisted environment settings (e.g. PATH, HOME, PI_PACKAGE_DIR, PI_OFFLINE). The recovered worker then launches with only the supervisor's ambient environment, which can select the wrong CLI or fail to relaunch. In launchWorker, resident recovery reads existing.descriptor.launchEnv, which was set to undefined during promotion. Consider retaining the persisted allowlisted subset via filterPersistedDaemonLaunchEnv(worker.launchEnv) instead of discarding all launch settings.
- worker.descriptor = { ...previousDescriptor, ownerClientId: undefined, launchEnv: undefined };
+ worker.descriptor = { ...previousDescriptor, ownerClientId: undefined, launchEnv: filterPersistedDaemonLaunchEnv(worker.launchEnv) };🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/daemon/daemon-supervisor.ts around line 2092:
`promoteOwnedWorker` clears `launchEnv` entirely when promoting a client-owned worker to resident, so a crash/restart after promotion loses the owner's persisted allowlisted environment settings (e.g. `PATH`, `HOME`, `PI_PACKAGE_DIR`, `PI_OFFLINE`). The recovered worker then launches with only the supervisor's ambient environment, which can select the wrong CLI or fail to relaunch. In `launchWorker`, resident recovery reads `existing.descriptor.launchEnv`, which was set to `undefined` during promotion. Consider retaining the persisted allowlisted subset via `filterPersistedDaemonLaunchEnv(worker.launchEnv)` instead of discarding all launch settings.
7 tasks
Contributor
Author
v0.8 stack checkpoint — 2026-08-11 15:50 UTCThis draft PR remains the GitHub ACP/Evaluation stack surface and will be kept current.
This is a progress checkpoint, not a readiness claim. |
sethkarten
changed the title
sethkarten
changed the title
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
1 participant
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.
Historical P2 candidate — superseded
This draft is frozen at
4673116c6debf5fe75650784cdefe9178078fc5bfor audit history. It is not the current P2 stack child and must not be merged.Current exact stack:
c3ab0a13ff164598ac42c3cca2bc426cc5cccd686dd9e86b75a18a13a7139631a679fb25593645d1, tree149ac0ccee3df28688bbd7d3d15ab7dbec3bd0c0No live or paid evaluation was run.