Expired messages leave their decrypted attachments on disk forever #11

New Issue

2026-09-02T13:09:59Z

paulhitt commented

2026-09-02 13:09:59 +00:00

(Migrated from gitlab.com)

Session.attachmentBytes writes the decrypted bytes of every attachment to
cacheDir/att-<id> on first fetch, and nothing ever deletes them.

// app/src/main/kotlin/io/flamenet/messenger/app/Session.kt:261
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()
    val bytes = conversations.fetchAttachment(ref)
    cached.writeBytes(bytes)          // never removed
    return bytes
}

Vault.pruneExpired (Vault.kt:988) drops the message's JSON row and nothing
else. So when a disappearing message expires the bubble goes and the plaintext
photo, video or voice note stays on disk indefinitely — which is the exact
failure the feature exists to prevent. The same applies to a story once its
exp passes.

iOS does not have this: E2EInbox.pruneDisappeared calls purgeAttachments(of:)
for every message it drops.

Evidence: no delete of att-* exists anywhere in the tree —
grep -rn 'att-' app/src/main engine/src/main returns only the write site above
and unrelated JSON keys.

Fix: collect the att.id of every row pruneExpired removes, return them,
and have the caller delete the matching cache files. Same for expired stories.
The engine cannot do it alone — cacheDir is an Android context path and the
engine module deliberately has no Android dependency — so the ids have to come
back out to Session.

Found while writing spec §6.11, which states the requirement explicitly:
"Attachments MUST be purged with the message that referenced them."

`Session.attachmentBytes` writes the **decrypted** bytes of every attachment to `cacheDir/att-<id>` on first fetch, and nothing ever deletes them. ```kotlin // app/src/main/kotlin/io/flamenet/messenger/app/Session.kt:261 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() val bytes = conversations.fetchAttachment(ref) cached.writeBytes(bytes) // never removed return bytes } ``` `Vault.pruneExpired` (Vault.kt:988) drops the message's JSON row and nothing else. So when a disappearing message expires the bubble goes and **the plaintext photo, video or voice note stays on disk indefinitely** — which is the exact failure the feature exists to prevent. The same applies to a story once its `exp` passes. iOS does not have this: `E2EInbox.pruneDisappeared` calls `purgeAttachments(of:)` for every message it drops. **Evidence:** no `delete` of `att-*` exists anywhere in the tree — `grep -rn 'att-' app/src/main engine/src/main` returns only the write site above and unrelated JSON keys. **Fix:** collect the `att.id` of every row `pruneExpired` removes, return them, and have the caller delete the matching cache files. Same for expired stories. The engine cannot do it alone — `cacheDir` is an Android context path and the engine module deliberately has no Android dependency — so the ids have to come back out to `Session`. Found while writing spec §6.11, which states the requirement explicitly: *"Attachments MUST be purged with the message that referenced them."*

paulhitt commented

2026-09-04 22:29:57 +00:00

(Migrated from gitlab.com)

Fixed and merged (!60). Your diagnosis and evidence were exactly right; the fix went a different way than the suggestion, for a reason worth recording.

Why a sweep rather than returning the dropped ids

You suggested collecting the att.id of every row pruneExpired removes and having the caller delete them. I found four paths that drop a row, not two — message expiry, story expiry, deleteLine (delete-for-me) and forgetHistory (clear thread) — and saveStories alone has six call sites. 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 today.

So it asks the other question: what is still referenced? Complete by construction, and self-healing.

🔴 The part that would have made the obvious fix destructive

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 the collector walks every record rather than the stores we remember.

That is not tidiness. Enumerating by hand gets it wrong today. The obvious sources are history and stories. The vault also keeps refs in:

the story archive (ios#54) — an expired story is dropped from the live set AND folded into the archive, which keeps its ref. Purging on expiry would have left somebody's own archive full of blank pictures, which is the one thing the archive exists to prevent;
per-contact avatars, and this account's own avatar — fetched and cached by the very same code path.

A sweep built on the obvious two would have deleted every avatar on the device the first time it ran. Both cases now have a test that fails without the shape-matching.

Three more properties, each with a failing-without-it test

Fails closed. If the in-use set cannot be built — one unreadable record — nothing is deleted. A partial answer to "what is in use" is the input that turns a cleanup into data loss.
Grace period, so a sweep racing a fetch cannot delete bytes on their way to the screen.
Throttled inside the function, not at the call site, since it reads every vault record and runs from the poll loop.

⚠️ The honest cost: a file survives to the next sweep rather than going at the instant of expiry. That is a poll interval, and it is named in the code rather than implied.

On the tests

17, and every guard proved by breaking it — except one that 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 that case; it now fails as it should.

1057 tests green.

**Fixed and merged (!60).** Your diagnosis and evidence were exactly right; the fix went a different way than the suggestion, for a reason worth recording. ### Why a sweep rather than returning the dropped ids You suggested collecting the `att.id` of every row `pruneExpired` removes and having the caller delete them. I found **four** paths that drop a row, not two — message expiry, story expiry, `deleteLine` (delete-for-me) and `forgetHistory` (clear thread) — and `saveStories` alone has six call sites. 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 today. So it asks the other question: **what is still referenced?** Complete by construction, and self-healing. ### 🔴 The part that would have made the obvious fix destructive 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 the collector walks every record rather than the stores we remember. That is not tidiness. **Enumerating by hand gets it wrong today.** The obvious sources are history and stories. The vault also keeps refs in: - the **story archive** (ios#54) — an expired story is dropped from the live set AND folded into the archive, which keeps its ref. Purging on expiry would have left somebody's own archive full of blank pictures, which is the one thing the archive exists to prevent; - **per-contact avatars**, and **this account's own avatar** — fetched and cached by the very same code path. A sweep built on the obvious two would have deleted every avatar on the device the first time it ran. Both cases now have a test that fails without the shape-matching. ### Three more properties, each with a failing-without-it test - **Fails closed.** If the in-use set cannot be built — one unreadable record — nothing is deleted. A partial answer to "what is in use" is the input that turns a cleanup into data loss. - **Grace period**, so a sweep racing a fetch cannot delete bytes on their way to the screen. - **Throttled inside the function**, not at the call site, since it reads every vault record and runs from the poll loop. ⚠️ The honest cost: a file survives to the next sweep rather than going at the instant of expiry. That is a poll interval, and it is named in the code rather than implied. ### On the tests 17, and every guard proved by breaking it — except one that 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 that case; it now fails as it should. 1057 tests green.

paulhitt (Migrated from gitlab.com) closed this issue

2026-09-04 22:29:57 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: extant2000/flamenet-messenger-android#11