fix(sessions): detach child sessions on delete to avoid FK rollback#710
fix(sessions): detach child sessions on delete to avoid FK rollback#710laofoot wants to merge 1 commit into
Conversation
deleteSessionRows deleted messages + the session row but never handled rows referencing it via the self-referential FK parent_session_id -> sessions.id (set by the agent backend for subagent runs / branches). better-sqlite3 enables PRAGMA foreign_keys=ON by default, so deleting a parent that still has a child threw 'FOREIGN KEY constraint failed'. deleteSession/deleteSessions run inside one transaction, so the violation rolled back the entire transaction -> 0 rows deleted, with no surfaced error (the renderer only console.errors it). A select-all delete almost always contains a parent->child pair, so the whole batch deleted nothing. Detach children (UPDATE parent_session_id = NULL) before deleting, guarded by a column-existence check for older schemas. Children are preserved (not cascade-deleted) so a session the user didn't select is never removed. Closes fathah#276
Greptile SummaryThis PR fixes a silent FK rollback bug where deleting a parent session (one with child sessions linked via
Confidence Score: 4/5Safe to merge for the primary fix; the detach-then-delete path is logically correct for single-level parent→child relationships, which is the common case. The core delete logic is correct and directly resolves the described FK rollback. The two findings are both non-blocking: the module-level column cache could cause the fix to be silently skipped if a migration runs mid-process (unlikely in normal desktop operation), and the shallow-only detach leaves multi-level subagent trees in a partially orphaned state. Neither finding reverses the benefit of the fix for the common case, but both are worth a follow-up. src/main/sessions.ts — specifically the Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let sessionsHasParentColumn: boolean | null = null; | ||
| function hasParentSessionColumn(db: Database.Database): boolean { | ||
| if (sessionsHasParentColumn !== null) return sessionsHasParentColumn; | ||
| try { | ||
| sessionsHasParentColumn = | ||
| db | ||
| .prepare( | ||
| "SELECT 1 FROM pragma_table_info('sessions') WHERE name = 'parent_session_id'", | ||
| ) | ||
| .get() != null; | ||
| } catch { | ||
| sessionsHasParentColumn = false; | ||
| } | ||
| return sessionsHasParentColumn; | ||
| } |
There was a problem hiding this comment.
db argument
sessionsHasParentColumn is a process-singleton, but hasParentSessionColumn accepts a db argument. Two problems follow:
- Schema migration after first call: if the column doesn't exist at startup, the cache is set to
false. A migration that later addsparent_session_id(during the same process lifetime) won't be detected, so the detach step is silently skipped and the FK error returns. - Tests or multiple DB paths: any test suite that opens a second database without
parent_session_id— or vice versa — will get the wrong cached answer for every subsequent call.
A simple fix is to key the cache on the database filename (db.name) or to clear sessionsHasParentColumn = null whenever the active database handle changes.
1 participant
Problem (fixes #276)
Deleting a session can silently fail: the row disappears optimistically, then reappears on refresh, with no error. Selecting all and deleting can delete nothing.
Root cause (verified on a real
state.db)The agent backend stores subagent runs / branches as child sessions linked via a self-referential FK:
better-sqlite3enablesPRAGMA foreign_keys=ONby default.deleteSessionRowsdeletesmessagesthensessions, but never handles rows referencing the session viaparent_session_id. So deleting a parent that still has a child throwsFOREIGN KEY constraint failed. BecausedeleteSession/deleteSessionsrun inside a single transaction, the violation rolls back the whole transaction → 0 rows deleted. The renderer onlyconsole.errors the rejection, so the UI shows no error. A "select all → delete" almost always contains a parent→child pair, so the entire batch deletes nothing.Fix
In
deleteSessionRows, detach child sessions (UPDATE sessions SET parent_session_id = NULL WHERE parent_session_id = ?) before deleting, guarded by a column-existence check for older schemas. Children are preserved (not cascade-deleted), so a session the user didn't explicitly select is never removed.Notes / open questions for maintainers
deleteSessionsreturns{requested, deleted}but the renderer ignores it. Surfacingdeleted < requestedwould make future failures visible (left out of this PR to keep it focused).Testing
Reproduced the FK rollback on a real
state.db(parent with a subagent child); confirmed the detach-then-delete sequence deletes the parent successfully underforeign_keys=ON.tscclean for the change.