Skip to content

Delete chat attachments no chat references any more - #736

Open
devin-ai-integration[bot] wants to merge 10 commits into
mainfrom
devin/or-574-chat-attachments-are-never-deleted
Open

devin-ai-integration[bot] wants to merge 10 commits into
mainfrom
devin/or-574-chat-attachments-are-never-deleted

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Fixes OR-574. No linked contributor PR existed, so this starts from main.

Summary

  • Root cause: attachments are written to a flat global data_dir/chat-attachments/att-<uuid>__<name> folder, and neither ChatHost::delete_session nor the delete_project route touched it.
  • Store::chat_attachment_ids(session_id) collects the att-<uuid> ids (followed by __) that chat_messages, chat_turns.prepared_input and chat_queued_messages mention, for one chat or for all.
  • remove_unreferenced_attachments(store, dir, deleted) removes files no remaining row mentions:
    • chat delete / project delete: only the deleted chats' files, right away; a file another chat still references stays;
    • orx up background task (at startup, then hourly, off the async workers and skipped during a data-dir move): every unreferenced file older than one hour, which reclaims existing orphans. The hour spares a send in flight, which saves its file before recording the message.
  • Cleanup is best-effort: failures are logged and never fail the delete.
  • Known trade-off: reference detection is textual, so a message that pastes a full att-<uuid>__name file name keeps that file alive (errs toward keeping data).

Bug reproduction on main

Attach an image in a real Codex chat, get a reply, delete the chat: the file stays in chat-attachments.

Bug reproduction on main

Post-fix check (Linux, real Codex, rebuilt at 003a2d3)

Two image chats → deleting the first removes only its file; deleting the project removes the second; another project's attachment is untouched. Project delete used the new dialog from #690 with artifact removal left unchecked (the default).

Post-fix functionality check

Test plan

  • cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo build --locked
  • cargo test --locked: new deleting_a_chat_removes_only_its_unreferenced_attachments covers message/turn/queue references, shared-file preservation, deleted-chat removal and the age-gated sweep. The 3 local::projects::tests git-identity tests fail identically on unmodified main on this VM (global git identity) and pass in CI.
  • Dashboard check on Linux with a real Codex agent on HEAD 003a2d31, after merging main (recording above).
  • Not exercised at runtime: the hour-age sweep, skipping during a data-dir move, and forked/shared references. The unit test covers the age gate and shared references.

Automation improvements

  • scripts/dev-slot.mjs requires exactly one main worktree even with --worktree; the blueprint should create one (e.g. git worktree add ../OpenResearch-main main) so testing on a PR branch doesn't stall.

Link to Devin session: https://app.devin.ai/sessions/30dc8b28ff4448968d819277bde14641
Open in Devin Desktop: https://app.devin.ai/desktop/session/30dc8b28ff4448968d819277bde14641?variant=devin

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

… removal

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

This PR removes attachment files after chat or project deletion when no remaining chat references them.

  • Deleting a chat or project removes attachments no chat still uses.
  • The hourly orx up sweep clears old attachment files no chat uses.

The previous-review commit and current head have different merge bases with the target commit, so this review used the full PR diff.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Chat or project deletion] --> B[Collect attachment IDs before deleting rows]
  B --> C[Delete chat rows]
  C --> D[Find references in remaining chats]
  E[Startup or hourly sweep] --> F[Take data-directory gate]
  F --> G[Skip if a move is active]
  G --> H[Run cleanup on a blocking worker]
  H --> D
  D --> I{File still referenced?}
  I -->|Yes| J[Keep file]
  I -->|No| K{Deletion candidate or older than one hour?}
  K -->|Yes| L[Remove file and log failures]
  K -->|No| J
Loading

Reviews (4) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile

Comment thread src/local/chat/mod.rs Outdated
Comment thread src/store.rs Outdated
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@greptileai review

Comment thread src/commands/up.rs Outdated
Comment thread src/commands/up.rs Outdated
…a-dir moves

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@greptileai review

@devin-ai-integration devin-ai-integration Bot added the awaiting review Devin bugfix PR is open and needs human review before merge label Oct 21, 2026
…achments-are-never-deleted

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

# Conflicts:
#	src/commands/up.rs
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@greptileai review

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review Devin bugfix PR is open and needs human review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant