Skip to content

Commit 695bfe7

Browse files
authored
fix(session-ingest): repair v2 session deletion query (#3662)
* fix(session-ingest): repair v2 session deletion query * fix(session-ingest): preserve child-first deletion event order
1 parent d0cdaac commit 695bfe7

2 files changed

Lines changed: 104 additions & 13 deletions

File tree

services/session-ingest/src/routes/api.test.ts

Lines changed: 93 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { SQL } from 'drizzle-orm';
2+
import { PgDialect } from 'drizzle-orm/pg-core';
13
import { Hono } from 'hono';
24
import { describe, expect, it, vi, beforeEach } from 'vitest';
35

@@ -79,7 +81,7 @@ function makeDbFakes() {
7981
const selectResult = vi.fn<() => Promise<unknown[]>>(async () => []);
8082
const select = {
8183
from: vi.fn(() => select),
82-
where: vi.fn(() => select),
84+
where: vi.fn((_condition: unknown) => select),
8385
limit: vi.fn(() => select),
8486
then: vi.fn((resolve: (v: unknown) => unknown) => resolve(selectResult())),
8587
};
@@ -99,7 +101,7 @@ function makeDbFakes() {
99101
// Drizzle delete chain: db.delete(table).where()
100102
const deleteResult = vi.fn<() => Promise<unknown>>(async () => undefined);
101103
const del = {
102-
where: vi.fn(() => del),
104+
where: vi.fn((_condition: unknown) => del),
103105
then: vi.fn((resolve: (v: unknown) => unknown) => resolve(deleteResult())),
104106
};
105107

@@ -131,11 +133,13 @@ function makeDbFakes() {
131133
insert: insertFn,
132134
insertResult,
133135
select: selectFn,
136+
selectWhere: select.where,
134137
update: updateFn,
135138
updateSet,
136139
updateWhere,
137140
updateReturning,
138141
delete: deleteFn,
142+
deleteWhere: del.where,
139143
selectResult,
140144
updateResult,
141145
deleteResult,
@@ -488,15 +492,36 @@ describe('api routes', () => {
488492
expect(ingestStub.getAllStream).toHaveBeenCalled();
489493
});
490494

491-
it('DELETE /session/:sessionId revokes cache, clears DO, and deletes row', async () => {
495+
it('DELETE /session/:sessionId revokes cache, clears DO, and deletes descendants child-first', async () => {
496+
const parentSessionId = 'ses_12345678901234567890123456';
497+
const childSessionId = 'ses_abcdefghijklmnopqrstuvwxyz';
492498
const { db, fns } = makeDbFakes();
493499
vi.mocked(getWorkerDb).mockReturnValue(db);
494500
// Ownership check
495-
fns.selectResult.mockResolvedValueOnce([{ session_id: 'ses_12345678901234567890123456' }]);
501+
fns.selectResult.mockResolvedValueOnce([{ session_id: parentSessionId }]);
496502
// Recursive CTE
497503
fns.executeResult.mockResolvedValueOnce({
498-
rows: [{ session_id: 'ses_12345678901234567890123456' }],
504+
rows: [{ session_id: childSessionId }, { session_id: parentSessionId }],
499505
});
506+
// Rows selected for session.deleted events
507+
fns.selectResult.mockResolvedValueOnce([
508+
{
509+
session_id: parentSessionId,
510+
parent_session_id: null,
511+
organization_id: null,
512+
git_url: null,
513+
git_branch: null,
514+
created_on_platform: null,
515+
},
516+
{
517+
session_id: childSessionId,
518+
parent_session_id: parentSessionId,
519+
organization_id: null,
520+
git_url: null,
521+
git_branch: null,
522+
created_on_platform: null,
523+
},
524+
]);
500525

501526
const sessionCache = {
502527
remove: vi.fn(async () => undefined),
@@ -513,17 +538,75 @@ describe('api routes', () => {
513538
);
514539

515540
const app = makeApiApp();
541+
const env = makeTestEnv();
516542
const res = await app.fetch(
517-
new Request('http://local/session/ses_12345678901234567890123456', {
543+
new Request(`http://local/session/${parentSessionId}`, {
518544
method: 'DELETE',
519545
}),
520-
makeTestEnv()
546+
env
521547
);
522548

523549
expect(res.status).toBe(200);
524-
expect(sessionCache.remove).toHaveBeenCalledWith('ses_12345678901234567890123456');
525-
expect(ingestStub.clear).toHaveBeenCalled();
526-
expect(fns.deleteResult).toHaveBeenCalled();
550+
551+
const deletedRowsPredicate = fns.selectWhere.mock.calls[1]?.[0];
552+
if (!(deletedRowsPredicate instanceof SQL)) {
553+
throw new Error('Expected pre-delete predicate');
554+
}
555+
const dialect = new PgDialect();
556+
const deletedRowsQuery = dialect.sqlToQuery(deletedRowsPredicate);
557+
expect(deletedRowsQuery.sql).toContain(
558+
'"cli_sessions_v2"."session_id" in ($1, $2) and "cli_sessions_v2"."kilo_user_id" = $3'
559+
);
560+
expect(deletedRowsQuery.params).toEqual([childSessionId, parentSessionId, 'usr_test']);
561+
562+
expect(fns.deleteWhere).toHaveBeenCalledTimes(2);
563+
const deletedSessionParams = fns.deleteWhere.mock.calls.map(([predicate]) => {
564+
if (!(predicate instanceof SQL)) {
565+
throw new Error('Expected delete predicate');
566+
}
567+
return dialect.sqlToQuery(predicate).params;
568+
});
569+
expect(deletedSessionParams).toEqual([
570+
[childSessionId, 'usr_test'],
571+
[parentSessionId, 'usr_test'],
572+
]);
573+
expect(sessionCache.remove).toHaveBeenNthCalledWith(1, childSessionId);
574+
expect(sessionCache.remove).toHaveBeenNthCalledWith(2, parentSessionId);
575+
expect(getSessionIngestDO).toHaveBeenNthCalledWith(1, env, {
576+
kiloUserId: 'usr_test',
577+
sessionId: childSessionId,
578+
});
579+
expect(getSessionIngestDO).toHaveBeenNthCalledWith(2, env, {
580+
kiloUserId: 'usr_test',
581+
sessionId: parentSessionId,
582+
});
583+
expect(ingestStub.clear).toHaveBeenCalledTimes(2);
584+
expect(notifyUserSessionEvent).toHaveBeenNthCalledWith(1, env, 'usr_test', {
585+
type: 'session.deleted',
586+
data: {
587+
source: 'v2',
588+
sessionId: childSessionId,
589+
parentSessionId: parentSessionId,
590+
organizationId: null,
591+
gitUrl: null,
592+
gitBranch: null,
593+
createdOnPlatform: null,
594+
deletedAt: expect.any(String),
595+
},
596+
});
597+
expect(notifyUserSessionEvent).toHaveBeenNthCalledWith(2, env, 'usr_test', {
598+
type: 'session.deleted',
599+
data: {
600+
source: 'v2',
601+
sessionId: parentSessionId,
602+
parentSessionId: null,
603+
organizationId: null,
604+
gitUrl: null,
605+
gitBranch: null,
606+
createdOnPlatform: null,
607+
deletedAt: expect.any(String),
608+
},
609+
});
527610
});
528611

529612
it('POST /session/:sessionId/share returns existing public_id when already shared', async () => {

services/session-ingest/src/routes/api.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Hono, type Context } from 'hono';
22
import { z } from 'zod';
3-
import { sql, eq, and, isNull } from 'drizzle-orm';
3+
import { sql, eq, and, inArray, isNull } from 'drizzle-orm';
44
import { getWorkerDb } from '@kilocode/db/client';
55
import { cli_sessions_v2 } from '@kilocode/db/schema';
66

@@ -146,7 +146,10 @@ api.delete('/session/:sessionId', async c => {
146146
.select()
147147
.from(cli_sessions_v2)
148148
.where(
149-
sql`${cli_sessions_v2.session_id} = ANY(${orderedSessionIds}) AND ${cli_sessions_v2.kilo_user_id} = ${kiloUserId}`
149+
and(
150+
inArray(cli_sessions_v2.session_id, orderedSessionIds),
151+
eq(cli_sessions_v2.kilo_user_id, kiloUserId)
152+
)
150153
);
151154

152155
await db.transaction(async tx => {
@@ -163,7 +166,12 @@ api.delete('/session/:sessionId', async c => {
163166
});
164167

165168
const deletedAt = new Date().toISOString();
166-
for (const row of deletedRows) {
169+
const deletedRowsBySessionId = new Map(deletedRows.map(row => [row.session_id, row]));
170+
for (const sessionId of orderedSessionIds) {
171+
const row = deletedRowsBySessionId.get(sessionId);
172+
if (!row) {
173+
continue;
174+
}
167175
notifyUserSessionEventFromContext(c, kiloUserId, {
168176
type: 'session.deleted',
169177
data: {

0 commit comments

Comments
 (0)