From 12501412b9e008c15070238643869b794099d4f1 Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Wed, 1 Apr 2026 06:02:14 +0000 Subject: [PATCH] fix: persist session completion to database in completeByDbId (#1532) completeByDbId only cleaned up in-memory state, leaving sdk_sessions rows with status='active' and completed_at=NULL indefinitely. Ghost sessions accumulated and exhausted the agent pool, causing 60s timeout errors. - Add SessionStore.markSessionCompleted() to set status/completed_at/completed_at_epoch - Call it at the start of completeByDbId before in-memory cleanup - Inject SessionStore into SessionCompletionHandler via constructor - Add 4 tests covering status, timestamps, isolation, and non-existent IDs Closes #1532 Co-Authored-By: Claude --- src/services/sqlite/SessionStore.ts | 10 +++ .../worker/http/routes/SessionRoutes.ts | 3 +- .../session/SessionCompletionHandler.ts | 7 +- .../session-store-mark-completed.test.ts | 66 +++++++++++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 tests/services/sqlite/session-store-mark-completed.test.ts diff --git a/src/services/sqlite/SessionStore.ts b/src/services/sqlite/SessionStore.ts index 29c4beed..ab8cad3b 100644 --- a/src/services/sqlite/SessionStore.ts +++ b/src/services/sqlite/SessionStore.ts @@ -886,6 +886,16 @@ export class SessionStore { `).run(memorySessionId, sessionDbId); } + markSessionCompleted(sessionDbId: number): void { + const nowEpoch = Date.now(); + const nowIso = new Date(nowEpoch).toISOString(); + this.db.prepare(` + UPDATE sdk_sessions + SET status = 'completed', completed_at = ?, completed_at_epoch = ? + WHERE id = ? + `).run(nowIso, nowEpoch, sessionDbId); + } + /** * Ensures memory_session_id is registered in sdk_sessions before FK-constrained INSERT. * This fixes Issue #846 where observations fail after worker restart because the diff --git a/src/services/worker/http/routes/SessionRoutes.ts b/src/services/worker/http/routes/SessionRoutes.ts index 7b33c8c3..bfec5708 100644 --- a/src/services/worker/http/routes/SessionRoutes.ts +++ b/src/services/worker/http/routes/SessionRoutes.ts @@ -40,7 +40,8 @@ export class SessionRoutes extends BaseRouteHandler { super(); this.completionHandler = new SessionCompletionHandler( sessionManager, - eventBroadcaster + eventBroadcaster, + dbManager.getSessionStore() ); } diff --git a/src/services/worker/session/SessionCompletionHandler.ts b/src/services/worker/session/SessionCompletionHandler.ts index 405fe929..a599dfa8 100644 --- a/src/services/worker/session/SessionCompletionHandler.ts +++ b/src/services/worker/session/SessionCompletionHandler.ts @@ -11,12 +11,14 @@ import { SessionManager } from '../SessionManager.js'; import { SessionEventBroadcaster } from '../events/SessionEventBroadcaster.js'; +import { SessionStore } from '../../sqlite/SessionStore.js'; import { logger } from '../../../utils/logger.js'; export class SessionCompletionHandler { constructor( private sessionManager: SessionManager, - private eventBroadcaster: SessionEventBroadcaster + private eventBroadcaster: SessionEventBroadcaster, + private sessionStore: SessionStore ) {} /** @@ -24,6 +26,9 @@ export class SessionCompletionHandler { * Used by DELETE /api/sessions/:id and POST /api/sessions/:id/complete */ async completeByDbId(sessionDbId: number): Promise { + // Persist completion to database before in-memory cleanup (fix for #1532) + this.sessionStore.markSessionCompleted(sessionDbId); + // Delete from session manager (aborts SDK agent) await this.sessionManager.deleteSession(sessionDbId); diff --git a/tests/services/sqlite/session-store-mark-completed.test.ts b/tests/services/sqlite/session-store-mark-completed.test.ts new file mode 100644 index 00000000..36e3ff5b --- /dev/null +++ b/tests/services/sqlite/session-store-mark-completed.test.ts @@ -0,0 +1,66 @@ +/** + * Tests for SessionStore.markSessionCompleted (fix for #1532) + * + * Mock Justification: NONE (0% mock code) + * - Uses real SQLite with ':memory:' - tests actual SQL and schema + */ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { SessionStore } from '../../../src/services/sqlite/SessionStore.js'; + +describe('SessionStore.markSessionCompleted', () => { + let store: SessionStore; + + beforeEach(() => { + store = new SessionStore(':memory:'); + }); + + afterEach(() => { + store.close(); + }); + + it('sets status to completed and records completed_at timestamps', () => { + const before = Date.now(); + const id = store.createSDKSession('session-1', 'project', 'prompt'); + + store.markSessionCompleted(id); + + const row = store.db.prepare( + 'SELECT status, completed_at, completed_at_epoch FROM sdk_sessions WHERE id = ?' + ).get(id) as { status: string; completed_at: string; completed_at_epoch: number }; + + expect(row.status).toBe('completed'); + expect(row.completed_at).toBeTruthy(); + expect(row.completed_at_epoch).toBeGreaterThanOrEqual(before); + expect(row.completed_at_epoch).toBeLessThanOrEqual(Date.now()); + }); + + it('leaves other sessions unaffected', () => { + const id1 = store.createSDKSession('session-a', 'project', 'prompt'); + const id2 = store.createSDKSession('session-b', 'project', 'prompt'); + + store.markSessionCompleted(id1); + + const row2 = store.db.prepare( + 'SELECT status, completed_at FROM sdk_sessions WHERE id = ?' + ).get(id2) as { status: string; completed_at: string | null }; + + expect(row2.status).toBe('active'); + expect(row2.completed_at).toBeNull(); + }); + + it('does not throw when called on a non-existent session id', () => { + expect(() => store.markSessionCompleted(99999)).not.toThrow(); + }); + + it('completed_at is a valid ISO timestamp', () => { + const id = store.createSDKSession('session-iso', 'project', 'prompt'); + store.markSessionCompleted(id); + + const row = store.db.prepare( + 'SELECT completed_at FROM sdk_sessions WHERE id = ?' + ).get(id) as { completed_at: string }; + + expect(() => new Date(row.completed_at).toISOString()).not.toThrow(); + expect(new Date(row.completed_at).getTime()).toBeGreaterThan(0); + }); +});