fix: give session deletion an event, so a deleted session stays deleted - #348
Open
ProfSynapse wants to merge 3 commits into
Open
fix: give session deletion an event, so a deleted session stays deleted#348ProfSynapse wants to merge 3 commits into
ProfSynapse wants to merge 3 commits into
Conversation
…ays deleted `deleteSession` wrote no JSONL event at all — `session_deleted` did not exist (`grep -rn "session_deleted" src/` returned nothing). It ran one `DELETE FROM sessions WHERE id = ?` and trusted an `ON DELETE CASCADE` that never fires, because SQLite foreign-key enforcement is off on the shared connection. Measured in a real vault (Obsidian 1.13.7, headless), deleting a session holding 2 states / 2 traces / 1 trace embedding: after delete : sessions 0, states 2, memory_traces 2, trace_embedding_metadata 1 after rebuild: sessions 1, states 2, memory_traces 2 The session itself came back: the workspace stream still held `session_created` and replay had no tombstone to cancel it with. This adds `SessionDeletedEvent` and applies it on replay, and puts the row purge in `src/database/sessionOwnership.ts` — one declaration that both `SessionRepository.delete` and `WorkspaceEventApplier.applySessionDeleted` call, so the live delete and its replay cannot drift. Same machinery and the same reasoning as `workspaceOwnership.ts` (#347), with one deliberate difference: a session owns no stream of its own. Its events live in the parent workspace's stream, shared with the workspace and its sibling sessions, so there is no file to remove and the tombstone IS the removal. Ordering, and what a partial failure does: the tombstone is written first, so a failed SQLite purge leaves stale cache over an event store that already says "deleted" and the next rebuild converges on deleted. A failed tombstone write throws before SQLite is touched, leaving a retryable no-op. The reverse order is what produced the measured resurrection. No backfill for old JSONL. The only trace of a pre-fix delete is "in JSONL, absent from SQLite", which is also what a cold or partial cache looks like; inferring tombstones from it would turn rebuildable cache state into permanent loss. Old streams carry no tombstone, the session returns once, and the user's next delete is permanent — verified against a stream written by the pre-fix build. Forward compatibility is unchanged: appliers switch with no default, so an older build drops the unknown event instead of failing (verified live). Post-fix, same vault: 0 states, 0 traces, 0 embedding rows, session gone, and still gone after `rebuildCache()` and after a cold restart. The sibling session and the workspace are untouched. Internal/UI only — sessions are context fields and no tool deletes one; this does not add a delete tool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
… own
Both the live delete and the replay of it said "CASCADE will handle it". It does
not: SQLite foreign-key enforcement is per-connection and off by default, and
nothing in this plugin turns it on (`grep -rn "foreign_keys" src/` is empty), so
every `ON DELETE CASCADE` in schema.ts is documentation.
Measured in a real vault (Obsidian 1.13.7, headless):
delete a project with 3 tasks / 1 dependency edge / 1 note link
after delete : projects 0, tasks 3, task_dependencies 1, task_note_links 1
after rebuild: identical — the orphans are reproduced, not cleared
delete a single task with 1 incoming edge / 1 note link / 1 child
after delete : task_dependencies 1, task_note_links 1,
child.parentTaskId still pointing at the deleted task
after rebuild: identical
The rebuild reproduces them because replay re-creates every task from
`task_created` and then drops only the parent row.
`src/database/taskOwnership.ts` declares the statement list once —
child-before-parent, edges resolved through `tasks` while those rows still exist
— and `ProjectRepository`, `TaskRepository` and `TaskEventApplier` all call it,
so the live delete and its replay cannot drift. Same shape as
`sessionOwnership.ts` and `workspaceOwnership.ts` (#347).
Sub-tasks are detached, not deleted: `parentTaskId` is declared `ON DELETE SET
NULL`, so a child of a removed task survives with a null parent. Deleting it
would destroy work nobody asked to delete, and neither path ever claimed to.
Ordering is unchanged (tombstone first, then the cache), which keeps a partial
failure converging on deleted: stale rows over an event store that says deleted
are cleared by the next rebuild.
Post-fix, same vault: 0 tasks, 0 edges, 0 links, child parentTaskId null, and all
still zero after `rebuildCache()` and a cold restart. The bystander project and
its task are untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
The conversation tombstone was already right — the conversation does not come back — and `ConversationEventApplier.applyConversationDeleted` has always removed the messages explicitly. `ConversationRepository.delete` did not: it relied on the `ON DELETE CASCADE` on `messages.conversationId`, which never fires. Measured in a real vault (Obsidian 1.13.7, headless), deleting a 1-message conversation left `conversations 0, messages 1`. The orphan self-healed at the next `rebuildCache()` (the replay path does it correctly), so this is cache hygiene rather than a resurrection bug — but the two paths should agree. The other conversation gap measured in the same run is NOT fixed here: the stream directory `Nexus/data/conversations/conv_<id>/` survives the delete and the rebuild. Removing it needs the stream-removal helper from `fix/workspace-delete-ownership` (#347) — `JSONLWriter.deleteStream` down through `StorageRouter` / `VaultEventStore` / `ShardedJsonlStreamStore`. Re-implementing that on top of main would fork the mechanism and collide with #347, so it is left as a follow-up on that branch: one `deleteStream` call after the tombstone. The replay self-cancels either way, so nothing resurrects meanwhile. Also records what this pass learned in the nexus-storage skill: how to recognise a deletion with no event type at all, where a tombstone goes when the entity owns no stream of its own, and why a new deletion event must not be backfilled onto old JSONL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to #347. Same family, three different failures.
Best reviewed after #347 — it reuses that branch's ownership-module pattern, and target 3 is deliberately left for a one-liner once it lands.
Sessions: the delete was cache-only by construction
session_deleteddid not exist as an event type.grep -rn "session_deleted" src/returned nothing, sodeleteSessionwrote no event, replay had nothing to apply, and the row came back on the next rebuild.Measured on a seeded vault — 1 workspace, 2 sessions, 2 states + 2 traces + 1 trace-embedding row on the target:
Bystander session, its state and trace, the workspace, and the linked conversation are all untouched — pinned by tests.
A session owns no stream. Its events live in the parent workspace's shared stream, so none of #347's stream-removal machinery applies here: the tombstone is the removal. There is a test asserting the workspace stream and the sibling session survive.
Projects and tasks: the same unenforced-FK shape
Deleting a project with 3 tasks, 1 dependency edge and 1 note link left
tasks 3, task_dependencies 1, task_note_links 1— and the rebuild reproduced them.Single-task delete leaked too:
task_dependencies 1, task_note_links 1, and a child task'sparentTaskIdstill pointing at the deleted parent.taskOwnership.tshandles both. Note the parent link isSET NULL, notCASCADE, so sub-tasks are detached rather than deleted — that is the declared intent, and the fix honours it instead of cascading.Conversations: two gaps, not one
The live delete left 1 orphan
messagesrow that only self-healed on rebuild. Fixed here — it needed nothing from #347.The stream-directory leak is deferred: it needs #347's
deleteStreamchain (4 files, ~90 lines), and re-implementing that onmainwould fork the mechanism and collide on merge. One-line follow-up once #347 lands.Migration: none, deliberately
The only signal of a pre-fix delete is "present in JSONL, absent from SQLite" — which is indistinguishable from a cold or partial cache. Backfilling would turn rebuildable state into permanent loss.
Verified in both directions live: a stream written by the pre-fix build shows the session back, and one re-delete makes it permanent through a rebuild; and an unknown event type injected into a real stream is silently dropped with the rebuild succeeding — which is exactly how an older build will treat
session_deleted.Partial failure
Tombstone first, cache second, in both fixes. A failed tombstone throws before touching SQLite and is a retryable no-op (tested: zero statements issued). A failed purge leaves a stale cache over an event store that already says deleted, and the next rebuild converges on deleted. The reverse order converges on resurrection — the measured bug.
Evidence
npx jest tests/unit tests/integration— 342 suites, 4388 passing.npm run buildgreen. New tests fail pre-fix, verified against the base commit'ssrc/: 8/12, 7/8 and 1/2 respectively.Live rebuild and cold-restart verification passed with
dev:errorsempty. Nothing tool-reachable was added — sessions are context fields, not tools, and no delete tool exists for any of these.🤖 Generated with Claude Code
https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
Generated by Claude Code