Skip to content

fix(server): report idle sessions as not-running on reconnect #402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dimakis wants to merge 1 commit into from
Closed

fix(server): report idle sessions as not-running on reconnect #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
98 changes: 98 additions & 0 deletions server/__tests__/ws-handler-v2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,104 @@ describe('handleReconnect reconnected summary (P1)', () => {
expect(reattachChat).not.toHaveBeenCalled();

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔴 regressions: The existing test at line 1064 ('reconnected summary does not include running field') explicitly asserts expect(summary.sessions[0]).not.toHaveProperty('running'). The three new tests directly contradict this by asserting running IS present with specific boolean values. One of these test groups must be updated — either the existing test is removed (if running is being added) or the new tests are wrong. [fixable]

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 missing_tests: No test verifies the behavioral side-effect: that idle sessions (lastSpeaker=assistant) skip the reattach logic on reconnect. The current tests only check the summary payload. A test should verify that reattachChat is NOT called for idle sessions even when the session is detached. [fixable]

});

it('reports running=false when session is active but idle (lastSpeaker=assistant)', () => {

@dimakis dimakis Jun 26, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 missing_tests: The new idle-session tests don't verify that reattachChat is NOT called when running is downgraded to false by the lastSpeaker check. The zombie test (line 1117) explicitly asserts expect(reattachChat).not.toHaveBeenCalled(), but the new idle tests skip this. Since the code at line 284 gates reattach on running, and running is now false for idle sessions, it would be good to confirm the reattach is correctly skipped — especially since the session is still alive (unlike zombies where it's removed). [fixable]

@dimakis dimakis Jun 26, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 missing_tests: No test covers the case where storeMeta exists, isActive is true, but lastSpeaker is null (brand-new session that hasn't had any turns yet). The implementation handles it correctly (null !== 'assistant' → running stays true), but an explicit test would document this edge case and guard against future regressions. [fixable]

const sessionReg = mockSessionRegistry();
sessionReg.findBySessionId.mockReturnValue({ clientId: 'driver-1' });
sessionReg.isActive.mockReturnValue(true);
sessionReg.isAttached.mockReturnValue(false);

const eventStore = mockEventStore();
eventStore.getSession.mockReturnValue({ isActive: true, lastSpeaker: 'assistant' });

const ctx = createContext({
sessionRegistry: sessionReg as unknown as V2HandlerContext['sessionRegistry'],
eventStore: eventStore as unknown as V2HandlerContext['eventStore'],
});
const transport = mockTransport();
ctx.connRegistry.register('c1', transport);

handleReconnect(
'c1',
{ type: 'reconnect', sessions: [{ sessionId: 'sess-idle', lastSeq: 0 }] },
ctx,
);

const summary = transport.sent.find((m) => m.type === 'reconnected') as {
sessions: Array<{ sessionId: string; running: boolean }>;
};
expect(summary.sessions[0].running).toBe(false);
});

it('reports running=true when session is active and mid-turn (lastSpeaker=user)', () => {
const sessionReg = mockSessionRegistry();
sessionReg.findBySessionId.mockReturnValue({ clientId: 'driver-1' });
sessionReg.isActive.mockReturnValue(true);
sessionReg.isAttached.mockReturnValue(false);

const eventStore = mockEventStore();
eventStore.getSession.mockReturnValue({ isActive: true, lastSpeaker: 'user' });

const ctx = createContext({
sessionRegistry: sessionReg as unknown as V2HandlerContext['sessionRegistry'],
eventStore: eventStore as unknown as V2HandlerContext['eventStore'],
});
const transport = mockTransport();
ctx.connRegistry.register('c1', transport);

handleReconnect(
'c1',
{ type: 'reconnect', sessions: [{ sessionId: 'sess-busy', lastSeq: 0 }] },
ctx,
);

const summary = transport.sent.find((m) => m.type === 'reconnected') as {
sessions: Array<{ sessionId: string; running: boolean }>;
};
expect(summary.sessions[0].running).toBe(true);
});

