Adversarial review: a cached summary still fetched the reader's identifiers

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",
                     {

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