Skip to content

Commit f9d8b65

Browse files
FlyM1ssclaude
andcommitted
fix(api): signals endpoint conditional-GET correctness + fail-closed gaps
- Last-Modified only on the unfiltered variant: generated_at is identical across tickers variants of one artifact, so a client revalidating a filtered request with If-Modified-Since alone (no If-None-Match — legal, and Django then never consults the ETag) got a 304 pointing at a differently-filtered cached body - ETag keyed on the normalized (deduped/uppercased/sorted) tickers filter shared with the view, each token percent-encoded, '+'-joined: the raw query string made equivalent filters miss revalidation; a raw comma in the ETag is rejected outright by Django's parse_etags (latent pre-fix bug — multi-ticker conditional GET could never 304); and an unencoded '+' join collides with a literal %2B token (adversarial-review catch, shared-cache poisoning vector under Cache-Control: public) - _get_artifact: one disk load per request instead of three (@condition calls etag/last_modified funcs before the view; glob+stat+read+parse ran 3x per GET); sorted() of candidates dropped — max() re-derives order - fail-closed holes in the "never a 500" contract: p.stat() inside the try (file pruned between glob and stat -> 404, was an uncaught FileNotFoundError), and tz-naive generated_at rejected up front (was a TypeError at staleness subtraction) Tests: 7 new endpoint tests, TDD RED-first (variant Last-Modified, IMS-only revalidation, ETag normalization + %2B collision, single-load memo, vanish race, naive timestamp). Endpoint+contract: 19 passed. Spec §4.4 amended. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011vVEHtvXuM84SrcjhTVLce
1 parent c4d3134 commit f9d8b65

3 files changed

Lines changed: 154 additions & 17 deletions

File tree