it('handles mixed running states across multiple sessions', () => {
const sessionReg = mockSessionRegistry();
sessionReg.findBySessionId
.mockReturnValueOnce({ clientId: 'driver-1' })
.mockReturnValueOnce(null)
.mockReturnValueOnce({ clientId: 'driver-3' });
sessionReg.isActive.mockReturnValueOnce(true).mockReturnValueOnce(false);

const ctx = createContext({
sessionRegistry: sessionReg as unknown as V2HandlerContext['sessionRegistry'],
});
const transport = mockTransport();
ctx.connRegistry.register('c1', transport);

handleReconnect(
'c1',
{
type: 'reconnect',
sessions: [
{ sessionId: 'sess-1', lastSeq: 0 },
{ sessionId: 'sess-2', lastSeq: 0 },
{ sessionId: 'sess-3', lastSeq: 0 },
],
},
ctx,
);

const summary = transport.sent.find((m) => m.type === 'reconnected') as {
sessions: Array<{ sessionId: string; running: boolean }>;
};
expect(summary.sessions).toHaveLength(3);
expect(summary.sessions[0]).toEqual(
expect.objectContaining({ sessionId: 'sess-1', running: true }),
);
expect(summary.sessions[1]).toEqual(
expect.objectContaining({ sessionId: 'sess-2', running: false }),
);
expect(summary.sessions[2]).toEqual(
expect.objectContaining({ sessionId: 'sess-3', running: false }),
);
});

it('replays multiple events in sequence order', () => {
const eventStore = mockEventStore();
eventStore.getEventsAfter.mockReturnValue([
Expand Down
16 changes: 16 additions & 0 deletions server/ws-handler-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,22 @@ export function handleReconnect(
});

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔴 bugs: The running field is never added to the reconnected summary message. The summaries array (line 233) is typed Array<{ sessionId: string; replayed: number }> and the summaries.push() at line 321 only pushes sessionId and replayed. The new code modifies a local running variable but never propagates it to the client-facing message. All three new tests will fail because summary.sessions[0].running is undefined, not true/false. The summaries.push() must include running for this fix to work. [fixable]

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 bugs: Even if running were added to the summary, the client-side protocol parser (packages/client/src/protocol-parser.ts:129-131) explicitly ignores it: 'Running state derived from replayed session_state_changed events, not from the reconnected message payload.' A client-side test also confirms this ('reconnected does not dispatch SET_RUNNING'). The stated goal (client sends messages immediately instead of queuing behind a 5-second timer) won't be achieved without client-side changes to consume the running field. [fixable]

ctx.sessionRegistry.remove(found!.clientId);
}
// Distinguish "query loop alive but idle" from "agent actively
// processing a turn". lastSpeaker === 'assistant' means the agent
// completed its last turn and is waiting for input — report as
// not-running so the client sends messages immediately instead of
// queuing them behind a 5-second fallback timer.
if (running) {
const storeMeta = ctx.eventStore.getSession(entry.sessionId);
if (storeMeta?.lastSpeaker === 'assistant') {
running = false;

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 regressions: Setting running = false for idle sessions prevents the reattach block (line 270: if (found && running && ...)) from executing on reconnect. The session stays detached and on the 10-minute TTL abort timer. The reattach only happens lazily on the next handleSendV2 call. If the user reconnects but doesn't send a message quickly, the session could be aborted. Consider whether idle sessions should still be reattached on reconnect even if reported as not-running. [fixable]

log.info('session alive but idle (last speaker: assistant)', {
connectionId,
sessionId: entry.sessionId,
clientId: found!.clientId,
});
}
}
if (found && running && !ctx.sessionRegistry.isAttached(found.clientId)) {
const ownerConnection = getOwnerConnection(found.clientId);
const ownerGone = !ctx.connRegistry.get(ownerConnection);
Expand Down
Loading