Skip to content

fix(rpc): stop browsers caching deliverable downloads - #838

Merged
0xKT merged 2 commits into
mainfrom
fix/deliverable_download_cache_headers
Oct 2, 2026
Merged

0xKT merged 2 commits into
mainfrom
fix/deliverable_download_cache_headers

Conversation

@LivXue

@LivXue LivXue commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Summary

A delivered file that the agent rewrote in place went on rendering in the
web UI as the file it replaced, until the browser cache expired or the
reader cleared it.

The cause is one missing response header. /files/download sent no
Cache-Control, so the browser fell back to heuristic freshness (RFC 9111
section 4.2.2) and answered from its own copy without asking the server.
Three facts together make that user-visible:

  • DeliverableStore.register reuses the token when a conversation delivers
    the same path again, so a rewritten file keeps the URL it had. That reuse
    is deliberate: it refreshes the reported size.
  • The delivery card points an <img> straight at this route, so for an
    image or SVG deliverable the route is not a download link but the pixel
    source.
  • The shortcut is silent. Nothing errors and no test reddens.

The heuristic lifetime is about a tenth of the file's age at the moment the
browser stores the response, which is why this reads as intermittent: zero
for a file written moments ago, minutes for one that sat on disk before the
reader first looked at it.

This adds Cache-Control: no-store to both download routes.

Why no-store rather than no-cache

Both close the bug. web.FileResponse derives its validator from the file
currently at record.path, so a conditional request after a rewrite does
get the new bytes; no-cache would have fixed the staleness too, and would
cost nothing on the unchanged case. An earlier draft of this description
argued otherwise and was wrong.

no-store is kept for the two reasons that do separate them:

  • it matches /file and /knowledge/file, which serve the same kind of
    file and already send it;
  • it is the only directive that keeps an agent's output out of the reader's
    disk cache.

The cost is real and worth stating: this route has no size ceiling, unlike
those two (they cap at MAX_VIEW_BYTES), and a delivered image is painted
full-resolution into an <img> on every tile mount, so an unchanged file is
re-sent on each view. Moving to no-cache later is a one-line change if
that traffic ever matters.

Deliberately unchanged: the token reuse in register. Changing it would
invalidate links already handed out, and the refresh-the-size behaviour is
the reason it exists.

The archive route takes the same directive for the same reason. Its HEAD is
answered by the early exit that skips the zip build, so the directive has to
sit on the response the constructor builds rather than be set on the GET
path afterwards; a test pins that placement.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Reproduced before fixing, in real Chromium against the real route, with the
server's own request count and the browser's Resource Timing as two
independent signals. At a one-hour file age, after rewriting the file with
the same path and name:

view 1: hits = 1  pixel = [255, 0, 0]   (red = old image)
rewrote in place; token reused: True
view 2: hits = 2  pixel = [0, 0, 255]   (blue = new image)
VERDICT: FIXED -- new image on screen, route asked again

Before the fix the same probe read hits +0 and transferSize 0: the
server was never asked and the old bytes were served from disk cache while
the file on disk held different bytes.

A control run isolates the header as the cause: same file, same age, same
browser, two routes differing only in whether the directive is sent.

first view   bare: [255,0,0]   fixed: [255,0,0]
second view  bare: [255,0,0]   fixed: [0,0,255]
server asked  bare: 0          fixed: 1

The validator measurement behind the no-cache note above, at the current
head:

1st GET      status=200 Etag="18dab27e58015863-78" CC='no-store'
conditional  status=304
token reused across the rewrite: True
after rewrite status=200 Etag="18dab27e9a95fd7b-78" moved=True bytes=120

Tests were written first and confirmed red, then green:

3 failed in 8.13s
E       KeyError: 'Cache-Control'

Commands and results at the pushed head:

uv run --frozen --python 3.12 --all-extras pytest \
  tests/test_rpc_transports_deliverables.py tests/test_rpc_files.py \
  tests/test_deliverable_store.py tests/test_deliver_files_tool.py \
  tests/integration/test_file_delivery_smoke.py -q
  118 passed in 8.56s

