fix(rpc): stop browsers caching deliverable downloads - #838
Conversation
`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>
gloryfromca
left a comment
There was a problem hiding this comment.
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.
|
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.
Both halves came apart when I went looking for them. "both routes" -- nothing on the browser side names the archive route at all. "would let the browser store the copy it is about to fetch" -- the two HEAD Half of this predates you: Keep the test. I checked whether it was a literal mirror of the line two One measurement, offered as information and not as a request. The commit message So I am not asking you to change it. Reviewed at head |
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
left a comment
There was a problem hiding this comment.
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.
|
Both points reproduced, both held, both fixed in The docstrings. I ran your two checks plus the positive control: So the archive route has no frontend caller at all -- no "Download all" anywhere -- and Confirmed the pre-existing half as well: Both docstrings now state what the tests actually pin: the route is registered
Same result. The validator tracks the rewrite, so I am keeping The description is rewritten accordingly and now carries a Review round section recording |
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/downloadsent noCache-Control, so the browser fell back to heuristic freshness (RFC 9111section 4.2.2) and answered from its own copy without asking the server.
Three facts together make that user-visible:
DeliverableStore.registerreuses the token when a conversation deliversthe same path again, so a rewritten file keeps the URL it had. That reuse
is deliberate: it refreshes the reported size.
<img>straight at this route, so for animage or SVG deliverable the route is not a download link but the pixel
source.
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-storeto both download routes.Why
no-storerather thanno-cacheBoth close the bug.
web.FileResponsederives its validator from the filecurrently at
record.path, so a conditional request after a rewrite doesget the new bytes;
no-cachewould have fixed the staleness too, and wouldcost nothing on the unchanged case. An earlier draft of this description
argued otherwise and was wrong.
no-storeis kept for the two reasons that do separate them:/fileand/knowledge/file, which serve the same kind offile and already send it;
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 paintedfull-resolution into an
<img>on every tile mount, so an unchanged file isre-sent on each view. Moving to
no-cachelater is a one-line change ifthat traffic ever matters.
Deliberately unchanged: the token reuse in
register. Changing it wouldinvalidate 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
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:
Before the fix the same probe read
hits +0andtransferSize 0: theserver 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.
The validator measurement behind the
no-cachenote above, at the currenthead:
Tests were written first and confirmed red, then green:
Commands and results at the pushed head:
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.
The three tests are appended to the existing
tests/test_rpc_transports_deliverables.pyrather than a new file.Review round
0xKTread the diff and reported one wrong reason, no request to changebehaviour. Both of his points were reproduced and both held:
in
ui-webnames the archive route at all, so no frontend pre-checks itwith HEAD; and the HEAD probes that do reach the single-file route already
pass
cache: 'no-store'as a request option, so the response directivechanges 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.
no-storerationale was wrong, as described above.gloryfromcareviewed at the same head and filed no blockers: 19 passed forthe changed file, 66 for
test_rpc_files.pyplus the delivery smoke test.Risk
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-TypeorContent-Dispositionchanged, 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