Expired messages no longer leave their plaintext on disk (android#11)
`attachmentBytes` wrote DECRYPTED bytes to `cacheDir/att-<id>` and nothing ever removed them. So a disappearing message expired, the bubble went, and the photo or voice note stayed on disk indefinitely — the exact failure the feature exists to prevent. Spec §6.11: attachments MUST be purged with the message that referenced them. 🔴 **A sweep, not a purge at each deletion point, and that changed what the fix had to be.** FOUR paths drop a row — message expiry, story expiry, delete-for-me and clear-thread — so threading "here is what I removed" out of each would still miss anything a future path drops, and would never reach the orphans already on disk. Asking instead "what is still referenced?" is complete by construction and self-healing. 🔴 **Refs are found by SHAPE.** An attachment reference serialises with `id`, `key` and `digest` together and nothing else in the vault has all three, so `attachmentIdsInUse` walks every record and matches on that. A hand-written list of stores is wrong the day somebody adds a store and does not update it — and the sweep would then delete attachments still in use, which is far worse than the leak. ⚠️ **That is not hypothetical: enumerating by hand gets it wrong today.** The obvious sources are history and stories. The vault ALSO holds refs in the story archive, in per-contact avatars and in this account's own avatar. A sweep built on the obvious two would have deleted every avatar on the device the first time it ran, and blanked the user's own story archive — which is the one thing that archive exists to prevent. Three more safety properties, each with a test that fails without it: fails closed when the in-use set cannot be built (a partial answer is what turns a cleanup into data loss); a grace period so a sweep racing a fetch cannot delete bytes on their way to the screen; and a throttle inside the function, since it reads every vault record and is called from the poll loop. ⚠️ The cost is that a file survives to the next sweep rather than going at the instant of expiry — named in the code rather than left implied. 17 tests. Every guard proved by breaking it. ⚠️ One did NOT hold at first: removing the `/` -> `_` filename mangling broke nothing, because every id in the suite was slash-free — the guard was untested by the tests meant to be guarding it. Added the case; it now fails as it should. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3caaa2172d
commit
46fb23dbe1
@@ -3287,7 +3287,16 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) {
|
||||
// A story arriving must appear without the user pulling
|
||||
// to refresh; it is filed by `receive()` and read back here.
|
||||
val received = withContext(Dispatchers.IO) {
|
||||
runCatching { current.receive() }
|
||||
val got = runCatching { current.receive() }
|
||||
// Cached plaintext for anything nothing refers to any
|
||||
// more — an expired message's photo, a story past its
|
||||
// lifetime. Throttled inside, and it declines to run at
|
||||
// all if it cannot tell what is still in use.
|
||||
runCatching {
|
||||
current.purgeOrphanedAttachments(
|
||||
getApplication<Application>().cacheDir)
|
||||
}
|
||||
got
|
||||
}
|
||||
// 🔴 410 from the poll: this device's registration is gone
|
||||
// — revoked from another phone, or expired by the sweep.
|
||||
|
||||
@@ -416,6 +416,72 @@ class Session(
|
||||
}
|
||||
|
||||
/** Bytes for an attachment, cached on disk after the first fetch. */
|
||||
/**
|
||||
* Delete cached attachment bytes that nothing refers to any more.
|
||||
*
|
||||
* 🔴 **`attachmentBytes` writes DECRYPTED bytes to the cache and nothing
|
||||
* ever removed them.** So a disappearing message expired, its bubble went,
|
||||
* and the plaintext photo or voice note stayed on disk indefinitely — which
|
||||
* is the exact failure the feature exists to prevent. Same for a story once
|
||||
* its expiry passed. Spec §6.11 says it plainly: attachments MUST be purged
|
||||
* with the message that referenced them.
|
||||
*
|
||||
* ⚠️ **A sweep, not a purge at the point of deletion, and that is a
|
||||
* deliberate choice.** Four different paths drop a row — expiry, story
|
||||
* expiry, delete-for-me and clear-thread — and threading "here is what I
|
||||
* removed" out of each one would still miss anything a future path drops,
|
||||
* and would never reach the orphans already on disk. Asking instead "what
|
||||
* is still referenced?" is complete by construction and self-healing.
|
||||
*
|
||||
* The cost is that a file survives until the next sweep rather than going
|
||||
* at the instant of expiry. Sweeps run on every poll, so that window is a
|
||||
* poll interval — worth naming rather than implying it is instant.
|
||||
*
|
||||
* 🔴 **It fails closed.** If the in-use set cannot be built — an
|
||||
* unreadable record, anything at all — nothing is deleted. A partial answer
|
||||
* to "what is still in use" is the one input that turns this from a cleanup
|
||||
* into data loss.
|
||||
*
|
||||
* ⚠️ [graceMillis] protects a file that was only just written, so a sweep
|
||||
* racing a fetch cannot delete bytes on their way to being shown.
|
||||
*
|
||||
* Returns how many files were removed, or 0 when it declined to run.
|
||||
*/
|
||||
private var lastAttachmentSweepAt = 0L
|
||||
|
||||
fun purgeOrphanedAttachments(
|
||||
cacheDir: java.io.File,
|
||||
now: Long = System.currentTimeMillis(),
|
||||
graceMillis: Long = 60_000L,
|
||||
minIntervalMillis: Long = 10 * 60_000L,
|
||||
): Int {
|
||||
// ⚠️ **Throttled here rather than at the call site**, so it is safe to
|
||||
// call from the poll loop unconditionally. Building the in-use set
|
||||
// reads every record in the vault; doing that every few seconds on a
|
||||
// phone with a lot of history would be a real cost for a cleanup that
|
||||
// does not need to be prompt.
|
||||
if (now - lastAttachmentSweepAt < minIntervalMillis) return 0
|
||||
lastAttachmentSweepAt = now
|
||||
|
||||
val inUse = try {
|
||||
// The SAME transformation `attachmentBytes` applies when it names
|
||||
// the file — comparing raw ids against mangled filenames would find
|
||||
// no match and delete everything.
|
||||
vault.attachmentIdsInUse().map { it.replace('/', '_') }.toSet()
|
||||
} catch (e: Exception) {
|
||||
return 0
|
||||
}
|
||||
val files = cacheDir.listFiles { f: java.io.File -> f.name.startsWith("att-") }
|
||||
?: return 0
|
||||
var removed = 0
|
||||
for (f in files) {
|
||||
if (now - f.lastModified() < graceMillis) continue
|
||||
if (f.name.removePrefix("att-") in inUse) continue
|
||||
if (f.delete()) removed++
|
||||
}
|
||||
return removed
|
||||
}
|
||||
|
||||
fun attachmentBytes(ref: Attachment.Ref, cacheDir: java.io.File): ByteArray {
|
||||
val cached = java.io.File(cacheDir, "att-" + ref.id.replace('/', '_'))
|
||||
if (cached.exists()) return cached.readBytes()
|
||||
|
||||
Reference in New Issue
Block a user