Repository navigation
fix(server): deleting a thread no longer removes another thread's attachments - #2
parmartejass wants to merge 5 commits into
Conversation
…achments Thread deletion only kept html-render ids minted for the deleted thread; message attachment ids were taken unfiltered. A thread whose messages reuse another thread's uploads (mobile "Work locally" relaunch) deleted the source's files. Both id kinds now share one ownership rule. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01DrdVGEmdN2qsKpaCV9dBFT
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fb52499b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Only ids minted for another thread are excluded now. Ids without a thread segment (pre-segment legacy data) are still cleaned up, and the legacy deletion test is back on its original fixture. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01DrdVGEmdN2qsKpaCV9dBFT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 924d9a4cf0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
github-actions
Bot
added
vouch:trusted
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6a27ca715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHERE remaining_thread.thread_id != ${threadId} | ||
| AND remaining_thread.deleted_at IS NULL | ||
| AND json_extract(remaining_attachment.value, '$.id') = json_extract(attachment.value, '$.id') |
There was a problem hiding this comment.
When two live threads sharing an attachment are deleted concurrently—for example from separate clients or simultaneous forced project deletions—the per-thread locks allow both lookups to observe the other thread as live, so both omit the shared ID. Both deletions then commit cleanup effects without that ID, and no surviving thread remains to schedule its removal, leaking the file indefinitely; the reference check and deletion commit need serialization across attachment owners or an atomic/ref-counted cleanup decision.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The race is real but narrow. Two threads sharing a file have to be deleted at the same moment, by separate clients or two project deletes at once. Project delete already removes its threads one at a time. The result is one leftover file on disk, not lost data. A fix needs a lock or ref-count across all threads, which is more code than one stray file justifies. Not changing it here; leaving this open for the author.
Generated by Claude Code
There was a problem hiding this comment.
Update: 34eb902 now serializes thread.delete through the orchestrator. One path remains: project deletion plans and commits thread deletes itself (ProjectService.ts:416-423) without that lock. Leaving this open for the author to decide.
Generated by Claude Code
AGENTS.md puts regression tests in the module's existing test file. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01DrdVGEmdN2qsKpaCV9dBFT
|
Failing check Generated by Claude Code |
Thread transfer impact
This comment will update automatically after the next completed run. |
|
CI run 39811700167 never ran on Generated by Claude Code |
Problem
Mobile “Work locally” can reuse a source thread's uploaded attachments. Deleting either thread previously scheduled those shared files for removal even while the other thread still referenced them.
Filtering by an attachment's thread prefix only protects deletion in one direction and excludes legacy IDs from cleanup.
Change
Both SQLite and in-memory projection stores now return message attachment IDs for cleanup only when no other non-deleted thread references them. Archived threads still protect their attachments; deleted threads no longer prevent cleanup when the last surviving thread is deleted. Legacy IDs follow the same reference checks, and the original legacy-deletion fixture is retained.
HTML-render ownership behavior is unchanged.
Scope and approval
This is a focused fix for an obvious data-loss bug in existing thread deletion. It qualifies for the small bug-fix exception in CONTRIBUTING.md and adds no product workflow or shared contract.
Verification
vp test run apps/server/src/orchestration-v2/ProjectionStore.attachments.test.ts apps/server/src/orchestration-v2/TurnStartReads.test.ts apps/server/src/project/ProjectService.deletion.test.ts apps/server/src/attachmentStore.test.ts apps/server/src/orchestration-v2/ThreadDeletion.test.ts: 23 tests passed.vp run check:changed upstream/main: format, lint, knip, GraphQL generation check, affected-package typecheck, and 38 adjacent tests passed.Original implementation: Claude Code. Revised and verified with GPT-6 in Codex.