fix(DATA-003): stop the scan re-dirtying the candle cache it just repaired - #112
fix(DATA-003): stop the scan re-dirtying the candle cache it just repaired#112DoRmAmMu1997 wants to merge 5 commits into
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>
|
@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>
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 |
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