python scripts/check_source_language.py HEAD   exit 0
python scripts/check_large_files.py HEAD       exit 0
python scripts/check_commit_messages.py origin/main..HEAD   exit 0
ruff check <both files>                        All checks passed

The source-language gate was proven live before its green was trusted: a
deliberate CJK line was added, the gate named the file and line, and the
probe was then removed.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

The three tests are appended to the existing
tests/test_rpc_transports_deliverables.py rather than a new file.

Review round

0xKT read the diff and reported one wrong reason, no request to change
behaviour. Both of his points were reproduced and both held:

  • Two docstrings stated frontend behaviour this tree does not have. Nothing
    in ui-web names the archive route at all, so no frontend pre-checks it
    with HEAD; and the HEAD probes that do reach the single-file route already
    pass cache: 'no-store' as a request option, so the response directive
    changes nothing for them. One of the two docstrings predates this branch
    (the base carries the same false "Download all" claim) and is corrected
    here as well, since it sits directly above the changed line. The tests
    themselves stay: he checked and they are not mirrors, and one of them pins
    the archive header's placement, which no other test does.
  • The stated no-store rationale was wrong, as described above.

gloryfromca reviewed at the same head and filed no blockers: 19 passed for
the changed file, 66 for test_rpc_files.py plus the delivery smoke test.

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Security: this only reduces what is written to disk and held in a browser
cache, on a route whose whole trust boundary is that a path never travels in
the URL. No auth or origin check changed.

Backward compatibility: responses to these routes now carry one additional
header. No status code, body, Content-Type or Content-Disposition
changed, and the frontend's HEAD pre-checks read status only.

Rollback: revert the commit. Browsers holding a stored copy will keep
serving it until it expires, so a revert restores the old behaviour
gradually rather than at once.

Related Issues

N/A

`register` reuses the token when a conversation delivers the same path
again, so a file rewritten in place keeps the URL it had. The delivery
card points an <img> straight at `/files/download`, and that route sent no
Cache-Control at all.

With no directive the browser invents a freshness lifetime from
Last-Modified, about a tenth of the file's age, and answers inside that
window from its own copy without asking the server. The window is zero for
a file written moments ago and opens as the file ages, so a delivered image
the agent replaced went on rendering as the one it replaced -- most visible
on a file that had been on disk for a while before the reader first looked
at it, and indistinguishable from a gateway that never picked the change up.

Confirmed in Chromium against the real route: at a one-hour file age the
second view reported transferSize 0 and never reached the server, while the
disk held different bytes. Adding the directive makes the route answer again
and the new image renders.

`no-store` rather than `no-cache`, matching `/file` and `/knowledge/file`,
which both serve the same kind of file and already say this. A token names a
delivery, not fixed bytes, so it is not a content address and must not be
cached as one -- revalidation would still be a claim the URL identifies the
content, and the token is deliberately reused to refresh the reported size.
The tradeoff is that a large image is re-sent on every view.

The archive route gets the same directive for the same reason: its members
can be rewritten between two downloads of one URL. HEAD responses carry it
too, because the frontend pre-checks both routes that way, and a HEAD that
omitted it would let the browser store the copy it is about to fetch.

Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
@LivXue
LivXue requested review from 0xKT and gloryfromca October 2, 2026 09:56

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the repository rules and runtime context, the complete main...HEAD diff, both download handlers, token reuse in DeliverableStore, web callers, adjacent file-serving cache policy, and the relevant file history. I also checked backward compatibility, test integrity, and the RPC/UI architecture boundary. The new no-store policy matches the mutable-token contract, covers both single-file and archive GET/HEAD responses, changes no URL or response-body contract, and the tests add coverage without weakening existing assertions.

Verification: uv run pytest tests/test_rpc_transports_deliverables.py -x (19 passed); uv run pytest tests/test_rpc_files.py tests/integration/test_file_delivery_smoke.py -x (66 passed). The first focused attempt stopped during repository-wide fixture setup because the default environment lacked raven_everos; after uv sync --extra dev installed the repository's declared test plugins, the unchanged test command passed. git diff --check also passed.

