Let a caller ask what an attachment transfer is doing by Bilb · Pull Request #166 · session-foundation/libsession-util

Bilb · 2026-09-16T04:11:08Z

Stacked on #165 — merge that first. This branch contains #165's two commits as well as its own, because the base repository's ruleset will not let a branch be created there to point a base at. Review the last three commits, from Let a caller ask what an attachment transfer is doing; the diff shown here collapses to just that once #165 lands.

Nothing about a transfer is stored. A client that was not listening — it had not started yet, the conversation was not open, its process was restarted — cannot learn from the reports what it missed, and asking for the bytes to find out would be a download rather than a question.

attachment_transfers(message_ids) answers nothing doing / running at N of M / already cached, without starting anything. New AttachmentStatus struct, both callback and await_t forms.

Why a page of ids rather than one attachment. That is how a transcript reads: the answer arrives with the messages it describes, so a row never draws a state it is about to correct. It also suits a caller that must not block the loop — one call with one handler rather than thirty, which would otherwise need a fan-in.

cached comes from the cache index, one query for the page rather than a stat per attachment. The index and the files can disagree until a sweep reconciles them, which is accepted deliberately: either way round costs only a fetch that answers differently from what was predicted, and the fetch settles it.

Attachments with nothing to fetch are left out rather than reported idle — an outgoing one that has not uploaded, or an incoming one whose sender gave no url. There is a difference between "nobody is fetching this" and "this is not fetchable", and only the first deserves a row offering to fetch it. An attachment nobody has asked for is reported, with both flags false, for exactly that reason.

Tests: a new case walking the three states — idle before anything asks, transferring while a download is started but unserved, cached once it lands — plus a message nobody has, which is absent rather than reported.

Built and run against the client suite: 135 cases, 1022 assertions, passing.

Independent of #164, which can merge in any order relative to this.

It does not depend on #165 for its behaviour — it takes the conversation from Message::conversation, not from AttachmentProgress. The one thing it needs from #165 is the conversation_id.hpp include in attachment.hpp, since AttachmentStatus carries a ConversationId of its own. Stacked rather than duplicating that include.

Note: tests/static-bundle-test fails to link on client already, before this change (undefined reference to SQLite::Database::exec). Unrelated and untouched.

A report handed to save_attachment's or attachment_data's progress handler carried only the message id, so a caller had to look the message up to find out whether the transfer was for anything it was showing. The conversation is already known at both sites, so the report carries it. callbacks::attachment_progress keeps its leading ConversationId: it is now the same value as the field, but other consumers take it.

It is on the report now, so the parameter said the same thing twice. A handler that files these by conversation needs the value wherever a report comes from, and the one handed to attachment_data or save_attachment has no second argument to carry it - so the struct is the only place that can hold it for both. display_picture_progress keeps its parameter: it has no struct to put one on, which is the same reason it has no message or index.

Nothing about a transfer is stored, so a client that was not listening -- it had not started, the conversation was not open, its process was restarted -- cannot learn from the reports what it missed. Asking for the bytes to find out would be a download rather than a question. Takes a page of message ids rather than one attachment because that is how a transcript reads: the answer arrives with the messages it describes, and a row never draws a state it is about to correct. It also suits a caller that must not block the loop, which is one call with one handler rather than thirty. Cached is read off the file rather than its index row: what decides whether the next fetch answers from disk is the file being there, and the two can disagree until a sweep reconciles them. An attachment with nothing to fetch is left out rather than reported idle -- there is a difference between nobody fetching it and it not being fetchable, and only the first deserves a row offering to fetch it.

It took a full message hydration per message and a second query per attachment: `_message` resolves replies and builds bodies to yield the two fields wanted here, and `_attachment_pointer` then joined `messages` again for a conversation that was thrown away. An attachment whose sender gave no url cost a thrown exception on top. One statement instead, the shape `load_attachments` already uses. `url IS NOT NULL` is the definition of `Attachment::uploaded`, so asking for it in SQL both drops the exception and states the rule once: an attachment nobody could fetch has no row rather than an idle one. Rows are read out before any conversation id is resolved, since resolving one is itself a query and the first is still stepping. The cache check stays a stat per file. What decides whether the next fetch answers from disk is the file being there, and the index can disagree with it until a sweep -- a truthful answer is worth more here than the query it would save.

One query for the page instead of a syscall per attachment. The index and the files can disagree until a sweep reconciles them, which is accepted here: either way round costs a fetch that answers differently from what was predicted -- a row offering to download bytes already present, or one offering to open bytes that turn out to need fetching -- and the fetch is what settles it in both cases. `out` is reserved once the rows are known rather than guessed from the message count, which is wrong in both directions: a message may carry no attachments or several. The cache name is one helper now, since `_in_flight` and `attachment_cache` are keyed by the same hash and the lookup needs it twice.

A 404 went past once, in a progress report, and was then unrecoverable: after a restart a file that is gone looked exactly like one nobody had fetched yet, so the only thing to offer was a download that could not work. `Attachment::unavailable` is set when a download fails in a way that will not come out differently -- 404 or 410, or bytes that arrived and were not what they claimed. A timeout, a negative network code or a server error is worth another go and is deliberately not recorded. On `Attachment` rather than `AttachmentStatus` because it is a fact about the file rather than about what this device is doing: it is true for everyone, it survives a restart, and it therefore belongs with the metadata that travels with the message. Nothing needs a second query to learn it. Written against the url, so two messages quoting one attachment are both marked -- they already share the transfer that discovered it. Never cleared: an attachment url names one upload and is never reissued.

