fix(0215): make the two-pivot guard runnable, and warn when a reference narrows - #304
Merged
Merged
Conversation
…ce narrows A peg-pivot step must issue one peg and TWO pivots (XLM, USDT). Nothing that runs in CI asserted that. The end-to-end coverage exists — the 0172 and all-tiers integration tests would both go red on a narrowed pivot set — but every test in ch_enrich_it.rs is #[ignore] and needs a live ClickHouse, and no workflow provides one, so `cargo test --workspace` skips all 19. The guard was written and never armed. Wiring ClickHouse into CI is task 0275. Split the statement planning out of enrich_peg_pivot_step into a pure plan_peg_pivot_step returning Vec<StepStatement>. The two variants carry different bind sequences, so this is an enum rather than Vec<String>. The executor now matches and binds; no behaviour changes. plan_issues_one_peg_and_two_pivots asserts one peg, two pivots, refs [xlm, usdt] in order, and that each pivot's SQL carries its ref id as the literal that made the defect visible in system.query_log at all. Verified by inducing, not assumed. Narrowing pivot_ids() to [xlm] fails it. Leaving pivot_ids() correct but adding a `break` to the loop ALSO fails it, while the existing reference_ids_helpers stays green — the new test covers the emission, which the set-level test cannot see. Also: resolve_reference_ids now warns, naming the absent code, when a reference does not resolve. Partial sets stay legal — bootstrap discovers assets in arbitrary order — but a narrowed pass must not happen quietly, which is what cost 26 days.
…test comment Two documentation defects introduced by the previous commit, both found in review. The StepStatement block was inserted between peg_sql's doc comment and peg_sql itself. Rust merges consecutive /// blocks, so that comment silently became the enum's documentation and peg_sql was left with none — losing its bind-order contract, which is precisely what the executor's .bind(watermark).bind(batch_size) arm depends on and what a future editor of that arm would go looking for. Move the enum and plan_peg_pivot_step above the comment so it reattaches. The no-USDC assertion's comment claimed the step "degrades to the peg alone" while the assertion beneath it requires the plan to be empty. The assertion is right: task 0172 made USDC the only peg member, so no USDC means stable_ids() is empty and peg_sql returns None — nothing to peg and nothing to pivot against. A reader trusting the comment would have "fixed" the assertion, in the one test that is currently the only armed guard on this path.
Task 0268 landed ~475 lines of external-tier tests in ch_enrich.rs while this branch was open. The single conflict sat entirely inside `mod tests` and was purely additive on both sides — 0268's external-tier block against 0215's `plan_issues_one_peg_and_two_pivots`. Both are kept. No production code conflicted: 0268 did not touch `enrich_peg_pivot_step`, `peg_sql` or `pivot_sql`, so the plan/executor split merges unchanged. Verified on the resolved tree: cargo test --workspace 903 passed / 0 failed, `plan_issues_one_peg_and_two_pivots` green, clippy --all-targets and fmt --check clean.
karczuRF
added a commit
that referenced
this pull request
Sep 10, 2026
PR #304 merged (b7496dd): the two-pivot guard now runs in CI, and a missing reference asset is named in the logs. The "fails loudly" AC is amended to "named in the logs", with the reasoning recorded. A hard failure would refuse to start on a fresh registry — bootstrap discovers assets in arbitrary order, so partial reference sets are legal. What turned this defect into a 26-day outage was the silence, not the tolerance, and the warn removes exactly that. `max_execution_time` records the scope decision (the scheduled worker only, not the operator CLIs) and why 120 s is not a load-bearing number after 0111. Left unticked: the induction against prod is outstanding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the code half of task 0215. The defect itself was fixed on 2026-08-21; this arms the guards that were meant to stop it recurring.
The gap
0215 asked for a test "so a silently-narrowed pivot set fails the suite instead of the quote leg."
The end-to-end coverage already existed — narrow
pivot_ids()to[xlm]andusdt_quoted_candles_pivot_on_the_measured_rate_not_a_dollar_peggoes red; narrow it to[usdt]andenrich_fills_close_usd_across_oracle_peg_and_pivot_tiersgoes red.But those tests have never run. Every test in
ch_enrich_it.rsis#[ignore](they need a live ClickHouse), CI runscargo test --workspacewhich skips them, and no workflow provides a database. Every CI run has printed19 ignoredfor months. The guard was written and never armed. Filed as 0275.What this PR does
Splits planning from sending.
enrich_peg_pivot_stepnow calls a pureplan_peg_pivot_stepreturningVec<StepStatement>; the executor matches and binds. The two variants take different bind sequences, which is why it's an enum rather thanVec<String>. No behaviour change.Adds
plan_issues_one_peg_and_two_pivots— a unit test that runs in CI today. It asserts one peg, two pivots, refs[5, 7]in order, a total of 3, and that each pivot's SQL carries its ref id as the literal that made this defect readable insystem.query_logat all.Warns when a reference does not resolve.
resolve_reference_idsnow names the absent code.stable_ids()/pivot_ids()flatten aNoneaway, so a missing USDT silently issues one statement where two belong and still reports success — the exact shape of the defect. Partial sets stay legal (bootstrap discovers assets in arbitrary order), so this warns rather than fails; what it must not do is happen quietly.Verified by inducing, not assumed
reference_ids_helpers(existing)plan_issues_one_peg_and_two_pivots(new)pivot_ids()narrowed to[xlm]pivot_ids()correct,breakafter first pivotThe second row is the point: an emission defect is invisible to a set-level assertion. Both inductions were run and reverted; the working tree carries neither.
cargo test -p enrichment-worker— 76 passed, 0 failed (was 75).cargo clippyclean,cargo fmt --checkclean.Prod state, re-verified today
The fix has held for three weeks, and 0111 moved it further out of reach:
BadResponse("")All prod reads were read-only — ClickHouse as
dev_read(readonly = 1), AWS asReadOnlyAccess.