Attachment fixes to address inconsistencies between DB and backend store#1672
Merged
Conversation
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
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.
Summary
Fixes attachment handling to prevent orphaned S3/filesystem files and phantom DB records (DB records pointing to missing files). These issues were caused by race conditions during server restarts and missing safeguards in the cleanup logic.
Attachment expiry cap
attachment_expiresis now capped to never exceedmessage.expires, ensuring attachments can never outlive their messageSingle owner for file deletion
pruneAttachments(),pruneMessages(), andsync()could delete attachment files, creating race conditions during restarts (files deleted but DB rows surviving → phantom records causing user-facing 404s)sync()deletes files from storage.pruneAttachments()andpruneMessages()only update the DB. If the process is killed mid-operation, the worst case is orphaned files (cleaned up bysync()) rather than phantom DB recordsBatched message/attachment pruning
pruneMessages()deleted rows one-by-one in a transaction (242K individualDELETE WHERE mid=$1over remote Postgres = 115 seconds, blocking the entire manager cycle)DELETE ... WHERE mid IN (SELECT mid ... LIMIT $2)query, capped atManagerBatchSize(default 30K) per cycle. Backlog clears over multiple 1-minute cycles (~2s each) instead of one long blocking operationMissing Postgres index
idx_message_attachment_expires ON message (attachment_expires) WHERE attachment_deleted = FALSE— SQLite already had this, Postgres didn't. Without it,AttachmentsExpiredandAttachmentsWithSizesqueries did full table scans on 1M+ rowsConfigurable orphan grace period
orphanGracePeriodis now injected through the store constructor viaConfig.AttachmentOrphanGracePeriodinstead of being a package-level constantRemoved dead code
DeleteMessages(),MessagesExpired(),AttachmentsExpired(),MarkAttachmentsDeleted(), and their per-row queries — all replaced by the batched equivalentsTest plan