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" + )