Skip to content

Drop attachment captions - #170

Closed
Bilb wants to merge 16 commits into
session-foundation:clientfrom
Bilb:feat/drop-attachment-caption
Closed

Drop attachment captions#170
Bilb wants to merge 16 commits into
session-foundation:clientfrom
Bilb:feat/drop-attachment-caption

Conversation

@Bilb

@Bilb Bilb commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Stacked on #168 ("Attachment availability: say what we hold of a file, and tell everyone showing it"). Merge that one first. The base here stays client because #168's head lives in a fork and GitHub will not accept it as a base branch, so the diff and commit list below also include #168's commits. Only the final commit, Drop attachment captions, belongs to this PR.

caption on an attachment was a pure pass-through with no consumer, so this removes it end to end.

What goes

  • OutgoingAttachment::caption and Attachment::caption (include/session/client/attachment.hpp)
  • the caption TEXT column on message_attachments in full_schema.sql, plus a new migration src/client/schema/006_drop_attachment_caption.sql dropping it from existing databases
  • the four statements in src/client/client.cpp that carried it: the attachment read, the outgoing write, the outgoing protobuf build, and the incoming write
  • optional string caption = 11 in AttachmentPointer
  • the incidental assertions in tests/test_client/attachments.cpp (the tests shrink; none are deleted)

Why it is safe

Nothing reads it. It went protobuf in -> column -> struct field -> column -> protobuf out and nowhere else: no client renders it, nothing searches on it, and it has no index, default or foreign key. The conversation-preview query in client.cpp already excludes it on purpose and says so in a comment.

Removing it also takes four positional statements down by one column each, which is four fewer places for a template parameter list, a ? count and a structured binding to drift apart.

Field 11 is reserved, not freed

AttachmentPointer gains its first reserved block:

reserved 11;
reserved "caption";

Old clients on the network keep sending field 11, so the number must never be reused - anything assigned to it later would parse their captions as its own data. This PR deliberately does not touch any other field number.

Protobuf regeneration

proto/SessionProtos.pb.h, proto/SessionProtos.pb.cc and proto/debug_print.cpp are checked in and are not generated at build time, so they are regenerated here with protoc 3.21.12, the version that produced the existing committed files (WebSocketResources.pb.* regenerates byte-identical, confirming the match). Outside the caption accessors the only churn is the expected _has_bits_ renumbering for the fields after 11.

Verification

On top of #168 (a8a81514):

Each of the four positional statements was re-derived against the post-#168 code rather than replayed, since #168 added unavailable and cached to message_attachments and changed the load query. Recounted by hand afterwards: the attachment read is 12 columns / 12 bindings / 12 template parameters, the outgoing write 8 / 8 / 8, the outgoing protobuf build 8 / 8 / 8, and the incoming write 12 / 12 / 12.

One pre-existing failure further back in the schema-history walk is untouched by this branch: a database from 3df4ae52 (v1.8.0-408) does not reach today's schema because of the group-member state CHECK constraints. It reproduces identically on a pristine client build, so it is unrelated to this change and to #168.

jagerman and others added 16 commits September 17, 2026 20:18
@jagerman
Report whether an attachment's bytes are already here
191a7b3 
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.
@jagerman
Report an attachment's availability changing as a message change
d618ac7 
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.
@jagerman
Key cache filenames, so a directory is not a list of what was downloaded
518b158 
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.
@jagerman
Update session-sqlite
73dc04a 
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.
@jagerman
Index message_attachments by url
9a8e48b 
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.
@jagerman
Report a file that cannot be fetched as unavailable
69fc150 
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.
@jagerman
Report how far an attachment's transfer has got
4cf479b 
`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.
@jagerman
Keep a copy of the files we send
8360b80 
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.
@jagerman
Reference an attachment's cached copy instead of recomputing it
a73c3a0 
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.
@jagerman
Report every change to what we hold of an attachment
51a2e01 
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.
@jagerman
Report messages in batches rather than one at a time
f772848 
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.
@jagerman
Stop a download its caller has given up on
bcb8602 
`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.
@Bilb @jagerman
Report how full the attachment cache is
7c7754c 
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.
@jagerman
Ask the cache its size in one place
2829ac0 
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.
@jagerman
Collapse the attachment migrations into one
a8a8151 
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.
@Bilb
Drop attachment captions
7282e48 
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.
@Bilb
Bilb force-pushed the feat/drop-attachment-caption branch from 4034def to 7282e48 Compare September 21, 2026 08:20
@Bilb

Bilb commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Reopened against the attachment-availability branch in jagerman's fork, where the branch this depends on actually lives: jagerman#2

Targeting client here meant the diff carried #168's sixteen commits alongside the one this PR is about. Against the real base it is one commit and nine files.

@Bilb Bilb closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Bilb @jagerman