Conversation
A client with auto-download off has to decide between drawing a file and drawing something to press to fetch it, and had no way to tell which: the only question it could ask was `attachment_data`, which fetches on a miss -- so asking was indistinguishable from downloading. `Attachment::availability` answers it, as a field rather than a call, because the caller that needs it most is rendering a page of them across a language boundary and a round trip each would cost more than the whole read. It is a hint about what `attachment_data` will *do* -- read, join, or fetch -- rather than a promise about the file: a `cached` that is evicted first just means the call fetches, which is correct and merely slower. `cached` comes from the `attachment_cache` row rather than a stat, since that row exists precisely so eviction need not walk the directory, and `fetching` from `_in_flight`, checked first: a transfer under way has no cache row yet, and answering `absent` while the bytes are arriving is what puts a download button over the top of a progress bar. The message builders become members to reach it. They were file-local statics because they needed nothing but the database, which is no longer true -- the answer depends on the cache directory and on which transfers are running, and both of those are Client's. Threading it in as an argument was the alternative and a worse one: it is needed one level down, inside the reply targets a read builds with the same code, so every signature in between would have carried a parameter it does not use.
Once availability is part of the message, a change to it is a change to the message: an application holding one now holds something we know is wrong, and `message_updated` is how everything else in here says so. Fired on the three transitions a transfer makes -- `absent` to `fetching` when one starts, and back to `cached` or `absent` when it finishes or fails -- and for every message showing that file, since more than one routinely does. Not display pictures: one belongs to a conversation rather than to a message, and has its own progress handler to report itself with. The completion emit goes before the waiters are told, so that a waiter which reads a message in its callback finds the settled state rather than the one it was called about.
A cached file was named `blake2b(base_url)` -- unkeyed, unpersonalised, and so a function of information the adversary already has. That makes the cache directory a membership oracle: hash a url you are curious about, look for that filename, and you know whether this account downloaded it. Encrypting the contents does nothing about it, because the question is answered by the name before any file is opened. It applied to display pictures equally, so "does this account have a copy of that contact's avatar" was testable the same way. Which defeats the reason the cache is encrypted at all, stated in its own header: the disk is not a trusted place, it ends up in backups, in disk images, and in whoever's hands the machine does. So the name is a MAC rather than a digest, keyed on `client:cache_key` -- the secret the files are already encrypted under, which lives only in the encrypted database -- and personalised, so that one key serving two purposes is kept apart by construction rather than by the primitives happening not to meet. A listing now says how many files there are and nothing else. The key is applied in `Client::_cache_path`/`_cache_name` rather than at each call site: a caller that forgot it would silently get the guessable naming back, so the way to get a name is to ask the object that has the key. Every existing name changes, so the migration drops the cache rows: the files they name are then referenced by nothing, which is what a sweep collects. A cache is the one thing that costs nothing to lose.
Brings binding and retrieval of scoped enums, which the attachment code wants so a column whose values are a named set can be read back as that enum rather than as a bare int the caller has to recognise.
Every question the attachment cache asks of this table is "which messages show this file": which ones to report as fetching when a transfer starts, as cached when it finishes, and as absent again when the file is evicted. More than one message routinely quotes one file, since an attachment url is a hash of the encrypted body and so the same file sent twice by the same account lands at the same url. Without an index each of those answers is a scan of every attachment row in the account to touch a handful of them. Partial on url because a row without one has nothing to fetch and is never the subject of any of it.
A download that fails because the file server does not hold it, or because what arrived did not authenticate, says something about the file rather than about the attempt. Until now that was reported once, to whoever asked, and forgotten: the next thing to look at the message offered the same fetch again, and the only way to find out it could not work was to run it. Record it on the attachment instead, as `Attachment::unavailable`, so a display can say the file cannot be had rather than invite an attempt that will fail. The value says which of the two it was, because they are different things to tell a user: 404 -- which is how an expired upload answers -- means asking the sender for it again will work, and ATTACHMENT_UNREADABLE means it will not, since an attachment url is a hash of the encrypted body and a resend of the same file reproduces the same bytes. Nothing transient is recorded: a timeout or a server error says nothing about the file and the next attempt may well succeed. Set and cleared across every row naming the url, not just the row that was being fetched, since it is one file and a transcript must not show one message's copy as broken and another's as fine. Clearing happens when a new attachment row quoting that url arrives, which is the resend the user was told to ask for; the verdict has to be a cached answer rather than a permanent one, or it would block exactly the action that repairs it. Both directions emit message_updated for the messages they change.
`availability` says a download is running but not where it has reached, which leaves a conversation opened mid-transfer with a bar it cannot position: the progress reports went to whoever started the fetch, and a display that was not listening has no other way back to the figures. Carry them on the attachment alongside the state they qualify, counting the same encrypted bytes `AttachmentProgress` does, so a bar seeded from the message and then fed by reports continues rather than jumping. Also covers the availability states themselves, which went in untested: absent before anyone asks, fetching from the moment the transfer starts, cached once it finishes, and absent again when the cache gives that copy up for another file.
A message we just sent drew as something to press, and pressing it fetched back a file that had come off this disk a moment earlier. Keep what we upload in the attachment cache so an outgoing attachment answers "is this here" the same way an incoming one does. Under the rule that would have applied to the same file arriving -- shared with the auto-download path rather than restated -- plus anything on a gallery-viewable message whatever that rule says: a gallery is drawn as its pictures, so those bytes have to be here or the conversation shows placeholders over files it could read directly. The gallery rule decides that, the same rule the display applies, rather than an attachment happening to be an image: a message mixing a picture with a document draws as a list like any other. The source is re-read, since the upload streams the file rather than holding it, and only when its length still matches what was uploaded: a url names one particular encrypted body, and storing anything else under it would leave the cache answering for that url with the wrong bytes. Everything else about it is best effort -- a file that has since moved just means a download if the message is ever drawn again.
A cached file is named by a keyed hash of its url, and that hash was how
everything found it: "is this file here" hashed the url and looked the
result up. Two things follow from that, and both are worth being rid
of.
The hash becomes load-bearing for every file already downloaded rather
than only for the ones being written, so changing it cannot be done
without invalidating the lot -- which is exactly what 005 had to do.
And it does not run backwards, so eviction, which holds a file name and
needs the messages drawing that file, could not get to them at all: it
deleted the row and the file and said nothing, leaving conversations
showing pictures whose files had gone. `AttachmentAvailability` already
promises those changes are reported.
Give `attachment_cache` a surrogate key and have an attachment row
reference it, `ON DELETE SET NULL`. Nothing is duplicated -- the row
stores an integer, not a second copy of the name or the url -- and the
reference runs the way the questions do:
- availability comes back with the row, so reading a page no longer
hashes a url or looks anything up per attachment;
- finding an existing copy goes through the reference and uses the
name the entry was stored with, so the hash applies only to a file
being created and can change freely;
- eviction reads the messages off an index before the delete, and the
foreign key clears their reference on the way out.
The reconcile sweep drops its stale rows through the same path, so a
file that vanished behind our back is reported like one we deleted on
purpose. `_in_flight` moves to being keyed by url at the same time: the
hashed name is about what a directory listing reveals, which is not a
question a map in this process has.
Caching a file said nothing. On the download path that was hidden: the fetch's completion emitted right after storing, so the messages showing the file were told. An upload has no such completion, so keeping a copy of a file we sent -- which makes it drawable for every message quoting that url, and an attachment url is a hash of the encrypted body, so somebody having sent us the same file is the ordinary case -- told nobody but the message being sent. Move the emit to `_cache_attachment`, which is where a file becomes cached and is reached both ways. `store` then says whether it got that far, and the fetch reports the transfer ending only when it did not: a failure, a file that could not be written, and no cache at all are the same event to a reader, and all three leave nothing behind. The entry also has to leave `_in_flight` before any of that runs, or the report of a file arriving says it is still arriving. The test drives one file, shown by two messages, through every transition -- absent to fetching, back to absent for a transient failure, to cached, out again by eviction, to unavailable on a 404 and clear again on a resend -- and asserts which messages were told and what they were told the state was, rather than counting events. A second covers the upload case across conversations.
One cause routinely changes several messages, and reporting them one call at a time made the application reassemble what libsession had just taken apart. A file is the clearest case: it is one file, so evicting it, fetching it, or finding it unfetchable changes every message showing it at once -- and reporting a message also reports its replies, since a reply draws a preview of its target. So `message_added` and `message_updated` become `messages_added` and `messages_updated`, taking a vector. Two things are now promised that could not be before: - **ordered oldest first**, by the timestamp history is ordered on and then by id, so a handler applying a batch in order lands where a reload would put it. That is the reverse of `messages()`, which pages backwards from the newest; - **a batch may span conversations**, which is what the fan-out cases need. Nothing is lost by allowing it: `Message` already carries its own `conversation`, which is why the callbacks no longer take one. Eviction and the reconcile sweep now collect across the whole pass, so a message showing two of the files being dropped is told once rather than twice, and the ids are resolved to Messages only at the end -- one build per message, in its settled state rather than in whatever state it was in partway through the pass.
`FileTransferRequest::cancelled` was consulted on every upload path and on none of the streaming download ones, so cancelling a download only stopped us from using what arrived: the rest of the file came down the stream anyway and was dropped a chunk at a time. That is the common case rather than an unusual one. The stream scheme authenticates each chunk as it arrives, so a file that has been tampered with, or one that runs past the length its sender claimed, is known to be unusable from the bad chunk onwards -- and a large attachment can be most of the way through when that happens. Shutdown was the same: the routers cancel their active downloads and then close, with the transfers still running. Check the flag as each chunk arrives, which is the only moment this end of a transfer is given -- a download is driven by the server -- and abort the stream when it is set. A stalled stream still notices nothing, and the caller's timeouts remain what end those. The layer above needed one correction to suit it: a download we abandoned now completes with the cancellation we asked for, so the reason we abandoned it has to be preferred over the status when there is one. Otherwise a decryption failure would report itself as "download failed with status -10200", and the code that says a retry is pointless would be lost.
There was a getter for the cache limit and none for what is in it, so a client could show the ceiling but not how close to it the cache was. Summed from the cache index, in bytes on disk, which is the measure the limit is in and the one eviction compares against.
Eviction and `attachment_cache_size` were the same query written twice, which is also two copies of the answer to "why is this an optional" -- SQL sums an empty set to NULL, so an empty cache came back as no answer rather than as no bytes. Answer it once, in SQL, where the NULL is produced: `coalesce` makes the query incapable of returning one, so the C++ type stops advertising a state that cannot occur. That is how the rest of this file defaults a NULL already. The connection-taking overload is for eviction, which holds one and is about to write through it.
The schema grew over four migrations as this branch was built, one per commit that needed it, which is right while the commits have to stand on their own and wrong once they are merged: a migration is permanent, numbered and ordered, and these four describe intermediate states that have never existed anywhere but here. One migration per merged change instead. 005 now does what all four did, and the commits still show how it was arrived at. Its predecessors are exactly the databases that cannot take it -- they have some of the four recorded already -- so the branch goes on the schema-history skiplist: every commit of it but this one, since no state of it before this is something anything can have started from.
Nothing consumes a caption. It arrived on an incoming protobuf pointer, went into a column, came back out as a struct field, and went back out on the wire unchanged: no client renders it, nothing searches it, no index or constraint refers to it, and the conversation-preview query already skips it deliberately. A field that only round-trips is one more column to keep positionally correct in four statements for no behaviour. Field 11 is reserved rather than deleted: clients on the network keep sending it, and the number must never be handed to anything else. The checked-in protobuf output and the generated debug printer are regenerated to match, with protoc 3.21.12 - the version that produced the existing files.
| void Client::_set_attachment_unavailable( | ||
| std::string_view url, std::optional<AttachmentUnavailable> code) { | ||
| auto c = core.database().conn(); | ||
|
|
||
| // Only the rows this would actually change, which is what keeps a repeated report from | ||
| // re-announcing anything: a failure is reported afresh every time something asks for the file, | ||
| // and a resend arrives once per message rather than once per file. A *different* code is a | ||
| // change -- what to tell the user about it has changed -- so it is not filtered out. | ||
| std::vector<int64_t> changed; | ||
| for (auto message : c.prepared_results<int64_t>( | ||
| R"( | ||
| SELECT DISTINCT message FROM message_attachments | ||
| WHERE url = ?1 AND unavailable IS NOT ?2 | ||
| )"s, | ||
| url, | ||
| code)) | ||
| changed.push_back(message); | ||
|
|
||
| if (changed.empty()) | ||
| return; | ||
|
|
||
| // `IS NOT` rather than `!=`, here and above: one side is NULL in every case that matters -- | ||
| // setting a code on a row that has none, or clearing one back to NULL -- and `!=` answers NULL | ||
| // for those rather than true, so nothing would ever be updated. | ||
| c.prepared_exec( | ||
| "UPDATE message_attachments SET unavailable = ?2 WHERE url = ?1 AND unavailable IS NOT " | ||
| "?2", | ||
| url, | ||
| code); | ||
|
|
||
| // By url rather than by row, so every message quoting this file is answered at once rather | ||
| // than each discovering it in turn -- and in both directions: a resend makes the file | ||
| // fetchable again for the message that first failed, not only for the one that carried it. | ||
| _emit_messages_showing(c, changed); | ||
| } |
There was a problem hiding this comment.
Claude report, not sure if we want to fix it.
Essentially, if someone sends an url with an invalid key, he can make sure you never download the correct one.
unreadable is recorded url-wide, but unreadability is a property of (url, key, size), not of the url
_set_attachment_unavailable matches on url alone — no conversation, no row scoping — and ATTACHMENT_UNREADABLE is produced by failures that depend on this row's key, digest and size rather than on the bytes at the url:
#L4363 — decryption failed: this row's key is wrong.
#L4318-L4321 — attachment is longer than the {}B its sender said: this row's size is wrong.
#L4423-L4425 — the same check at completion.
Failure scenario.
Alice sends a photo to a closed group; every member sees its url U.
Mallory, also a member, sends you a 1:1 message quoting U with a garbage key. Auto-download (or a tap) fetches Mallory's copy, the stream fails to authenticate, and _set_attachment_unavailable(U, unreadable) writes unavailable = -20002 to every row naming U, in every conversation — including Alice's group message, whose key is fine. Per session/client/attachment.hpp#L134-L138 the client is now told "a resend of the same file reproduces these same bytes and this same failure", so a conforming display tells the user not to bother asking — and the column persists across restarts, with no clearing path short of a new row quoting U arriving. The non-adversarial version is two clients disagreeing about what belongs in AttachmentPointer.size for one file.
not_found genuinely is a fact about the url and should stay url-wide; unreadable should not.
Suggested fix: pass the failing row's identity down to _set_attachment_unavailable and, for unreadable, scope the UPDATE to rows sharing that url and the same key/size — or, simplest, to the (message, idx) that provoked the fetch. Leave not_found as it is.
| auto id = c.prepared_get<int64_t>("SELECT id FROM attachment_cache WHERE name = ?", name); | ||
|
|
||
| // Every message showing this file, not only whoever asked for it: it is one file, and a row | ||
| // that missed this would offer to fetch what is already here. | ||
| c.prepared_exec( | ||
| "UPDATE message_attachments SET cached = ?2 WHERE url = ?1 AND cached IS NOT ?2", | ||
| url, | ||
| id); | ||
|
|
||
| // Said here rather than by whoever asked for the bytes, because this is the moment it becomes | ||
| // true and there is more than one way to reach it: a download completing, and a file of our own | ||
| // being kept as it is uploaded. Only the first has a caller waiting to hear about it, so an | ||
| // emit belonging to that path told nobody about the second -- including the other messages | ||
| // quoting the same file, which an upload makes drawable just as much as a download does. | ||
| _emit_attachment_availability(url); | ||
|
|
||
| // After the emit, so the file that provoked this is reported as here before anything it pushed | ||
| // out is reported as gone. Checked when something is added, which is the only moment the total | ||
| // can grow. | ||
| _evict_cache(id); | ||
| return true; |
There was a problem hiding this comment.
Claude report, not sure if we want to fix it (it depends on the previous comment I guess)
A successful fetch never clears unavailable, so one row can report cached and unavailable at once
Failure scenario.
Given Warning 1 (my previous comment), Mallory's bad-key row has latched unreadable on Alice's row too. The user taps Alice's attachment anyway; that fetch uses Alice's correct key, succeeds, and the file lands in the cache. _load_attachments now builds an Attachment carrying availability == cached and unavailable == unreadable (#L3249-L3264). The header gives no precedence rule for that pair, and a display that checks unavailable first — which is exactly what include/session/client/attachment.hpp#L177-L180 tells it to do — draws "this file cannot be had" over a file sitting on this disk. The same contradiction is reachable without Warning 1 whenever a not_found was the server's temporary problem and a later retry succeeds.
Suggested fix: clear it in the same statement, and widen the change predicate so the emit still fires:
c.prepared_exec(
"UPDATE message_attachments SET cached = ?2, unavailable = NULL"
" WHERE url = ?1 AND (cached IS NOT ?2 OR unavailable IS NOT NULL)",
url, id);| auto& entry = _in_flight[name]; | ||
| // A transfer starting is a change to what every message showing this file can offer: `absent` | ||
| // a moment ago, `fetching` now. Attachments only -- a display picture belongs to a | ||
| // conversation rather than to a message, and has its own progress handler to say so. | ||
| if (target.dir == cache::ATTACHMENT_DIR) | ||
| _emit_attachment_availability(target.url); | ||
| entry.plain = std::make_shared<std::vector<std::byte>>(); | ||
| if (progress) |
There was a problem hiding this comment.
A fetch that cannot start leaves a permanent _in_flight entry: the file reports fetching for ever and its waiters never fire
_fetch_cached inserts into _in_flight, emits fetching to every message showing the file, and then calls _download_decrypted with no try. _download_decrypted throws before it reaches net->download in four cases (#L4248-L4254 and the need_key/digest checks just below): unparseable url, no network attached, wrong-length key, wrong-length legacy digest. The only place _in_flight is erased is the download's completion lambda (#L1175-L1182), which never runs.
Failure scenario. The application calls attachment_data before the network is attached, or a peer sends a pointer with a 5-byte key (explicitly stored — Warning 1). The caller gets its error, but:
_in_flight[url] survives for the life of the process, so _attachment_availability (#L3543-L3560) reports fetching, 0/0 on every message showing that file, for ever — a progress bar that never moves and never goes away, the exact state AttachmentAvailability exists to prevent;
every later attachment_data for that url joins the dead entry and never calls its callback (#L1113-L1124), and so does save_attachment (#L4832-L4842).
Half of this is pre-existing — the entry leaked before this PR too — but (1) is new, and it turns a silent leak into something permanently wrong on screen. A second consequence of the same missing try is also pre-existing and worth fixing while you are here: _auto_download calls _attachment_data directly (#L2270-L2280), so the throw escapes _on_message_received past _emit_message(true, client_id) and the arrival is never announced at all.
Suggested fix: wrap the _download_decrypted call; on throw, erase the entry, re-emit availability, and report the error to everything registered on it:
try {
_download_decrypted(...);
} catch (const std::exception& e) {
auto dead = std::move(_in_flight[name]);
_in_flight.erase(name);
if (target.dir == cache::ATTACHMENT_DIR)
_emit_attachment_availability(target.url);
for (const auto& w : dead.waiting)
_report(w, std::optional{std::string{e.what()}}, std::vector<std::byte>{});
return;
}
| _download_decrypted( | ||
| url, | ||
| DownloadKind::attachment, | ||
| std::move(key), | ||
| std::move(digest), | ||
| claimed_size, | ||
| [state](std::span<const std::byte> plain) { | ||
| state->out.write(reinterpret_cast<const char*>(plain.data()), plain.size()); | ||
| }, | ||
| report, | ||
| finish); |
There was a problem hiding this comment.
I am not too sure if this one is relevant as this part is hard to follow, also, we shouldn't get too often multiple times the same file offered for download.
_save_attachment starts a real transfer that _in_flight never learns about
A save that is not served from the cache and does not join an existing transfer calls _download_decrypted directly: nothing goes into _in_flight, and no availability is emitted. The comment above it explains why nothing is registered (a save streams to its destination and keeps no buffer to hand a joiner), and that reasoning is right for waiting — but it also makes the transfer invisible to availability.
Failure scenario. The user taps "save" on a 40 MB video that is not cached. For the whole download, every message showing that file reports availability == absent, so the conversation view draws a download button over a file that is at that moment coming down the wire; tapping it starts a second, independent transfer of the same bytes. Meanwhile include/session/client/attachment.hpp#L109-L112 promises fetching means "a transfer is already under way, whoever started it — an auto-download, another message quoting the same file, or another part of the application".
Suggested fix: either register a save in _in_flight with a null plain and a flag saying it is not joinable (progress and availability then work; waiting refuses), or narrow the header to say outright that a save in progress reads as absent. The first is better — it also removes the duplicate download.
| @@ -1050,6 +1124,11 @@ | |||
| } | |||
|
|
|||
| auto& entry = _in_flight[name]; | |||
| // A transfer starting is a change to what every message showing this file can offer: `absent` | |||
| // a moment ago, `fetching` now. Attachments only -- a display picture belongs to a | |||
| // conversation rather than to a message, and has its own progress handler to say so. | |||
| if (target.dir == cache::ATTACHMENT_DIR) | |||
| _emit_attachment_availability(target.url); | |||
| entry.plain = std::make_shared<std::vector<std::byte>>(); | |||
There was a problem hiding this comment.
Same comment, idk if we want to fix this as it seems unlikely to get in different messages the same attachment
Suggestion 1 — _in_flight and the cached/unavailable lookups key on the full url; the cache file keys on the base url
Before this PR _in_flight was keyed by path_for(...).filename() — a hash of the url with query and fragment stripped — so two references to one file differing only in fragment deduplicated. It is now keyed by the raw url while cache::name_for still strips ?/#. The same split applies to _messages_showing, cached_id_for and _set_attachment_unavailable, all of which compare url = ? exactly.
Consequence: two rows quoting …/file/123 and…/file/123#… start two concurrent downloads of the same bytes, and only the second to finish gets cached set on its own rows. It converges after one redundant download, and our own outgoing urls always carry the same fragment (generate_download_url(..., stream_encrypted=true)), so this is a cost rather than a correctness bug — but it is a small regression from the old keying. Keying _in_flight on cache::name_for(key, url), or on a stored base_url(url), restores it.
| } | ||
|
|
||
| void Client::_cache_attachment( | ||
| void Client::_cache_outgoing_attachment(int64_t client_id, size_t index, const std::string& url) { |
There was a problem hiding this comment.
This one might be resolved by your PR about the threading contract. Let me know if not.
Suggestion 2 — _cache_outgoing_attachment reads and re-encrypts the whole file on the Core loop thread, unbounded
It runs inside _upload_next's loop.call continuation, allocates std::vector<std::byte>(size), reads the file synchronously, and hands it to cache::write, which encrypts it in one pass. The AUTO_DL_MAX_KEY ceiling is checked first, but that limit is optional (#L4527) — with no limit set, sending a 200 MB file stalls the loop, and everything queued behind it, for a full read plus an encryption pass. Worth either a hard ceiling of its own or moving the read and encrypt off the loop, the way _sweep_cache already moves the directory walk.
There was a problem hiding this comment.
Suggestion 4 — Migration 005 duplicates ~90 lines of full_schema.sql prose, against this directory's own convention
Suggestion 5 — Migration 005's header narrates the branch rather than stating a constraint
| -- `name` is the file's name, which is the hashed base url -- the same value `cache::path_for` | ||
| -- produces -- so a row can be matched to a file, and to a `message_attachments.url`, without | ||
| -- storing either the path or the url. |
There was a problem hiding this comment.
Suggestion 6 — full_schema.sql's attachment_cache preamble is now wrong about how name is produced
cache::path_for now returns a path and takes a key; the name comes from cache::name_for. More to the point, "a row can be matched … to a message_attachments.url" is exactly what this PR abolished — the hash is keyed and does not run backwards, which is the whole reason cached exists. Unchanged lines, but this PR is what made them false, and they sit directly above the block it did rewrite.
2 participants
A client with auto-download off cannot currently draw an attachment without guessing. It knows the file exists and it knows how to ask for the bytes, but not whether asking means a disk read or a download — so it either fetches everything to find out, or offers a download button over a file that is already here.
if (already_have_it) show_it(); else show_download_button();was not answerable.This adds that, as fields on
Attachmentrather than as a separate query: a page of fifty attachments should not cost fifty round trips across a language boundary to find out what to draw.This PR supercedes/includes parts of #166 and #164.
What a client gets
Attachment::availabilityiscached,fetching, orabsent— respectively "a read", "join the transfer already running", and "the decision is the user's".fetch_done/fetch_totalsay how far a running transfer has reached, in the same encrypted bytesAttachmentProgresscounts, so a conversation opened midway can draw a bar that continues rather than jumps.unavailablerecords a fetch that failed in a way that trying again will not fix, and says which:not_found(404, which is also how an expired upload answers) orunreadable(the bytes arrived and did not authenticate). Only the first is worth telling a user to ask for a resend about.unavailableis a cached answer, not a fact about the url, and that distinction is load-bearing. An attachment url is a hash of the encrypted body and the encryption is deterministic, so the same file sent again by the same account lands at the same url. That is the repair path — the recipient is told it could not be fetched, asks for it again, the sender's re-upload puts those bytes back where they were — and a flag that never cleared would block exactly the action that fixes the problem. So it is set across every row naming a url when a fetch of it fails, and cleared across every row naming that url when a new attachment row quoting it arrives.What we send is kept under the same rule that would have fetched it had it arrived, plus anything on a gallery-viewable message whatever that rule says: a gallery is drawn as its pictures, so those bytes have to be here or the conversation shows placeholders over files that came off this disk. The gallery rule decides it — the same rule the display applies — rather than an attachment happening to be an image, so a message mixing a picture with a document draws as a list like any other and neither file is kept.
Breaking change
message_addedandmessage_updatedbecomemessages_addedandmessages_updated, takingstd::vector<Message>&&, and both lose theirConversationId&¶meter.Message::conversationalready carried it, and passing it alongside stops making sense once a batch can span conversations. Anything implementing these needs updating — session-cli and the session-app bridge at least.One cause routinely changes several messages, and reporting them one call at a time made the application reassemble what libsession had just taken apart. A file is the clearest case: it is one file, so evicting it, fetching it, or finding it unfetchable changes every message showing it at once — and reporting a message also reports its replies, since a reply draws a preview of its target. Two things are now promised that could not be before:
Conversation::messages(), which pages backwards from the newest.Schema
One migration,
005_attachment_availability, in whichattachment_cacheis dropped and recreated: cached files become orphans that the reconcile sweep clears on its next pass. The cache is disposable by construction, its filenames are keyed now so the old ones name files nothing will look up again, and a surrogate key cannot be added to an existing table without rebuilding it anyway. Nothing else loses data. It also adds theunavailablecolumn, thecachedreference, and a partial index for each.The schema grew over four migrations as the branch was built, one per commit that needed it — right while the commits have to stand on their own, wrong once they merge, since a migration is permanent and numbered and those four describe states that have never existed anywhere but this branch. They are collapsed in the last commit and the history still shows how it was arrived at.
That makes the branch's own intermediate commits the one thing that cannot take the replacement — they have some of the four recorded already — so they go on
tests/schema_history_skip.txt, every commit of the branch but the last. A rebase after this point invalidates those hashes and the list has to be regenerated; merging keeps them reachable and it stays valid. Verified separately from the walk: a database atclienttakes the new005cleanly.Two things worth a reviewer's attention
The cache directory was a membership oracle. Files were named by an unkeyed hash of the url, so anyone who could list the directory could test whether this account had downloaded a particular file — they need only hash the url they are interested in and look. Names are now keyed on the account's cache key with a personalisation, which is half of why the migration discards the cache.
Eviction said nothing. It deleted the row, unlinked the file, and left every message that was drawing that file silently claiming to have it.
AttachmentAvailabilitydocuments that changes are reported, so this was a promise the code did not keep. It could not: eviction holds a file name, the name is a keyed hash, and a keyed hash does not run backwards — reaching the messages meant hashing every url in the table. Thecachedreference is what makes the question answerable, and it also takes the hash off the read path entirely: finding an existing copy now goes through the stored name rather than recomputing it, so a future change to the naming costs nothing already downloaded instead of costing the whole cache again.Supersedes #164
#164 — Report how full the attachment cache is — is cherry-picked here with its authorship intact, so it should be closed rather than merged. It is carried here rather than left to land on its own because the two do not survive each other: keying the cache filenames changed
cache::path_forto take the account's key, and #164's test was written against the old signature. Since that test is an insertion into a region this branch never touches, git merges the two without a single conflict and produces a tree that does not compile — a silent break for whoever merges second. The one call is fixed in that commit rather than after it, so no commit here is left unbuildable.A follow-up commit then removes what having both in one tree exposed: eviction and
attachment_cache_sizewere the same query written twice, along with two copies of the answer to "why is this an optional". SQL sums an empty set to NULL, so an empty cache came back as no answer rather than as no bytes;coalescesettles that in the query, where the NULL is produced, and the C++ stops advertising a state that cannot occur.Also here
One commit here fixes a download-cancellation bug that is independent of the rest but collides with it directly.
FileTransferRequest::cancelledwas consulted on every upload path and on none of the streaming download ones, so cancelling a download only stopped us from using what arrived — the rest of the file came down the stream anyway and was dropped a chunk at a time. That is the common case rather than an unusual one: the stream scheme authenticates each chunk as it arrives, so a tampered file is known to be unusable from the bad chunk onwards, and a large attachment can be most of the way through when that happens. Shutdown had the same shape, cancelling its active downloads and then closing with the transfers still running.It collides with this branch because, once cancellation works, a download we abandoned completes with the cancellation we asked for — so the reason we abandoned it has to be preferred over the status. Otherwise a decryption failure reports itself as
download failed with status -10200and the code that says a retry is pointless is lost, which is precisely theunavailablemachinery above.Testing
405 cases pass, clang-format clean, no compiler warnings.
The transition coverage is the part worth looking at: one file shown by two messages, driven through absent → fetching → absent (transient failure) → fetching → cached → absent (eviction) → unavailable (404) → cleared (resend), asserting at each step which message ids were reported and what state they were told, rather than counting events. Separate tests cover a batch spanning two conversations in the promised order, and the upload case where the message reported is in a different conversation from the send.
Two emission sites have no test of their own, deliberately: the reconcile sweep's stale rows and
save_attachment's permanent-failure path. Both are shared calls into paths that are covered, so a test there would pin the call site rather than the behaviour.tests/schema_history_check.shfails upgrading from3df4ae52, on a groupstate/kicked_timestampcheck constraint that84327685did not recreate. That predates this branch and is unrelated to it; the migration here upgrades cleanly fromclient, which the walk now reaches as its first real starting point. CI does not run that script.