From 5723e2349f6814d831a9d72b37082f45cd3db33d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 18:25:55 +0000 Subject: [PATCH] Adversarial review: a cached summary still fetched the reader's identifiers The cache was looked up *after* the disclosure fetch, so every reload of an already-summarised analysis went to the Analysis Service for the reader's unmatched identifiers and then discarded them. Not a disclosure beyond what they chose, but a pointless request on the one endpoint where pointless requests are worth avoiding -- and it meant the cache did nothing for the path it should protect most. The lookup now precedes the fetch, under the tier the reader *asked* for, while the write still uses the tier that *applied*. The asymmetry is deliberate and was worth working out rather than assuming: a request whose disclosure failed stores an aggregate summary under `aggregate`, so the next disclosing request misses and gets another attempt at the identifiers, instead of being served the fallback forever. Both directions are now tested. One thing the review confirmed rather than changed, and it is worth writing down because it looks like waste. The analysis result is fetched on every request, before the cache is consulted, and that is what keeps `gone` correct: a release deletes the analysis, and a reader must be told to re-run rather than handed a confident summary of something that no longer exists. A cache that skipped the fetch would serve stale summaries of deleted results indefinitely. Now pinned by a test. Verified by sabotage, with the sabotage asserted to have applied. Co-Authored-By: Claude Opus 5 --- src/api/analysis_summary.py | 50 +++++++++++++++---------- tests/api/test_analysis_summary.py | 60 ++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 19 deletions(-) diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index 006610e..b47ea2d 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -148,37 +148,49 @@ async def stream() -> AsyncIterator[str]: payload = for_tier(fetched.result, body.disclosure) model_input = prompt_input(payload) - # Which tier the answer was actually built from, which is - # not always the one asked for. + release = await current_release() + + # Looked up BEFORE the disclosure fetch, and under the tier + # the reader *asked* for. The first version looked it up + # after, so a cache hit on the disclosing tier still went and + # fetched the reader's identifiers and then discarded them -- + # a pointless request to the endpoint that returns their data, + # on every reload of a summary we already had. + # + # Stored under the tier that *applied*, which is not always + # the same. The asymmetry is deliberate: a request whose + # disclosure failed stores an aggregate summary under + # `aggregate`, so the next disclosing request misses and gets + # another chance at the identifiers rather than being served + # the fallback forever. + cached = ( + _store.get(body.token, release, body.disclosure) + if release + else None + ) + + # Which tier the answer was actually built from. applied: Tier = body.disclosure - if body.disclosure == "identifiers": + if cached is None and body.disclosure == "identifiers": # The only place this service asks for the reader's own - # identifiers, reached only because they chose it. A - # separate call taking a separate decision, never a flag - # with a default, and never on the aggregate path. + # identifiers, reached only because they chose it and + # only when there is nothing to reuse. A separate call + # taking a separate decision, never a flag with a + # default, and never on the aggregate path. unmatched = await fetch_not_found(body.token) if unmatched: model_input["identifiers_not_found_names"] = unmatched elif model_input.get("identifiers_not_found"): # There were unmatched identifiers and we could not - # retrieve them. The summary is therefore the - # aggregate one, and saying so is the whole point: - # this is the same "disclosure with no benefit" the - # tier was just fixed for, arriving down the failure - # path instead. A reader who chose to disclose and - # silently got the other summary has been told - # nothing and given nothing. + # retrieve them, so the summary is the aggregate one. + # Saying so is the point: a reader who chose to + # disclose and silently got the other summary has + # been told nothing and given nothing. applied = "aggregate" logger.warning( "identifier tier requested but the not-found " "lookup returned nothing; serving aggregate" ) - release = await current_release() - # Keyed on the tier that *applied*, not the one requested: a - # disclosure that could not be honoured produced an aggregate - # summary, and storing it under `identifiers` would serve it - # back later as though the identifiers had been used. - cached = _store.get(body.token, release, applied) if release else None yield _sse( "start", { diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 86e202b..2fbc516 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -547,3 +547,63 @@ def cited(response: Any) -> list[str]: assert cited(second) == cited(first) assert cited(second), "no citations at all" + + +def test_a_cached_disclosing_summary_does_not_refetch_the_identifiers( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # The first version looked the cache up *after* the disclosure fetch, so + # every reload of an already-summarised analysis went and asked the + # Analysis Service for the reader's identifiers again and discarded + # them. Pointless, and on the one endpoint where pointless requests are + # worth avoiding. + asked = _with_not_found_spy(monkeypatch) + private, public = keys + _post(public, caller_token=_token(private), disclosure="identifiers") + assert len(asked) == 1 + second = _post(public, caller_token=_token(private), disclosure="identifiers") + assert _events(second.text)[0][1]["cached"] is True + assert len(asked) == 1, "a cached summary refetched the user's identifiers" + + +def test_a_failed_disclosure_does_not_poison_later_requests( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # A request whose disclosure failed stores an aggregate summary under + # `aggregate`. The next disclosing request must miss and try again, + # rather than being served that fallback forever -- which is why the + # lookup uses the requested tier and the write uses the applied one. + failing = {"on": True} + + async def _sometimes(_token: str, **_kwargs: Any) -> list[str] | None: + return None if failing["on"] else ["SMITH_LAB_SECRET_GENE_001"] + + monkeypatch.setattr("api.analysis_summary.fetch_not_found", _sometimes) + private, public = keys + first = _post(public, caller_token=_token(private), disclosure="identifiers") + assert _events(first.text)[0][1]["disclosure"] == "aggregate" + + failing["on"] = False + second = _post(public, caller_token=_token(private), disclosure="identifiers") + assert _events(second.text)[0][1]["cached"] is False, "served the fallback" + assert _events(second.text)[0][1]["disclosure"] == "identifiers" + + +def test_a_deleted_result_is_reported_even_when_a_summary_is_stored( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # The result is fetched on every request, before the cache is consulted, + # and that is what keeps `gone` correct: a release deletes the analysis, + # and a reader must be told to re-run rather than handed a confident + # summary of something that no longer exists. + private, public = keys + _post(public, caller_token=_token(private)) + + async def _gone(_token: str) -> Fetched: + return Fetched("gone") + + monkeypatch.setattr("api.analysis_summary.fetch_result", _gone) + assert ( + _events(_post(public, caller_token=_token(private)).text)[-1][1]["state"] + == "gone" + )