`_record_saved` emits a message update when it writes `saved_at`, and this wrote a field of the same kind without one -- so a transcript already on screen when a background fetch gave up went on offering a download that could not work until something reloaded it. The rows are read before the write, since afterwards nothing matches the predicate, and one already marked is not collected: the report this arrives on repeats, and a repeat has nothing new to announce. While here, answer a page in one pass over its rows. The cache name was hashed twice for each -- once to bind the lookup, once to answer with it -- and the two loops existed only because the lookup has to be built from every name before any row can be answered. Hashing as the rows are read leaves one pass and one hash.

A save reaches `_download_decrypted` directly rather than through `_fetch_cached`, so recording the failure in the latter missed exactly the case a user is most likely to meet: tapping save on a file the server has dropped, having never opened the conversation. It is recorded where both paths meet instead, behind the kind test, so they cannot drift again. `message_attachments.url` gains an index. Both the marking and the page status ask by it, and the primary key is (message, idx), so neither had one. Partial: a row with no url is one nothing looks for. The url-scoping comment claimed more than it did. It answers every message that already quotes the file; one that arrives afterwards is not marked and finds out for itself. Three cases the doc comments asserted and nothing checked: an attachment with no url is left out rather than reported idle, a message with two attachments is answered per index rather than per message, and a page of two messages resolves its conversation once.

Two files for one change, and neither has been in a released database, so a client upgrading applies them together regardless. One step to read.

The hand-rolled step loop was the only one left in this file, and its inner scope existed to close the statement before the update; the range form ends its own temporary and needs no scope.

Three claims that were wider than the code. The report does not always follow the mark: a fetch reports through the loop and sees the row written, a save reports from the network thread and can arrive first, leaving the announcement to settle it. The flag is not true for everyone. It is set on every message quoting the url when the download failed and on no others, so one that arrives later starts false. The page status filters on url but looks rows up by message, so the url index has one user, not two.

The case the marking was moved into _download_decrypted for, and the one a person is most likely to meet: a save never reads what a fetch would have cached, so tapping save on an attachment nobody opened is how a 404 is usually found.

The same claim the header carried: it answers every message already quoting the url and none arriving afterwards. Nor is sharing a transfer the reason it is written that way - a save never joins one, and is marked all the same.

Bilb · 2026-09-16T23:07:07Z

+    // One query for the page, as `load_attachments` does and for the same reason.  What this needs
+    // is a url and a conversation; reaching them a message at a time meant a full hydration --
+    // bodies, replies resolved a level deep -- for two fields, and then a second query per
+    // attachment whose own join it threw away.
+    //
+    // `url IS NOT NULL` is the definition of `Attachment::uploaded`, so it is also the test for
+    // there being anything to fetch: an attachment nobody could fetch is left out rather than
+    // reported idle, and asking in SQL means no row rather than a thrown exception per attachment.
+    auto st = c.prepared_st(
+            R"(
+        SELECT a.message, a.idx, a.url, m.conversation FROM message_attachments a
+        JOIN messages m ON m.id = a.message
+        WHERE a.url IS NOT NULL AND a.message IN ({}) ORDER BY a.message, a.idx
+    )"_format(sqlite::placeholders(message_ids.size())));


I think this won't work for outgoing messages with attachments that are not sent.
i.e. we should be able to display an attachment that is pending upload (for instance we are offline)

Bilb · 2026-09-16T23:08:47Z

+        auto known = conversations.find(row.conversation);
+        if (known == conversations.end())
+            known = conversations.emplace(row.conversation, conversation_id_at(c, row.conversation))
+                            .first;


we should probably throw if we end up loading messages from another conversation

Bilb · 2026-09-16T23:10:08Z

+    -- Written against the url rather than one row because what failed is the file: every message
+    -- already quoting it is answered at once, and one arriving later starts false.
+    unavailable INTEGER NOT NULL DEFAULT 0,


I don't think we should do this for every message attachment that has this url. Instead we should do it for every message attachment that has this url and was not already downloaded

Bilb added 2 commits September 16, 2026 14:02

This was referenced Sep 16, 2026

Say which conversation an attachment transfer belongs to #165

Closed

Carry the conversation on attachment progress, and report cache usage #162

Closed

Bilb force-pushed the feat/attachment-transfer-status branch from 59e72a7 to 104db50 Compare September 16, 2026 06:16

Bilb added 10 commits September 16, 2026 16:52

Fold the url index into the migration that needs it

b1cd5a5

Two files for one change, and neither has been in a released database, so a client upgrading applies them together regardless. One step to read.

Read the rows a failure settles with prepared_results

baa5c7e

The hand-rolled step loop was the only one left in this file, and its inner scope existed to close the statement before the update; the range form ends its own temporary and needs no scope.

Cover a save of a file the server has dropped

76886d5

The case the marking was moved into _download_decrypted for, and the one a person is most likely to meet: a save never reads what a fetch would have cached, so tapping save on an attachment nobody opened is how a 404 is usually found.

Say what the unavailable column settles, in the schema too

74d2e07

The same claim the header carried: it answers every message already quoting the url and none arriving afterwards. Nor is sharing a transfer the reason it is written that way - a save never joins one, and is marked all the same.

Bilb commented Sep 16, 2026

View reviewed changes

Bilb requested a review from jagerman September 16, 2026 23:11

Bilb marked this pull request as draft September 16, 2026 23:11

Navigation Menu

Uh oh!

Let a caller ask what an attachment transfer is doing - #166

Let a caller ask what an attachment transfer is doing#166
Bilb wants to merge 13 commits into
session-foundation:clientfrom
Bilb:feat/attachment-transfer-status

Bilb commented Sep 16, 2026 •

edited

Loading

Uh oh!

Bilb Sep 16, 2026

Uh oh!

Bilb Sep 16, 2026

Uh oh!

Bilb Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Bilb commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bilb Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Bilb Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Bilb Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Bilb commented Sep 16, 2026 •

edited

Loading