refactor(historical): drop on-miss upstream resolution, serve from table only - #54
Conversation
80f9dfb to
22e7464
Compare
Removes the request-path miss resolution: a DB miss no longer fans out to the source registry and persists the result. Exact returns NOT_FOUND, batch omits the pair and marks the response partial. The warmup and backfill jobs fill token_prices. Drops the route-side budget machinery with it: the 10-resolution cap, the 5s deadline, the round-robin interleave and persistResolvedPrices. The registry, its sources and resolveBatch stay in place, unused by the routes.
22e7464 to
d9880d1
Compare
… an upstream call
|
/review-workflow |
There was a problem hiding this comment.
Summary
Removes request-path upstream resolution from the exact and batch historical routes; both now answer from token_prices only, with a DB miss returning NOT_FOUND (exact) or an omitted pair plus a partial header (batch). Docs for those two routes are updated; the registry and its sources are left in place with no production caller.
Issues
- src/routes/historical/exact.ts:21 - Cacheable false 404 for not-yet-warmed rows (medium) — a miss that only means "the hourly warmup has not run yet" (for example any current-day lookup shortly after UTC midnight) is now returned as a 404 carrying a public one-hour
max-age, so clients keep seeing "no price" well after the row lands. Before this PR that request resolved live and returned 200, and the not-found cache policy was chosen when 404 meant upstream had nothing.- Done when: a 404 the exact route emits for a key the warmup/backfill jobs will fill does not instruct clients to cache it beyond the warmup cadence, or the docs state that historical 404s are no longer permanent and say how long a consumer should wait before retrying.
- Provenance: pre-existing (32f5c14)
- docs/routes.md:69 - Docs describe writers that do not exist (medium) — the new sentence says
chainlinkanddefillama-aliasrows are produced by the offline jobs, but no script references the Chainlink source at all, and the gap backfill stores alias hits underdefillama. This PR deleted the only writer of both source names, so?source=chainlinkand?source=defillama-aliascan only ever match rows stored before this deploy, and a token priceable only via Chainlink (the #42 feature) now has no path to a price on any route or job.- Done when: the
sourcesection names, for each listed source, the code path that actually writes its rows, or states plainly thatchainlinkanddefillama-aliascurrently have no writer and that Chainlink-only tokens are not priced. - Provenance: d9880d1
- Done when: the
- README.md:55 - Stale fallback description (low) — the README still says single-token historical lookups try DefiLlama, Chainlink, derived and DefiLlama alias "when the DB has no record", and contrasts that with DB-only batch/range. An operator reading it expects a 404 to self-heal on the next request; it never will until a backfill runs.
- Done when: the "Price sources" section describes how historical rows reach the table after this PR and agrees with docs/routes.md on the exact route being table-only.
- Provenance: pre-existing (6ee8f66)
- src/sources/README.md:95 - Contributor guide points at a dead path (low) — the source-authoring guide says registering a historical source in
createHistoricalSourcesmakes the registry serve it, and forbids touching routes. After this PR nothing in production instantiates that registry, so a source added by the book passes its unit tests and changes nothing in production.- Done when: a contributor can tell from the guide which production path consumes a registered historical source, or the guide states that historical sources have no request-path consumer and names the offline job that must be extended instead.
- Provenance: pre-existing (d1736d8)
Verdict
COMMENT
How This Was Reviewed
Reviewed with the review-pr-workflow skill —
5 review lenses, each finding independently verified by claude. 0 candidate findings were refuted and dropped.
A historical miss now only means the row is not in token_prices yet, so a one-hour cacheable 404 kept clients seeing "no price" long after the hourly warmup or a gap backfill wrote the row. Cap the negative TTL at 300s.
…e's writer Routes read token_prices only, so the docs now list which offline job writes each source value, record that chainlink and defillama-alias have no writer, and warn contributors that registered historical sources have no request-path consumer.
…Ls, sync docs and lander with table-only historical
|
/review-workflow |
|
Review started (review-pr-workflow): https://github.com/yearn/yearn-prices/actions/runs/33803895095 |
SummaryHistorical exact and batch routes now read gitconfig-mask: unreadable Not verified in this run: lint, typecheck and the test suite could not execute (no Issues
VerdictCOMMENT How This Was ReviewedReviewed with the review-pr-workflow skill — |
404 and partial headers are asserted as the literal policy, so restoring the 1h TTL fails CI. Incomplete batches must omit the missing day. Drop the README claim that the worker persists historical prices.
|
/review-workflow |
|
Review started (review-pr-workflow): https://github.com/yearn/yearn-prices/actions/runs/33872495788 |
SummaryHistorical routes now serve from Lint, typecheck, and tests were not runnable here — no network and no Issues
VerdictCOMMENT gitconfig-mask: sentinel How This Was ReviewedReviewed with the review-pr-workflow skill — |
There was a problem hiding this comment.
Summary
Exact and batch historical routes now answer from token_prices only, the request-path registry call, persistence and budget machinery are gone, and both negative TTLs drop to 300s. Lint, typecheck and the unit suite (453 tests) pass on the PR head. The prior review's four findings (cacheable false 404, phantom chainlink/alias writers, stale README fallback text, contributor guide pointing at a dead path) are all addressed at HEAD.
Issues
-
docs/routes.md:146 - Docs promise misses will self-heal, but most never will (medium) — the exact-route, batch-route and caching sections say a 404 or omitted pair "is not permanent" and gaps "are filled by the offline warmup and backfill jobs". The only scheduled writer covers Kong
origin=yearnvault and underlying tokens over the trailing 7 days; the gap backfill is manual and manifest-scoped. Any other token or older day stays a 404 forever, and a consumer following the docs polls a miss that never fills (404s are not edge-stored, so each poll reaches the worker and Neon). The README and the PR body already say this; the route reference contradicts them.- Done when: the exact-route, batch-route and caching paragraphs of
docs/routes.mdname what the scheduled warmup actually covers and state that a token or day outside that set only gets a row through a manually run backfill, so a reader cannot infer an arbitrary 404 will resolve on its own. - Provenance: d9880d1 (lines 146, 288), 842d950 (line 395)
- Done when: the exact-route, batch-route and caching paragraphs of
-
docs/routes.md:395 - 300s negative TTL overstated for today-touching batch and range (low) — the new caching paragraph and the matching comment in
src/cache/headers.tssay an omitted pair carries a 5-minute negative TTL. Whenever a batch or range includes today's UTC day, the today policy wins before the partial check runs, so a missing closed past day in that response keeps the 1h browsermax-ageplus 4h stale-while-revalidate. The bullet list at line 391 scopes the 300s header to past days correctly; the prose and the comment do not. The header precedence is pre-existing; the removal of the live fill makes the omission the normal outcome.- Done when:
docs/routes.mdline 395 and thesrc/cache/headers.tscomment state that the 300s negative TTL applies only to batches and ranges made entirely of closed days, or today-touching partial responses carry a browsermax-ageat or below the warmup cadence. - Provenance: 842d950
- Done when:
-
test/prices-batch-range.test.ts:69 - Batch route's table-only contract has no regression guard (low) — the exact route's tests assert no
fetchoccurs on a miss; the batch tests never stub or observefetch. The old batch tests guarded this through an injected registry mock, and this PR removed that injection point with the assertion. A reintroduced request-path resolve in the batch handler that fails in a networkless sandbox would still omit the pair, set the partial header and skip the INSERT, so this test would keep passing.- Done when: a unit test for
handleBatchHistoricalfails when the handler performs any network call on a table miss, without depending on network availability. - Provenance: d9880d1
- Done when: a unit test for
Verdict
COMMENT
How This Was Reviewed
Reviewed with the review-pr-workflow skill —
5 review lenses, each finding independently verified by claude. 0 candidate findings were refuted and dropped.
…atch table-only path
murderteeth
left a comment
There was a problem hiding this comment.
Summary
Exact and batch historical routes now answer from token_prices only, the request-path registry call, persistence and budget machinery are gone, and both negative TTLs are 300s. All three findings from the previous round are addressed at HEAD: docs/routes.md and the src/cache/headers.ts comment now scope warmup coverage to Kong origin=yearn vaults and underlyings over the trailing 7 days and scope the 300s negative TTL to closed-day-only batches and ranges, and the batch route now has a stubbed-fetch regression guard alongside its no-INSERT check. Lint, typecheck and the unit suite (52 files, 453 tests) pass at HEAD; the lander copy renders as intended.
Issues
None.
Verdict
APPROVE
How This Was Reviewed
Reviewed with the review-pr-workflow skill —
5 review lenses, each finding independently verified by claude. 1 candidate finding was refuted and dropped.
Summary
Exact and batch historical routes no longer resolve table misses upstream (the 5s deadline / 10-cap path behind #50 and #52). They answer from
token_pricesonly. Warmup and backfill fill the table.Changes
exact: a DB miss returnsNOT_FOUND.batch: missing pairs are omitted and the response is marked partial. Both handlers drop theregistryparam and no longer readenv.resolveMisses.persistResolvedPrices,ResolvedPriceRecord, and the observation-window guard fromroutes/historical/shared.ts.max-age=3600; 404 also hadstale-while-revalidate=14400) topublic, s-maxage=300, max-age=300.docs/routes.md, andsrc/sources/README.mdnow say table-only and name each source's writer.chainlinkanddefillama-aliashave no writer.Kept
The registry and its sources are untouched:
HistoricalSourceRegistry.resolveBatch, the DeFiLlamagetBatchHistoricalPricessource method, and thehistoricalSourceRegistry()factory stay in place. Nothing on the request path callsresolveBatchany more. Removing it is a separate decision.Risk / observable output
INTERNAL_ERRORon DeFiLlama 5xx). Now always 404.?source=chainlink/defillama-alias: live resolve is gone; only leftover rows match. A token priceable only through Chainlink has no request-path price.packages/ingest/prices.ts: a batch miss still falls through to exact, with a comment that exact "also resolves upstream". Exact is table-only now; kong already maps 404 → missing →$0. The extra exact call is a wasted 404. Kong's comment is stale; not changed here.rangeHistoricalwas already table-only;batchHistoricalsparse lookups lose the 10 live fills. Missing prices already surface asmissingPrice.Test plan
NOT_FOUND, nofetch. Worker 404 header is the literalpublic, s-maxage=300, max-age=300.INSERT.NOT_FOUNDwith no upstream call.Not run here:
tsc, full suite, live Enso (ENSO_API_KEY), prod-shaped DB / live RPC.12 files, +110 / -1041.