Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions packages/cli/src/serve/httpAcpBridge.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ import type {
import { createDaemonStatusProvider } from './daemonStatusProvider.js';
import {
createHttpAcpBridge,
detachSessionIdFromEntryChannel,
findChannelInfoForEntry,
InvalidClientIdError,
InvalidPermissionOptionError,
InvalidSessionMetadataError,
Expand Down Expand Up @@ -299,6 +301,56 @@ function makeChannel(opts: FakeAgentOpts = {}): ChannelHandle {
}

describe('createHttpAcpBridge', () => {
it('selects a session entry channel when the current attach channel has changed', () => {
// Model the channel-overlap window directly: old channel A still has
// an entry, while new channel B is the current attach target.
const channelA = { name: 'A' };
const channelB = { name: 'B' };
const infoA = { channel: channelA, label: 'old-dying-channel' };
const infoB = { channel: channelB, label: 'fresh-attach-channel' };

expect(
findChannelInfoForEntry(infoB, [infoA, infoB], {
channel: channelA,
}),
).toBe(infoA);
expect(
findChannelInfoForEntry(infoB, [infoB], {
channel: channelA,
}),
).toBeUndefined();
});

it('detaches a session from its entry channel during channel overlap', () => {
// Model the close/kill overlap window directly: old channel A still owns
// the session entry while fresh channel B is the current attach target.
// The detach helper used by closeSession/killSession must decrement A,
// not the current B channel.
const channelA = { name: 'A' };
const channelB = { name: 'B' };
const infoA = {
channel: channelA,
sessionIds: new Set(['session-a']),
label: 'old-dying-channel',
};
const infoB = {
channel: channelB,
sessionIds: new Set(['session-b']),
label: 'fresh-attach-channel',
};

const selected = detachSessionIdFromEntryChannel(
infoB,
[infoA, infoB],
{ channel: channelA },
'session-a',
);

expect(selected).toBe(infoA);
expect(infoA.sessionIds.has('session-a')).toBe(false);
expect(Array.from(infoB.sessionIds)).toEqual(['session-b']);
});

it('accepts a valid BridgeOptions.eventRingSize at construction time', () => {
// Smoke: positive finite integers are accepted; the underlying
// EventBus ring-size threading is exercised end-to-end in
Expand Down Expand Up @@ -5840,6 +5892,37 @@ describe('createHttpAcpBridge', () => {
await bridge.shutdown();
});

it('keeps a shared ACP channel alive while closing one of several sessions', async () => {

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.

[Suggestion] The new test validates the "shared channel stays alive while closing one of several sessions" invariant, which is good regression coverage — but it does not exercise the specific bug this PR fixes.

All 3 sessions are spawned on a single channel (expect(handles).toHaveLength(1)), so the module-scoped channelInfo always points to the correct (only) channel throughout the test. The channelInfoForEntry fast path (channelInfo?.channel === entry.channel) always matches. The pre-fix code (const ci = channelInfo) would also pass this test.

The actual bug manifests only during a channel-overlap window: channel A is dying (SIGTERM in flight), channel B is freshly spawned (channelInfo = B), then closeSession is called on a session that still belongs to A. A regression test for this scenario would:

  1. Spawn sessions on channel A, trigger A into isDying (e.g., via slow-kill factory).
  2. Spawn a new session on channel B (channelInfo = B).
  3. Call closeSession on a session belonging to A.
  4. Assert A's sessionIds decremented (not B's) and B's channel is not killed.

Without this, a future revert to the buggy code goes undetected.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in e1bb5ef.

I added a focused regression for the overlap selector itself: it directly models old channel A still owning the session entry while fresh channel B is the current attach target, then asserts the lookup returns A and not B. I kept the existing closeSession shared-channel coverage as the lifecycle-level companion test.

Validation run:

npm run test --workspace=packages/cli -- src/serve/httpAcpBridge.test.ts -t "selects a session entry channel|closeSession|shared ACP channel"
npx prettier --check packages/cli/src/serve/httpAcpBridge.ts packages/cli/src/serve/httpAcpBridge.test.ts
npx eslint packages/cli/src/serve/httpAcpBridge.ts packages/cli/src/serve/httpAcpBridge.test.ts
npm run lint --workspace=packages/cli
npm run typecheck --workspace=packages/cli
git diff --check

I also re-ran the full httpAcpBridge.test.ts file: 162/163 passed; the only failure is the pre-existing Windows symlink permission failure at fsp.symlink(...) in writeTextFile preserves symlinks (BX8Yw).

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.

[Suggestion] This test validates the ref-count contract (channel survives partial close) but uses a single shared channel where channelInfo === entry.channel at all times — so channelInfoForEntry(entry) always hits the fast-path. The bug this PR fixes (channel overlap window where channelInfo has moved to a new channel while the session entry still points to the old one) is not exercised at the integration level.

Consider adding an overlap scenario: spawn session A on channel X, force channel X into isDying + spawn channel Y (so channelInfo points to Y), then closeSession(A.sessionId) and assert channel X's sessionIds was decremented (not Y's). This guards against accidentally reverting channelInfoForEntry(entry) back to channelInfo.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept this covered at the selector/unit boundary rather than adding a synthetic integration hook. In the current public bridge state machine, a session entry from channel A is not reachable for closeSession() after A has been marked dying and channel B has become the attach target: killSession/closeSession remove the entry before marking a channel dying when the last session leaves, while shutdown()/killAllSync() clear byId. The overlap lookup is therefore exercised by the focused findChannelInfoForEntry regression, and the public lifecycle tests cover shared-channel close/kill behavior plus the killSession-then-spawn overlap.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Handled in 0df4f66. I extracted the entry-channel detach path used by both closeSession and killSession, then added an overlap regression that models old channel A still owning the entry while fresh channel B is current. The test asserts the session id is removed from A and B is untouched.

const handles: ChannelHandle[] = [];
const factory: ChannelFactory = async () => {
const h = makeChannel({ sessionIdPrefix: `s${handles.length}` });
handles.push(h);
return h.channel;
};
const bridge = makeBridge({
channelFactory: factory,
sessionScope: 'thread',
});
const a = await bridge.spawnOrAttach({ workspaceCwd: WS_A });
const b = await bridge.spawnOrAttach({ workspaceCwd: WS_A });
const c = await bridge.spawnOrAttach({ workspaceCwd: WS_A });
expect(handles).toHaveLength(1);

await bridge.closeSession(b.sessionId);
expect(bridge.sessionCount).toBe(2);
expect(handles[0]?.killed).toBe(false);

await bridge.closeSession(a.sessionId);
expect(bridge.sessionCount).toBe(1);
expect(handles[0]?.killed).toBe(false);

await bridge.closeSession(c.sessionId);
expect(bridge.sessionCount).toBe(0);
expect(handles[0]?.killed).toBe(true);

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.

[Suggestion] This integration test validates single-channel ref-counting (3 sessions on one channel, close sequentially) — but channelInfo.channel === entry.channel is always true here, so findChannelInfoForEntry takes the fast-path and the aliveChannels fallback is never exercised. The bug this PR fixes — closeSession/killSession operating on the wrong channel when channelInfo has rotated to channel B while the entry still points to dying channel A — is only covered by the unit-level helper tests (lines 304-355).

Consider adding an integration test that goes through the closeSession entry point during overlap: spawn session on channel A, force A into isDying (e.g. via killSession on a sibling that triggers channel kill), let ensureChannel spawn channel B, then closeSession the original session and assert that A's channel is killed while B's sessionIds remains undisturbed. The existing ChannelFactory / slow-kill infrastructure (see "killSession marks the channel dying" test) supports this setup.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked this against the current bridge state. The public closeSession/killSession flow cannot naturally construct a stale entry channel while a fresh attach channel is current without adding a test-only state hook: when an entry is still live, the shared channel remains the attach target; when the last entry leaves, the entry is deleted before a fresh channel can become current. The actual regression point is the selector/detach boundary, which is covered by selects a session entry channel when the current attach channel has changed and detaches a session from its entry channel during channel overlap, where current.channel !== entry.channel and the aliveChannels fallback is exercised directly. I am leaving this at the helper boundary rather than adding a test-only bridge mutation path.


await bridge.shutdown();
});

it('throws SessionNotFoundError for unknown session', async () => {
const bridge = makeBridge();
await expect(bridge.closeSession('nonexistent')).rejects.toThrow(
Expand Down
58 changes: 41 additions & 17 deletions packages/cli/src/serve/httpAcpBridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,33 @@ interface ChannelInfo {
isDying: boolean;
}

/** @internal Visible for bridge lifecycle regression tests. */
export function findChannelInfoForEntry<T extends { channel: unknown }>(
current: T | undefined,
alive: Iterable<T>,
entry: { channel: unknown },
): T | undefined {
if (current?.channel === entry.channel) return current;
for (const info of alive) {
if (info.channel === entry.channel) return info;
}
return undefined;
}

/** @internal Visible for bridge lifecycle regression tests. */
export function detachSessionIdFromEntryChannel<
T extends { channel: unknown; sessionIds: Set<string> },
>(
current: T | undefined,
alive: Iterable<T>,
entry: { channel: unknown },
sessionId: string,
): T | undefined {
const info = findChannelInfoForEntry(current, alive, entry);
info?.sessionIds.delete(sessionId);
return info;
}

interface SessionEntry {
sessionId: string;
workspaceCwd: string;
Expand Down Expand Up @@ -2031,15 +2058,8 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
return channelInfo;
};

const channelInfoForEntry = (
entry: SessionEntry,
): ChannelInfo | undefined => {
if (channelInfo?.channel === entry.channel) return channelInfo;
for (const info of aliveChannels) {
if (info.channel === entry.channel) return info;
}
return undefined;
};
const channelInfoForEntry = (entry: SessionEntry): ChannelInfo | undefined =>
findChannelInfoForEntry(channelInfo, aliveChannels, entry);

const getChannelClosedReject = (info: ChannelInfo): Promise<never> => {
if (!info.statusClosedReject) {
Expand Down Expand Up @@ -2887,10 +2907,12 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
: ''),
);
if (defaultEntry === entry) defaultEntry = undefined;
const ci = channelInfo;
if (ci && ci.channel === entry.channel) {
ci.sessionIds.delete(sessionId);
}
const ci = detachSessionIdFromEntryChannel(
channelInfo,
aliveChannels,
entry,
sessionId,
);
for (const id of Array.from(entry.pendingPermissionIds)) {
resolvePending(id, { outcome: { outcome: 'cancelled' } });
}
Expand Down Expand Up @@ -3717,10 +3739,12 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
// Detach from the channel. The channel dies only when its LAST
// session leaves — other sessions on the same channel keep
// running.
const ci = channelInfo;
if (ci && ci.channel === entry.channel) {
ci.sessionIds.delete(sessionId);
}
const ci = detachSessionIdFromEntryChannel(
channelInfo,
aliveChannels,
entry,
sessionId,
);
// PR 14b fix (codex round 5): tombstone the killed sessionId
// so any in-flight `extNotification` from the (about-to-be-
// killed) child can't seed the early-event buffer for a
Expand Down
Loading