@0xKT

0xKT commented Oct 2, 2026

Copy link
Copy Markdown
Member

Not a blocker -- the change is right and I am not asking for it to move. One

Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
docstring in the new tests states a frontend behaviour this tree does not have,
and in a repo where the comment is the documentation that is worth correcting
before it is trusted.

tests/test_rpc_transports_deliverables.py:237-240,
test_head_carries_the_directive_as_well:

The frontend pre-checks both routes with HEAD, and a HEAD response that
omitted the directive would let the browser store the copy it is about to
fetch.

Both halves came apart when I went looking for them.

"both routes" -- nothing on the browser side names the archive route at all.
Positive control first, because an empty search proves nothing on its own:
git grep -c 'files/download' -- 'ui-web/src' finds 3 files, so the query works
on this tree; git grep -rn 'download-archive|downloadArchive|download_archive' -- ui-web ui-tui then exits 1 with no hits, and over the whole tree outside
tests/ the only line left is raven/rpc/transports/deliverables.py:178, the
registration itself. The route has no in-repo consumer, so it is not pre-checked
with HEAD by anybody.

"would let the browser store the copy it is about to fetch" -- the two HEAD
probes that do hit /files/download are both issued as
fetch(url, { method: 'HEAD', credentials: 'same-origin', cache: 'no-store' })
(ui-web/src/features/transcript/TranscriptPage.tsx:1324 and :1356). The
request option already forbids storing, so the response directive changes
nothing for those two callers; and a HEAD response carries no body to store in
the first place.

Half of this predates you: git show e84152c5:tests/test_rpc_transports_deliverables.py
already says "The frontend pre-checks "Download all" with HEAD too" at line 142.
Line 238 restates it rather than inventing it.

Keep the test. I checked whether it was a literal mirror of the line two
above it and it is not. Control mutant, both literals to no-cache:
3 failed, 16 passed -- all three new tests reach the code. Then the pointed
one: move the archive header out of the StreamResponse(...) constructor to
response.headers["Cache-Control"] = "no-store" placed after the
if request.method == "HEAD": ... return response block -> 1 failed, 18 passed,
the single red being test_head_carries_the_directive_as_well with
KeyError: 'Cache-Control', while test_archive_download_forbids_storing_too
stayed green. It pins a placement property no other test pins. Only the stated
reason is wrong; a true one is right there.


One measurement, offered as information and not as a request. The commit message
argues no-store over no-cache because "a token names a delivery, not fixed
bytes ... revalidation would still be a claim the URL identifies the content".
On this route that premise does not hold: web.FileResponse derives its
validator from the file currently at record.path, so revalidation tracks the
rewrite correctly. Measured on b533e0e5:

1st GET       status=200 Etag="18dab1343c18efa9-40" Last-Modified='Fri, 02 Oct 2026 10:57:20 GMT' CC='no-store'
conditional   status=304                                                                           CC='no-store'
token reused across the rewrite: True
after rewrite status=200 Etag="18dab1343c44efa4-64" moved=True bytes=100

So no-cache would also have fixed the staleness, and at zero bytes on the
unchanged case -- which matters more here than on the view routes, because
/file and /knowledge/file cap at MAX_VIEW_BYTES and this route has no
ceiling, while a delivered image is painted full-resolution into an <img>
(TranscriptPage.tsx:1309) on every tile mount.

I am not asking you to change it. no-store is strictly safer than the bug, it
matches the two nearest siblings, it is the only directive that keeps an agent's
output off the reader's disk, and no rule in this repo binds the choice. It is
your call; I only think the stated reason should not be the one that survives
into main, since the PR body becomes the squash commit body.

Reviewed at head b533e0e5 onto tip e84152c5.

Review measured both of the reasons this branch gave, and both were wrong.