Docs/superpowers/specs/2026-07-06-news-to-signals-pipeline-design.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ A machine-readable JSON Schema ships at `Heartbeat/schemas/signals-v1.schema.jso
160160
- **200** — public serialization of the newest `signals-*.json` (newest = greatest mtime, filename as a deterministic tiebreak; same-day supplemental stems sort lexicographically before the date-only stem, so stem order alone is not recency): the artifact **minus** `generator`, `model`, `prompt_version` (recon-value stripping), **plus** `"staleness_hours"` computed server-side from `generated_at`.
161161
- `?tickers=AAPL,MSFT` — optional filter on `signals` keys (case-insensitive; unknown tickers simply absent).
162162
- **404** `{"error": "no_signals"}` — no artifact exists.
163-
- Headers: `ETag` (from `generated_at`), `Last-Modified`, `Cache-Control: public, max-age=300`; conditional GET returns 304. Rate-limited via the existing `django_ratelimit` + `api.identity.ratelimit_key` infra.
163+
- Headers: `ETag` (from `generated_at` + `source_items` + the normalized tickers filter, `+`-joined — Django's `parse_etags()` rejects commas inside an ETag), `Last-Modified` **on the unfiltered variant only** (it is identical across tickers variants, so an `If-Modified-Since`-only revalidation of a filtered request must get a full 200, never a 304 pointing at a differently-filtered cached body), `Cache-Control: public, max-age=300`; conditional GET returns 304. Rate-limited via the existing `django_ratelimit` + `api.identity.ratelimit_key` infra.
164164
- Serving path: container gets a **runtime-enforced `:ro` mount of `$HEARTBEAT_HOME/signals/` only** — never the whole digests tree (§7.2).
165165

166166
### 4.5 ATL adapter projection (future session; pinned now)

Main/backend/api/signals_views.py

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import logging
1414
from datetime import datetime, timezone
1515
from pathlib import Path
16+
from urllib.parse import quote
1617

1718
from django.conf import settings
1819
from django.http import HttpRequest, JsonResponse
@@ -33,36 +34,71 @@ def _load_latest():
3334
directory = Path(configured)
3435
if not directory.is_dir():
3536
return None
36-
candidates = sorted(directory.glob("signals-*.json"))
37+
candidates = list(directory.glob("signals-*.json"))
3738
if not candidates:
3839
return None
39-
# Newest by mtime, filename as a deterministic tiebreak: same-day
40-
# supplemental stems (items-<date>-<HHMMSS>.jsonl ->
41-
# signals-<date>-<HHMMSS>.json) sort lexicographically BEFORE the
42-
# date-only stem ("." > "-" in ASCII), so stem order alone is not
43-
# recency — a same-day re-run would otherwise serve the stale artifact.
44-
newest = max(candidates, key=lambda p: (p.stat().st_mtime, p.name))
40+
newest = None
4541
try:
42+
# Newest by mtime, filename as a deterministic tiebreak: same-day
43+
# supplemental stems (items-<date>-<HHMMSS>.jsonl ->
44+
# signals-<date>-<HHMMSS>.json) sort lexicographically BEFORE the
45+
# date-only stem ("." > "-" in ASCII), so stem order alone is not
46+
# recency — a same-day re-run would otherwise serve the stale
47+
# artifact. stat() stays inside the try: a file pruned between
48+
# glob() and stat() fails closed, never 500s.
49+
newest = max(candidates, key=lambda p: (p.stat().st_mtime, p.name))
4650
artifact = json.loads(newest.read_text(encoding="utf-8"))
47-
datetime.fromisoformat(artifact["generated_at"]) # must parse
51+
generated = datetime.fromisoformat(artifact["generated_at"])
52+
if generated.tzinfo is None:
53+
# fromisoformat accepts naive strings; the view subtracts this
54+
# from an aware now() for staleness_hours
55+
raise ValueError("generated_at must be timezone-aware")
4856
if not isinstance(artifact["signals"], dict):
4957
raise ValueError("signals must be a JSON object")
5058
return artifact
5159
except (OSError, ValueError, KeyError, TypeError) as exc:
52-
logger.error("signals: unreadable artifact %s: %s", newest.name, exc)
60+
logger.error("signals: unreadable artifact %s: %s",
61+
newest.name if newest else "<vanished>", exc)
5362
return None # fail closed: unreadable == no signals
5463

5564

65+
def _get_artifact(request: HttpRequest):
66+
"""One disk load per request: @condition calls _etag and _last_modified
67+
before the view body runs, and all three need the artifact."""
68+
if not hasattr(request, "_signals_artifact"):
69+
request._signals_artifact = _load_latest()
70+
return request._signals_artifact
71+
72+
73+
def _tickers_filter(request: HttpRequest):
74+
"""Normalized tickers filter (deduped, uppercased, sorted) — the single
75+
definition shared by the ETag variant key and the view's filtering."""
76+
raw = request.GET.get("tickers") or ""
77+
return sorted({t.strip().upper() for t in raw.split(",") if t.strip()})
78+
79+
5680
def _etag(request: HttpRequest):
57-
artifact = _load_latest()
81+
artifact = _get_artifact(request)
5882
if artifact is None:
5983
return None
60-
tickers = (request.GET.get("tickers") or "").upper().replace(" ", "")
84+
# Each token percent-encoded so the "+" join is unambiguous (a literal
85+
# "+" inside a token — reachable via %2B — must not collide with the
86+
# separator), and "+"-joined rather than ","-joined because Django's
87+
# parse_etags() rejects an ETag containing a comma (HTTP list separator).
88+
tickers = "+".join(quote(t, safe="") for t in _tickers_filter(request))
6189
return f'"{artifact["generated_at"]}|{artifact.get("source_items", "")}|{tickers}"'
6290

6391

6492
def _last_modified(request: HttpRequest):
65-
artifact = _load_latest()
93+
# Only the unfiltered variant carries Last-Modified: generated_at is
94+
# identical across every tickers variant of one artifact, and a client
95+
# revalidating a filtered request with If-Modified-Since alone (legal —
96+
# and without If-None-Match Django never consults the ETag) would get a
97+
# 304 telling it to reuse a differently-filtered cached body. The ETag
98+
# is the only validator that can carry the variant.
99+
if _tickers_filter(request):
100+
return None
101+
artifact = _get_artifact(request)
66102
return (datetime.fromisoformat(artifact["generated_at"])
67103
if artifact else None)
68104

@@ -73,17 +109,16 @@ def _last_modified(request: HttpRequest):
73109
method=ALL, block=True)
74110
@condition(etag_func=_etag, last_modified_func=_last_modified)
75111
def news_signals(request: HttpRequest) -> JsonResponse:
76-
artifact = _load_latest()
112+
artifact = _get_artifact(request)
77113
if artifact is None:
78114
return JsonResponse({'error': 'no_signals'}, status=404)
79115
body = {k: v for k, v in artifact.items() if k not in _PUBLIC_STRIP}
80116
generated = datetime.fromisoformat(artifact["generated_at"])
81117
now = datetime.now(timezone.utc)
82118
body["staleness_hours"] = round(
83119
(now - generated).total_seconds() / 3600, 1)
84-
raw = request.GET.get("tickers")
85-
if raw:
86-
wanted = {t.strip().upper() for t in raw.split(",") if t.strip()}
120+
wanted = set(_tickers_filter(request))
121+
if wanted:
87122
body["signals"] = {k: v for k, v in body["signals"].items()
88123
if k in wanted}
89124
response = JsonResponse(body)

Main/backend/tests/test_signals_endpoint.py

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
import time
88
from datetime import datetime, timedelta, timezone
99
from pathlib import Path
10+
from unittest import mock
1011

1112
from django.core.cache import cache
1213
from django.test import SimpleTestCase, override_settings
1314

15+
from api import signals_views
16+
1417
URL = "/api/signals/news/"
1518

1619
# Bare pytest (no Django test runner, no pytest-django) never calls
@@ -146,6 +149,105 @@ def test_etag_is_variant_specific_to_tickers_filter(self):
146149
HTTP_IF_NONE_MATCH=filtered_etag)
147150
self.assertEqual(repeat.status_code, 304)
148151

152+
def test_last_modified_only_on_unfiltered_variant(self):
153+
# Last-Modified (generated_at) is identical across every tickers
154+
# variant of one artifact, so it can only be emitted where it
155+
# uniquely identifies the variant: the unfiltered response.
156+
self._write("2026-07-06", make_artifact(self._recent_iso()))
157+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC):
158+
unfiltered = self.client.get(URL)
159+
filtered = self.client.get(URL, {"tickers": "msft"})
160+
self.assertTrue(unfiltered.has_header("Last-Modified"))
161+
self.assertFalse(filtered.has_header("Last-Modified"))
162+
163+
def test_if_modified_since_revalidates_unfiltered_but_never_filtered(self):
164+
# RFC 9110: with no If-None-Match, the server answers from
165+
# If-Modified-Since alone and never consults the ETag — so a 304
166+
# here would tell the client to reuse a differently-filtered body.
167+
self._write("2026-07-06", make_artifact(self._recent_iso()))
168+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC):
169+
first = self.client.get(URL)
170+
ims = first["Last-Modified"]
171+
unfiltered = self.client.get(URL, HTTP_IF_MODIFIED_SINCE=ims)
172+
filtered = self.client.get(URL, {"tickers": "msft"},
173+
HTTP_IF_MODIFIED_SINCE=ims)
174+
self.assertEqual(unfiltered.status_code, 304)
175+
self.assertEqual(filtered.status_code, 200)
176+
177+
def test_etag_is_stable_across_tickers_order_whitespace_and_dupes(self):
178+
# The view filters on a normalized set; the ETag must key on the
179+
# same normalization or equivalent requests never revalidate.
180+
art = make_artifact(self._recent_iso(), signals={
181+
"MSFT": make_artifact("x")["signals"]["MSFT"],
182+
"AAPL": dict(make_artifact("x")["signals"]["MSFT"], guid="g2"),
183+
})
184+
self._write("2026-07-06", art)
185+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC):
186+
first = self.client.get(URL, {"tickers": "MSFT,AAPL"})
187+
second = self.client.get(URL, {"tickers": " aapl , msft ,msft"})
188+
self.assertEqual(first["ETag"], second["ETag"])
189+
revalidated = self.client.get(URL, {"tickers": "AAPL,MSFT"},
190+
HTTP_IF_NONE_MATCH=first["ETag"])
191+
self.assertEqual(revalidated.status_code, 304)
192+
193+
def test_etag_distinguishes_literal_plus_token_from_plus_joined_list(self):
194+
# %2B decodes to a literal '+' inside one token; the ETag's '+' join
195+
# must not let {'AAPL','MSFT'} and the single token 'AAPL+MSFT'
196+
# collide, or a 304 would point a client (or the shared proxy cache
197+
# behind Cache-Control: public) at a differently-filtered body.
198+
art = make_artifact(self._recent_iso(), signals={
199+
"MSFT": make_artifact("x")["signals"]["MSFT"],
200+
"AAPL": dict(make_artifact("x")["signals"]["MSFT"], guid="g2"),
201+
})
202+
self._write("2026-07-06", art)
203+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC):
204+
joined = self.client.get(URL, {"tickers": "AAPL,MSFT"})
205+
literal = self.client.get(URL, {"tickers": "AAPL+MSFT"})
206+
self.assertEqual(len(joined.json()["signals"]), 2)
207+
self.assertEqual(literal.json()["signals"], {})
208+
self.assertNotEqual(joined["ETag"], literal["ETag"])
209+
210+
def test_artifact_loaded_from_disk_once_per_request(self):
211+
# @condition calls _etag and _last_modified before the view runs;
212+
# without per-request memoization one GET pays 3x glob+stat+read.
213+
self._write("2026-07-06", make_artifact(self._recent_iso()))
214+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC), \
215+
mock.patch.object(signals_views, "_load_latest",
216+
wraps=signals_views._load_latest) as loader:
217+
resp = self.client.get(URL)
218+
self.assertEqual(resp.status_code, 200)
219+
self.assertEqual(loader.call_count, 1)
220+
221+
def test_artifact_vanishing_between_glob_and_stat_404s_fail_closed(self):
222+
# A retention job (or manual cleanup) can unlink a candidate after
223+
# glob() lists it and before max() stat()s it — that race must fail
224+
# closed to 404, never surface as a 500.
225+
self._write("2026-07-06", make_artifact(self._recent_iso()))
226+
real_stat = Path.stat
227+
228+
def racing_stat(self, **kwargs):
229+
if self.name.endswith(".json"):
230+
raise FileNotFoundError(self.name)
231+
return real_stat(self, **kwargs)
232+
233+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC), \
234+
mock.patch.object(Path, "stat", autospec=True,
235+
side_effect=racing_stat):
236+
resp = self.client.get(URL)
237+
self.assertEqual(resp.status_code, 404)
238+
self.assertEqual(resp.json(), {"error": "no_signals"})
239+
240+
def test_naive_generated_at_404s_fail_closed(self):
241+
# fromisoformat accepts tz-naive strings; subtracting one from the
242+
# aware now() would raise TypeError in the view — the validator must
243+
# reject a naive generated_at up front.
244+
naive = datetime.now().isoformat(timespec="seconds")
245+
self._write("2026-07-06", make_artifact(naive))
246+
with override_settings(SIGNALS_DIR=str(self.dir), **_HERMETIC):
247+
resp = self.client.get(URL)
248+
self.assertEqual(resp.status_code, 404)
249+
self.assertEqual(resp.json(), {"error": "no_signals"})
250+
149251
def test_malformed_newest_artifact_404s_fail_closed(self):
150252
(self.dir / "signals-2026-07-06.json").write_text("{broken",
151253
encoding="utf-8")

0 commit comments

Comments
 (0)