fix(DATA-003): stop the scan re-dirtying the candle cache it just repaired - #112
Conversation
Nine symbols were quarantined with DUPLICATE_DATE on scan run_id=4, 46 seconds after the DATA-002 repair had cleaned them. The repair was not at fault: it ran, worked, and was overwritten by the scan itself. Aligning the DB (UTC) with the app log (IST) shows the parquet files being rewritten at 19:11:57-19:15:24, between the repair finishing at 19:11:11 and the failures at 19:17:50. Two defects, one causing the other. Defect A - vendor duplicates were persisted verbatim. normalize_daily_payload sorted but never de-duplicated, and of the six cache-write sites in the loader only the incremental merge deduped first. DhanHQ repeats bars: AEGISLOG carries two byte-identical rows for 2024-06-05. Fixed at the vendor boundary with drop_duplicates() over all six columns, so no write path can persist a redundant bar and every consumer benefits, including frames handed straight to screeners. Deliberately narrow: only rows identical in EVERY column are dropped. Bars sharing a date but differing in any value - including volume alone, which is a partial-vs-final bar - survive to be reported, because choosing between them would fabricate a price series that never existed. A guard test asserts a conflicting bar still reaches disk so DATA-001 quarantines it. Defect B - the cache-hit test could not be satisfied. get_daily_history required last_date >= requested_end, and scans request "through today" while the vendor's newest published bar is Friday's. Zero of 577 symbols qualified, so every scan re-downloaded the whole universe, which is what fired Defect A across the cache and made the scan take six minutes. The end comparison now tolerates STALE_LATEST_TOLERANCE_DAYS, the constant DATA-001 already defines as how far the newest candle may trail today before that is suspicious; a frame inside it still raises STALE_LATEST_CANDLE, so nothing is hidden. The start comparison stays strict, preserving the original guard against running a long-lookback screener on an interrupted prefetch's partial file. Verified against a copy of the real 577-file cache: fatal symbols 18 -> 5, 1,264 rows removed, and a scan-shaped read of the repaired symbols is now a cache hit that leaves the files untouched (previously it re-downloaded and re-dirtied them). The remaining five carry price-level conflicts from the vendor and stay quarantined by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w opened
Both cache-coverage checks require first_date <= requested_start, where
requested_start is today minus ten years. A stock that listed after that date can
never satisfy it, because DhanHQ has nothing earlier to give. 200 of 577 cached
symbols were in that state (DMART listed 2017-03-21, RBLBANK 2016-08-31, LTTS,
COHANCE, ~196 more): every prefetch and every scan re-downloaded their full
history, wrote the same short frame back, and did it again next time.
The two cases look identical from the cached file alone and must not be conflated:
an interrupted prefetch leaves a partial file while the vendor DOES have the
missing years and must be refetched; a later listing means the vendor has nothing
earlier and refetching is waste forever.
Fixed by recording what the vendor actually served, following the sidecar
precedent DATA-002 established with .checked and .repaired. A new .firstbar marker
stores the probe's requested_from, the earliest_available bar that came back, and
recorded_on. A cache is then treated as reaching back far enough when it either
literally covers the requested start or already begins at the vendor's earliest
known bar.
Three properties keep the marker from ever hiding real missing history:
- it only counts when the recorded probe reached at least as far back as the
current request, so a five-year probe cannot suppress a ten-year refetch;
- it expires after VENDOR_EARLIEST_RECHECK_DAYS (30), because vendors do
occasionally backfill history;
- any read or parse failure returns None and the strict rule applies, so a corrupt
marker can only cost an extra request.
Suppressing the backfill deliberately falls through to the normal freshness and
incremental logic rather than returning early: a later listing still needs its
daily top-up, and the point is to stop the pointless ten-year refetch, not to
freeze the symbol.
Measured on a copy of the real 577-file cache, nifty_500 (500 rows):
prefetch pass 1 500 requests, 176 full-window backfills (learns)
prefetch pass 2 176 requests, 0 full-window backfills
prefetch pass 3 0 requests, 0 full-window backfills
scan path before: 324 hits / 176 misses (176 full re-downloads)
after: 500 hits / 0 misses, 0 files rewritten
Stacked on DATA-003 (PR #112), which introduced _cache_covers_range and is not yet
merged; this branch targets that one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex can you review this? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eab5bc5527
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tolerated_end = requested_end - timedelta(days=STALE_LATEST_TOLERANCE_DAYS) | ||
| return first_date <= requested_start and last_date >= tolerated_end |
There was a problem hiding this comment.
When a scan runs on Monday with a cache ending on the preceding Thursday, subtracting the four-day tolerance makes that cache a hit even though Friday's published candle is missing. The scanner then produces signals from stale prices without attempting a refresh; moreover, validate_candles only warns when the gap is greater than four days, so this exact boundary is not reported despite the docstring's claim. Limit the relaxation to dates known to be unpublished/non-trading rather than applying a blanket calendar-day subtraction.
Useful? React with 👍 / 👎.
Addresses Codex P1 on PR #112. The blanket "tolerate STALE_LATEST_TOLERANCE_DAYS" rule served any cache within four calendar days of the requested end, so a cache ending Thursday was handed to a Monday scan even though Friday's bar had been published. Screeners then ran on prices demonstrably behind the market with no refresh attempted. Worse, the docstring justified this by claiming DATA-001 would still raise STALE_LATEST_CANDLE as a backstop. It does not: the warning fires only when the gap is GREATER than the same constant, so the two rules cover disjoint ranges and everything the coverage test tolerated passed silently. That claim was simply wrong and is now corrected in place. The end test is now satisfied by evidence rather than by elapsed time: - _only_unpublished_days_missing: nothing is absent except weekends and possibly the requested end itself. Tolerating the request end is load-bearing, because the current session's EOD bar is routinely unpublished when a scan runs; without it every weekday scan becomes a miss again, which is the problem DATA-003 set out to fix. - the loader's existing .checked marker: written by the prefetch precisely when it asked Dhan for this tail and got nothing back. That covers market holidays, which are weekdays and so fail the arithmetic above despite there being no bar to fetch. Codex suggested limiting the relaxation to known non-trading days via a trading calendar. That dependency was explicitly rejected in the DATA-001 design as heavier than the problem warrants, so this gets the same result from weekday arithmetic plus a receipt the loader already writes. Behaviour, verified by test: cache Fri, scan Mon hit (weekend only - the case DATA-003 exists for) cache Thu, scan Mon MISS (Friday's bar exists - Codex's case) cache Fri, scan Tue MISS (Monday's bar exists) cache Mon, scan Tue hit (today's bar not published yet) cache 3 weeks old MISS holiday gap + .checked marker hit same gap without the marker MISS marker older than the request end MISS STALE_LATEST_TOLERANCE_DAYS is no longer imported by the loader; the constant keeps its DATA-001 meaning and is no longer overloaded to mean two things. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses Codex P2 on PR #114, and merges the updated DATA-003 base. get_daily_history stamped recorded_on with the request's own end_date and judged the marker's age against that same boundary. Two consequences: repeating a historical request always computed an age of zero, so the 30-day expiry never fired; and a future-dated end_date kept the marker fresh indefinitely. Either way a vendor backfill could stay hidden behind a partial cache forever. DailyDataLoader now takes an injectable today_func, alongside the existing sleep_func injection, and both _record_vendor_earliest and _vendor_earliest_for use it. The requested window and the wall clock are now explicitly separate things: "the date I am asking about" is not "the date it is now", and conflating them is precisely what caused the bug. ensure_daily_history keeps its today argument, which describes the data window only. Merge resolution: _cache_covers_range now carries both kinds of evidence, judged independently. The front uses vendor_earliest (DATA-004: how far back the vendor's history goes) and the back uses weekday arithmetic plus the .checked marker (DATA-003: which recent days the market actually published). Re-measured on a copy of the real 577-file cache, nifty_500, after the stricter DATA-003 rule: prefetch pass 1 500 requests, 176 backfills prefetch pass 2 176 requests, 0 backfills prefetch pass 3 0 requests, 0 backfills scan path 500 hits / 0 misses / 0 files rewritten Codex's case Tuesday scan with Monday's bar missing -> 0/50 hits, all refetched Both earlier wins survive the stricter rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by a self-review pass over the previous commit. The .checked branch of _cache_covers_range had no staleness bound, so the marker could certify a cache that was arbitrarily far behind as fully covering the request. Concretely: normalize_daily_response turns a "no data" / "no records" vendor response into an empty frame rather than raising, so a DhanHQ data outage has ensure_daily_history stamp .checked = today on every prefetch. Since the fallback only tested checked_through >= requested_end and never how far last_date trailed, a scan weeks later still got from_cache=True and computed signals on stale prices while the cache-miss counters reported a clean hit. The weekday walk it bypasses is bounded by _MAX_TOLERABLE_GAP_DAYS; the fallback skipped that guard entirely. The marker exists to rescue market holidays, which are short gaps by definition, so it now carries the same bound. A genuinely halted or delisted symbol therefore becomes a cache miss again rather than being served silently - the honest outcome, and DATA-001 already raises STALE_LATEST_CANDLE for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Verified against the code — you were right, and the worst part of it was a claim I wrote. Fixed in The docstring was backwards. The coverage tolerance and the DATA-001 warning cover disjoint ranges, not overlapping ones:
So everything the tolerance accepted passed silently. My justification that the warning "still fires as a backstop" was simply false, which is exactly why the end test could not be allowed to lean on a tolerance window. The end test is now evidence-based rather than time-based:
On your suggested remedy: a trading calendar was explicitly rejected in the DATA-001 design as "heavier than a warning needs", so this gets the same outcome from weekday arithmetic plus a receipt the loader already writes. Behaviour, each covered by a test: A self-review pass then found a third bug in that fix, which
Re-measured on a copy of the real 577-file cache (nifty_500): the scan path still gets 500 hits / 0 misses / 0 files rewritten for the Friday-cache-Monday-scan shape, and a Tuesday scan with Monday's bar missing now correctly refetches 50/50. Gates: 1,949 tests, 89.81% coverage, ruff/mypy/bandit clean. |
DoRmAmMu1997
left a comment
DoRmAmMu1997
left a comment
There was a problem hiding this comment.
Comment-only review at the current PR head. I found one P2 data-correctness issue and will address it in the approved follow-up commit.
| """True when nothing the market has actually published is missing from the cache. | ||
|
|
||
| Two kinds of day may be absent without the cache being out of date: | ||
|
|
There was a problem hiding this comment.
[P2] Keep historical requests strict
_only_unpublished_days_missing always exempts requested_end itself, but get_daily_history is also used by the forward-return service with caller-supplied historical as_of dates. A cache ending on 2026-08-24 is therefore reported as a hit for a historical request through the published weekday 2026-08-25, so Dhan is never asked for the missing bar; a later .checked marker can also rescue that historical gap. Make unpublished-tail handling an explicit scanner-only opt-in and keep the direct API conservative by default. I reproduced the current result as historical_end_without_marker=True at this head.
Co-authored-by: Codex <codex@openai.com>
Codex follow-up completePushed Changes
Review and verification
The commit includes |
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
…t-bar fix(DATA-004): stop re-downloading stocks that listed after the window opened
1 participant
Why
Scan
run_id=4quarantined nine symbols withDUPLICATE_DATE— the exact defect DATA-002 was built to fix, 46 seconds after the DATA-002 repair had cleaned them.The repair was not at fault. It ran, worked, and was overwritten by the scan itself. Aligning the DB (UTC) with the app log (IST, +5:30):
trigger=prefetch, 570 checked)run_id=4startsDUPLICATE_DATETwo defects, one causing the other.
Defect A — vendor duplicates were persisted verbatim
normalize_daily_payloadended without.sort_values("timestamp").reset_index(drop=True)— it sorted but never de-duplicated. Of the sixto_parquetsites indaily_data_loader.py, only the incremental merge deduped first. The other five wrote the raw vendor frame, includingget_daily_history's cache-miss path.The duplicates come straight from DhanHQ. AEGISLOG's, verbatim from the cache:
Byte-identical. This is also where the original 18 dirty symbols came from.
Defect B — the cache-hit test could never be satisfied
get_daily_historycounted a hit only whenlast_date >= requested_end, and scans request "through today" while the vendor's newest published bar is Friday's. 0 of 577 symbols qualified, so every scan re-downloaded the entire universe — which fired Defect A across the whole cache and made the scan take six minutes.What changed
A — de-duplicate at the vendor boundary.
drop_duplicates()over all six columns innormalize_daily_payload, so no write path can persist a redundant bar and every consumer benefits, including frames handed straight to screeners.Deliberately narrow: only rows identical in every column are dropped. Two identical bars for one day cannot both be real observations, so removing one loses nothing and costs no trading day. Bars sharing a date but differing in any value — including volume alone, which is a partial-vs-final bar — survive to be reported. Choosing between them would fabricate a price series that never existed; that resolution belongs to the DATA-002 repair, against the vendor. A guard test asserts a conflicting bar still reaches disk so DATA-001 quarantines it.
B — tolerate an unpublished tail. The end comparison now allows
STALE_LATEST_TOLERANCE_DAYSof slack — the constant DATA-001 already defines as "how far the newest candle may trail today before that is suspicious". A frame inside it still raisesSTALE_LATEST_CANDLEas a warning, so nothing is hidden by serving it. The start comparison stays strict, preserving the original guard against running a long-lookback screener on an interrupted prefetch's partial file.Guard test (
tests/test_candle_cache_write_paths.py) locks the invariant at every entry point — cache miss, fresh download, backfill — rather than trusting each call site to remember.Verified against a copy of the real cache
.tmpfilesThe before/after on the cache-hit behaviour is the one that matters — the same read that destroyed the repair last time now leaves the files alone.
Expected to still fail, honestly
ABREL, LTF, MOTHERSON, PATANJALI, PVRINOX carry price-level conflicts straight from DhanHQ (two merged series from symbol renames — MOTHERSON has 3,800 conflicting rows). Neither this fix nor the DATA-002 repair may resolve those without inventing prices, so they stay quarantined and reported as
unrepairable. That is correct behaviour, not a gap.Follow-up found, not fixed here
200 of 577 symbols listed after the 10-year start date (RBLBANK 2016-08-31, DMART 2017-03-21, …). They can never satisfy
first_date <= requested_startbecause the vendor has no earlier data, so they re-download on every prefetch and every scan, permanently. With this PR those re-downloads at least write clean data, so it is now a cost-and-latency issue rather than a correctness one. Worth its own ticket.Gates
1,939 tests pass, coverage 89.82% (floor 89%); ruff, mypy, bandit, compileall clean. No dependency changes.
🤖 Generated with Claude Code