`no-cache` would also have closed the staleness window: web.FileResponse
derives its validator from the file now at record.path, so a conditional
request after a rewrite does get the new bytes. The source comment and the
description argued the opposite. `no-store` stays for the two reasons that
do separate them -- it matches /file and /knowledge/file, which serve the
same kind of file, and it is the only directive that keeps an agent's output
out of the reader's disk cache. The cost is stated too: this route has no
size ceiling, unlike those two.

Two test docstrings claimed frontend behaviour this tree does not have.
Nothing in ui-web names the archive route, so no frontend pre-checks it with
HEAD; and the HEAD probes that do reach the single-file route already pass
cache: 'no-store' as a request option, so the response directive changes
nothing for them. One of the two predates this branch, which carried the
same false "Download all" claim, and is corrected here because it sits
directly above the changed line.

No behaviour changes: both routes still send no-store, and no test logic
moved. The archive's HEAD test is kept -- it pins the directive's placement
on the constructed response, which the early exit makes observable and which
no other test covers.

Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the delta from b533e0e and rechecked the resulting full diff against AGENTS.md/CLAUDE.md and the runtime context, relevant callers and history, backward compatibility, test integrity, and the RPC/UI boundary. The new commit correctly removes the inaccurate frontend-HEAD claims, retains the distinct archive HEAD-path coverage, and replaces the overstated cache rationale with one consistent with aiohttp's file validators. It changes neither runtime behavior nor test assertions.

Verification: uv run pytest tests/test_rpc_transports_deliverables.py -x (19 passed); git diff --check passed.

@LivXue

LivXue commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

Both points reproduced, both held, both fixed in 710cb89e. Thank you for measuring
rather than asserting; the second one corrected a rationale I had already written into
the description, so it would have reached main.

The docstrings. I ran your two checks plus the positive control:

git grep -rc 'files/download' -- ui-web/src
  TranscriptPage.test.tsx:1  WorkspacePage.test.tsx:5  DeskPalette.test.tsx:1   (query works)
git grep -rn 'download-archive|downloadArchive|download_archive' -- ui-web ui-tui
  exit 1, no hits

So the archive route has no frontend caller at all -- no "Download all" anywhere -- and
"both routes" was wrong. On the second half you are right too: the three probes that do
reach /files/download all pass cache: 'no-store' as a request option
(TranscriptPage.tsx:1325, :1356, store.ts:369), so the response directive changes
nothing for them.

Confirmed the pre-existing half as well: git show e84152c5:tests/test_rpc_transports_deliverables.py
carries the same false "Download all" claim. It sits directly above the line I changed,
so I corrected it in the same commit rather than leaving a known-false comment adjacent to
a corrected one. Flagging it here explicitly since it is not mine to have introduced.

Both docstrings now state what the tests actually pin: the route is registered
allow_head=True, and HEAD is a second path -- the archive answers it through the early
exit that skips the zip build, which is why the directive has to sit on the constructed
response. Your mutant (moving the header assignment after the early return) is exactly the
property the kept test pins, and no other test covers it, so it stays.

no-store vs no-cache. I reproduced your measurement independently:

1st GET      status=200 Etag="18dab27e58015863-78" CC='no-store'
conditional  status=304
token reused across the rewrite: True
after rewrite status=200 Etag="18dab27e9a95fd7b-78" moved=True bytes=120

Same result. The validator tracks the rewrite, so no-cache would have fixed the staleness
too -- my stated reason was wrong, and it was the one that would have survived into the
squash body.

I am keeping no-store, for the reasons you gave plus one I checked: it is the only
directive that keeps an agent's output out of the reader's disk cache. The cost is now
stated in the source comment and in the description, including that this route has no
MAX_VIEW_BYTES ceiling where /file and /knowledge/file both do, and that moving to
no-cache later is a one-line change if that traffic ever matters.

The description is rewritten accordingly and now carries a Review round section recording
what you found. Reviewed at the current head 710cb89e (two commits above the one you
read: the fix is comments and one docstring, no behaviour change -- both routes still send
no-store).

@0xKT
0xKT merged commit 23d9bba into main Oct 2, 2026
26 checks passed
@0xKT
0xKT deleted the fix/deliverable_download_cache_headers branch October 2, 2026 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants