From b533e0e5441f13f530ef736f6f69a92e4b4ee677 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 09:54:29 +0000 Subject: [PATCH 1/2] fix(rpc): stop browsers caching deliverable downloads `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 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]) --- raven/rpc/transports/deliverables.py | 14 ++++++++ tests/test_rpc_transports_deliverables.py | 43 +++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/raven/rpc/transports/deliverables.py b/raven/rpc/transports/deliverables.py index 576bce709..f3c9d6ca2 100644 --- a/raven/rpc/transports/deliverables.py +++ b/raven/rpc/transports/deliverables.py @@ -118,6 +118,16 @@ async def download(request: web.Request) -> web.StreamResponse: headers={ "Content-Type": record.media_type, "Content-Disposition": _attachment(record.name), + # `register` reuses the token when a conversation delivers the + # same path again, so a rewritten file keeps its URL -- and the + # delivery card points an straight at this route. Without + # a directive the browser invents a freshness lifetime from + # Last-Modified (about a tenth of the file's age) and answers + # from its own copy without asking, so an image the agent + # replaced went on rendering as the one it replaced. The file + # route and the knowledge route already say this; a token is + # not a content address and must not be cached as one. + "Cache-Control": "no-store", }, ) @@ -134,6 +144,10 @@ async def download_archive(request: web.Request) -> web.StreamResponse: headers={ "Content-Type": "application/zip", "Content-Disposition": _attachment("deliverables.zip"), + # Same reason as the single-file route: a token names a + # delivery, not a fixed set of files, and the members behind it + # can be rewritten between two downloads of the same URL. + "Cache-Control": "no-store", } ) if request.method == "HEAD": diff --git a/tests/test_rpc_transports_deliverables.py b/tests/test_rpc_transports_deliverables.py index 63e9c25a1..7057ec754 100644 --- a/tests/test_rpc_transports_deliverables.py +++ b/tests/test_rpc_transports_deliverables.py @@ -205,6 +205,49 @@ async def test_an_unregistered_route_still_answers_404(client) -> None: assert res.status == 404 +async def test_download_forbids_storing_so_a_rewritten_file_is_fetched_again(client) -> None: + """A deliverable that was rewritten in place must not come back from the + browser cache. + + ``register`` reuses the token when a conversation delivers the same path + again, so a rewritten file keeps the URL it had -- and the delivery card + points an ```` straight at this route. With no ``Cache-Control`` the + browser invents a freshness lifetime from ``Last-Modified`` (about 10% of + the file's age) and answers from its own copy without asking. An image the + agent replaced then went on rendering as the one it replaced, which is + indistinguishable from a server that did not pick the change up. + """ + record = _register(client.store, client.tmp_path) + + res = await client.get("/files/download", params={"token": record.token}) + + assert res.headers["Cache-Control"] == "no-store" + + +async def test_archive_download_forbids_storing_too(client) -> None: + """The same route for a set of files: a token whose member changed must not + be answered from a stored copy either.""" + one = _register(client.store, client.tmp_path, "a.txt", b"AAA") + + res = await client.get("/files/download-archive", params={"token": one.token}) + + assert res.headers["Cache-Control"] == "no-store" + + +async def test_head_carries_the_directive_as_well(client) -> None: + """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.""" + record = _register(client.store, client.tmp_path) + one = _register(client.store, client.tmp_path, "a.txt", b"AAA") + + file_head = await client.head("/files/download", params={"token": record.token}) + archive_head = await client.head("/files/download-archive", params={"token": one.token}) + + assert file_head.headers["Cache-Control"] == "no-store" + assert archive_head.headers["Cache-Control"] == "no-store" + + async def test_resolve_download_drops_stale_entry(tmp_path) -> None: store = DeliverableStore(tmp_path / "deliverables.json") record = _register(store, tmp_path) From 710cb89e1c39bf95c6a4c35ee20209b60ccb0267 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 11:47:28 +0000 Subject: [PATCH 2/2] fix(*): correct the reasons stated for the cache directive 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]) --- raven/rpc/transports/deliverables.py | 12 +++++++++--- tests/test_rpc_transports_deliverables.py | 13 ++++++++----- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/raven/rpc/transports/deliverables.py b/raven/rpc/transports/deliverables.py index f3c9d6ca2..298d44524 100644 --- a/raven/rpc/transports/deliverables.py +++ b/raven/rpc/transports/deliverables.py @@ -124,9 +124,15 @@ async def download(request: web.Request) -> web.StreamResponse: # a directive the browser invents a freshness lifetime from # Last-Modified (about a tenth of the file's age) and answers # from its own copy without asking, so an image the agent - # replaced went on rendering as the one it replaced. The file - # route and the knowledge route already say this; a token is - # not a content address and must not be cached as one. + # replaced went on rendering as the one it replaced. + # `no-cache` would also close that window: `web.FileResponse` + # derives its validator from the file now at this path, so a + # conditional request after a rewrite does get the new bytes. + # `no-store` is chosen 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. This + # route has no size ceiling, unlike those two. "Cache-Control": "no-store", }, ) diff --git a/tests/test_rpc_transports_deliverables.py b/tests/test_rpc_transports_deliverables.py index 7057ec754..a61e6a710 100644 --- a/tests/test_rpc_transports_deliverables.py +++ b/tests/test_rpc_transports_deliverables.py @@ -139,8 +139,9 @@ async def test_control_characters_in_a_filename_are_stripped(client) -> None: async def test_archive_head_agrees_with_get(client) -> None: - """The frontend pre-checks "Download all" with HEAD too, so the branch that - skips the zip build must answer with the same status.""" + """The route is registered with ``allow_head=True``, so a HEAD has to answer + what a GET would. The archive builds its zip only on the GET path, so HEAD + is a second path that could drift from it.""" one = _register(client.store, client.tmp_path, "a.txt", b"AAA") ok = await client.head("/files/download-archive", params={"token": one.token}) @@ -235,9 +236,11 @@ async def test_archive_download_forbids_storing_too(client) -> None: async def test_head_carries_the_directive_as_well(client) -> None: - """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.""" + """The directive has to be on the response HEAD is answered with. + + HEAD never reaches the archive's zip build: it returns through the early + exit before that work starts, so the directive has to sit on the response + the constructor builds rather than be set on the GET path afterwards.""" record = _register(client.store, client.tmp_path) one = _register(client.store, client.tmp_path, "a.txt", b"AAA")