Repository navigation
test(dst): name unreachable crash windows and fix index-build reach - #1035
Conversation
The reach census asserted 48 crossed windows, a floor set when the catalog had 66; on main it measured 40/52 and failed every run. Two windows had gone dark because their recipes aimed at shapes that existed only while branches created table forks. branch_merge.post_table_effect needs a merged table that stages its own effect, and a merge into an unchanged main only switches pins, so it now takes the diverged-main rewrite recipe. ensure_indices.post_table_effect built indexes on the milestone branch; on main the first index build of a table crosses it, so it now takes the family default, and the branch milestone goes. The census measures 42/52. The floor is 40, and its comment names why each of the ten dark windows cannot be reached.
Review of
|
| Commit | Command (from crates/omnigraph-dst) |
Result |
|---|---|---|
base 0b5e17fa |
cargo test --locked --test scenarios dst_window_reach_probe -- --exact --ignored --nocapture |
REACH CENSUS: 40/52, 12 dark, panic milestone reach regressed below the measured 50 (40 crossed): fails, as the PR says |
head 97ff9339 |
same | REACH CENSUS: 42/52, 10 dark (both recipe windows left the dark list), 1 passed |
head 97ff9339 |
cargo test --locked --no-fail-fast |
lib 53 passed; integration 2 passed, 3 ignored; scenarios 51 passed, 28 ignored |
head 97ff9339 |
DST_FLEET_SEEDS=1 DST_FLEET_SEED_BASE=10047 and =10006 cargo test --locked --test scenarios dst_fleet -- --exact --ignored --nocapture |
crash:branch_merge.post_table_effect@op5 (NotApplied), crash:ensure_indices.post_table_effect@op2 (Applied); each run reports 0 failures |
base 0b5e17fa |
same fleet seeds | also crash at op5 and op3; each run reports 0 failures |
| head + temporary edit (reverted) | census with the old ensure-indices sequence, but EnsureIndicesMain |
still dark (41/52); see the inline comment on harness.rs:786 |
The CI and DST workflows passed on exact head 97ff9339, including DST pinned suite, Lint (clippy), and Format (rustfmt). CI does not run the census.
Regression proof. The same command fails on base for the predicted reason (40 < 48). It passes on head (42 ≥ 40), and the two re-pointed windows move from dark to crossed. GQT cannot express this defect. It is in the recipes and floor of a DST instrument, not in rows, counts, column types, or errors. The existing owner is the DST scenario test, and the PR changes that test.
Findings
Blocking: none.
Optional:
scenarios.rs:3482-3498: replace the count floor with an executable expected-dark set. This is the main liability point; details are inline.harness.rs:786: the reason given for the ensure-indices recipe does not match the code. The new recipe also depends on seed order: an earlier samplerOptimizepre-empts the window.scenarios.rs:3417: stale probe text just outside the hunk (66-windowignore message, "recovery internals", a run command that does not work from the root).
Note: harness.rs:825: please say in the PR body that this change also affects the nightly fleet's window arm.
Tradeoffs and liability
- Removed: an instrument that was always red. Also removed: a misleading milestone.
EnsureIndicesBranchclaimed branch scope, butexec_oprunsdb.ensure_indices()on the session's branch, which is main. Net −7 lines. - Added: a new magic number (40) and a dated prose list ("42/52 on 2026-10-16"). Both need a manual edit after every catalog change. test(dst): reach the dead-staging sweep window #1034 already makes the list stale (53 windows). With a fixed floor, each new crossing window adds slack. After test(dst): reach the dead-staging sweep window #1034, three existing windows could go dark without a failure. This PR had to exist for the same reason: no per-window ledger was checked in. Optional finding 1 removes that liability, and it is about the same amount of code.
- Overlap. test(dst): reach the dead-staging sweep window #1034 adds the 53rd window, and test(cleanup): qualify sweep retries across storage and DST scheduling #1036 replaces
census_setupand edits the census body. The conflicts are text only. The PR that lands second must measure again and restate the dark list. With finding 1 it adds one entry instead. - Lance direction.
optimize.rsnotes that Lance 11 folds indexes only through a linear commit. A later Lance version can change howoptimizeand index builds behave, and with that, which maintenance windows cross. A named set reports the exact window that changed. A count does not.
Limits
- I ran the census once per commit on one machine. Universes are pure functions of their seeds, so I did not repeat the runs.
- I did not run the full nightly fleet or the hunt. My fleet evidence covers only the two seeds that land on the changed windows.
- I did not run clippy locally. I rely on the exact-head
Lint (clippy)job. - This review makes no performance claims.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…-reach-census-floor
A count floor could not say which window went dark, and its slack grew with every window added. REACH_EXPECTED_DARK lists each window the census cannot cross, with its reason; the census now fails, naming the window, when another window stays dark, a listed one crosses, a listed name left the catalog, or a universe trips an oracle. ensure_indices.post_table_effect went dark because a sampled Optimize builds the missing indexes before the first EnsureIndices in all three of its seeds, not because its recipe named a branch: the harness runs every index build on main. The recipe note says so. The probe's doc describes the current census and a run command that works. Two nits from the dead-staging review: the pin drops its redundant window_hit check, and the fleet comment says a setup crash runs first.
ragnorc
changed the title
Review of
|
| Commit | Command | Result |
|---|---|---|
main 85a46138 |
cargo test --locked --test scenarios dst_window_reach_probe -- --ignored --nocapture |
REACH CENSUS: 41/53, 12 dark (the ten listed windows plus branch_merge.post_table_effect and ensure_indices.post_table_effect), errored empty, panic milestone reach regressed below the measured 50 (41 crossed) |
head b0dc0e94 |
same | REACH CENSUS: 43/53, the 10 dark names are exactly REACH_EXPECTED_DARK, errored empty, 1 passed in 135.77s |
| head, temporary list, then restored | same | panic names newly dark ["blob_read.post_capture"], listed dark but crossed ["branch_control.pre_gates"], listed but not in the catalog ["renamed.away"] |
head b0dc0e94 |
cargo test --locked --no-fail-fast |
lib 53 passed. integration 2 passed, 3 ignored. scenarios 53 passed, 28 ignored |
head b0dc0e94 |
cargo clippy -p omnigraph-dst --all-targets --locked -- -D warnings -W clippy::dbg_macro |
exit 0, finished in 4m 23s |
CI on this exact commit is green, including DST pinned suite and Lint (clippy). CI does not run the census. Local results above are separate from that CI.
Regression proof. GQT cannot express this defect. The defect is the floor and the recipes of a DST instrument. The owner is dst_window_reach_probe. The same command fails on main 85a46138 for the predicted reason (41 crossed, old floor 48, message cites the measured 50). It passes on b0dc0e94 (43 crossed, dark set equals the list).
Limits
- I ran each census once, on one machine. Each universe is a pure function of its seed.
- Fleet evidence here is the window arm for seeds 10028 and 10039. I did not run the other seven universes of
dst_fleetfor those seeds. - This review makes no performance claims.
Optional inline findings
1. crates/omnigraph-dst/src/harness.rs:865
Optional. The first two sentences match the code. The last sentence does not.
exec_op runs EnsureIndices as db.ensure_indices() on the session branch, which stays main. plan_index_work_node adds a btree only when has_btree_index is false, and has_btree_index is true once any btree exists. apply_optimize_table_effects builds that same missing index. An earlier Optimize leaves ensure_indices.post_table_effect nothing to commit.
I logged the three census seeds for this window (catalog index 22, so seeds 40000 + 22, 50000 + 22, 60000 + 22):
- seed 40022:
Optimizeat op1, thenEnsureIndicesat op2.crossed=false - seed 50022:
EnsureIndicesat op1, and noOptimizebefore it.crossed=true - seed 60022:
Optimizeat op0, thenEnsureIndicesat op4.crossed=false
The census reports the window as crossed because seed 50022 crosses. Two of the three seeds do sample Optimize first.
Please change the last sentence. The census crosses this window on seed 50022 only. The PR body says the same false sentence ("none of them samples an Optimize first").
2. crates/omnigraph-dst/tests/scenarios.rs:3692
Optional. The loop below keeps a cross over an earlier error. The arm Ok(report) if report.crossed sets the verdict and breaks. The comment on the loop says "first cross wins".
This sentence says no universe may trip an oracle. The PR body says the same. An error fails the census when no seed of that window crosses. A later seed that crosses replaces the error.
Please say that here, and in the PR body. The loop is the right rule. The census asks whether the window is reachable.
A recorded setup crash arms its target, but the dead-staging window's setup, ensure_indices.post_phase_b_pre_manifest_commit, is crossed by an index build with nothing to build as well. In 12 of 30 hunt cells such a build recorded the setup without stranding any staging, and the target was then scheduled on a cleanup it could not reach. The setup is now ensure_indices.post_table_effect, crossed only after a detached index commit; dst_setup_crash_needs_its_aftermath pins seed 2. An Optimize builds missing indexes too, so while a recipe still owes its index build the sampler draws again instead of emitting one: the first index build is an EnsureIndices with the fixture's indexes to build, which makes ensure_indices.post_table_effect structural rather than a property of its seeds. REACH_EXPECTED_DARK moves into the harness. The census asserts it, a lib test keeps its names in the catalog, and the hunt and the fleet no longer schedule its windows.
ragnorc
changed the title
Review of
|
| Commit | Command | Result |
|---|---|---|
| head with the old setup window | cargo test --locked --test scenarios dst_setup_crash_needs_its_aftermath -- --exact --nocapture |
fail. left: 1, right: 0. Verdict crash:ensure_indices.post_phase_b_pre_manifest_commit@op13 (Applied). The cleanup window is absent |
1488664c |
same | 1 passed in 3.51s. DST_OP_LOG=1: seed 2 has Optimize at op 2 and EnsureIndices at op 13. The new setup records no crash |
1488664c |
cargo test --locked --test scenarios dst_dead_staging_sweep_window_is_reached_and_retried -- --exact --nocapture |
1 passed in 11.23s. Fleet and hunt both defer the cleanup window after the new setup |
1488664c |
cargo test --locked --test scenarios dst_setup_crash_waits_for_a_crossed_window -- --exact --nocapture |
1 passed |
1488664c |
cargo test --locked --lib catalog::tests::expected_dark_windows_are_catalog_windows -- --exact |
1 passed |
1488664c |
cargo test --locked --test scenarios dst_window_reach_probe -- --ignored --exact --nocapture |
REACH CENSUS: 43/53. Dark set equals REACH_EXPECTED_DARK. errored empty. 1 passed in 120.75s |
CI on exact head 1488664c: DST run 39488091975 success, including DST pinned suite. Format (rustfmt), Lint (clippy), and Test Workspace success (run 39488091995). CI Gate fails because Dependency Guard (cargo deny) reports RUSTSEC-2026-0382 on rustls-webpki 0.103.15. main b692610c has the same lock line. main CI succeeded at 16:08 UTC. This run failed at 19:59 UTC. This diff does not change Cargo.lock. CI does not run the census.
Limits
- The before row is the old setup window on this head, not a rebuild of
b0dc0e94. The engine files are the same. The hunt cell does not use the redraw. - I did not re-run the hunt. The PR body reports 227 universes, 37 hit, and 0 never reached.
- I did not re-run the 20-seed table in the PR body. The census shows that seed 40022 now crosses.
- I ran each test once. Each universe is a pure function of its seed.
- This review makes no performance claims.
Inline comment
crates/omnigraph-dst/tests/scenarios.rs line 3699, side RIGHT:
Optional. This sentence is still false. The loop below keeps a cross over an earlier error. The arm Ok(report) if report.crossed sets the verdict and breaks. The comment on the loop says "first cross wins".
An error fails the census when no seed of that window crosses. A later seed that crosses replaces the error. The PR body says the same thing ("any universe trips an oracle").
Please say that here, and in the PR body. The loop is the right rule. The census asks whether the window is reachable.
…-reach-census-floor
The census keeps the first seed that crosses a window, so a seed that trips an oracle fails the census only when no seed of that window crosses. The doc said no universe may trip an oracle.
|
Thanks. The open finding is fixed in 910ea3f: the probe doc now says that the first seed that crosses a window wins, so a seed that trips an oracle fails the census only when no seed of that window crosses ( |
Review of
|
| Commit | Command | Result |
|---|---|---|
3df1edf9 |
cargo test --locked --test scenarios dst_window_reach_probe -- --ignored --exact --nocapture |
REACH CENSUS: 43/53. Dark names equal REACH_EXPECTED_DARK. errored empty. 1 passed in 128.59s |
3df1edf9 |
cargo test --locked --test scenarios dst_setup_crash_needs_its_aftermath -- --exact --nocapture |
1 passed in 3.65s |
3df1edf9 |
cargo test --locked --lib catalog::tests::expected_dark_windows_are_catalog_windows -- --exact |
1 passed |
Census dark names: blob_read.post_capture, branch_merge.adopt_after_append_pre_upsert, branch_merge.adopt_after_upsert_pre_delete, branch_merge.adopt_between_insert_chunks, branch_merge.between_delete_chunks, classify.fresh_read, cleanup.resolve_branch_snapshot, init.after_coordinator_init, init.post_manifest_create, mutation.post_no_effect_pre_gate.
CI on this head: DST run 39711550558 success (DST pinned suite). Format (rustfmt), Lint (clippy), Test Workspace, and Dependency Guard (cargo deny) success in run 39711550552. GQ Logic Tests run 39711550485 success. The CI workflow is red because Azure Deployment Validation could not pull mcr.microsoft.com/azure-cli (connection reset by peer, exit 125). The CI Gate job succeeded. main 7fff9dfd CI succeeded (run 39709020616). CI does not run the census.
Regression proof. This delta changes a comment. GQT cannot express that comment. A test cannot fail before the change for a reason the comment causes. The reach contract under the merged engine is the census above. I added no temporary case.
Limits
- I ran the census once. Each universe is a pure function of its seed.
- I did not re-run the hunt or the fleet. This delta changes no recipe.
- I did not re-run the full DST crate locally. The pinned suite on this head succeeded in CI.
- This review makes no performance claims.
Findings
Blocking: none.
Optional: none.
Summary
The DST reach census (
dst_window_reach_probe, an ignored instrument no workflow runs) assertedcrossed.len() >= 48, a floor set when the crash-window catalog had 66 windows. Windows were removed since, so the census failed on every run, and a count could not say which window went dark. Reviewing that census and the dead-staging work from #1036 also turned up two reach defects in the same harness code, fixed here.Named unreachable windows
harness::REACH_EXPECTED_DARKlists each window no seeded universe can cross, with its reason.errored). The first seed that crosses wins, since the census asks whether a window is reachable. The floor caught an errored window only when the count dropped.expected_dark_windows_are_catalog_windows) keeps every listed name incatalog::CRASH_WINDOWSon every run.Each census universe is a pure function of its seed, so a change in reach comes from a code change, and no date or count needs upkeep.
init.after_coordinator_init,init.post_manifest_createdst_birth_contract_sweepowns themblob_read.post_capturemutation.post_no_effect_pre_gatebranch_merge.between_delete_chunksclassify.fresh_read,cleanup.resolve_branch_snapshotbranch_merge.adopt_after_append_pre_upsert,branch_merge.adopt_after_upsert_pre_delete,branch_merge.adopt_between_insert_chunksadopt_advances_head)No per-window ledger of the 2026-08-12 run was checked in, so that baseline could not be diffed; this list is that ledger from now on.
The dead-staging setup needs its aftermath
A recorded setup crash arms its target. The dead-staging window's setup was
ensure_indices.post_phase_b_pre_manifest_commit, which an index build with nothing to build also crosses (table_ops.rs, the no-work branch). In 12 of 30 hunt cells (seeds 1–30, skip 0), such a build recorded the setup without stranding any staging, and the target was then scheduled on a cleanup it could not reach. The setup is nowensure_indices.post_table_effect, which is crossed only after a detached index commit exists. With it, those 12 cells record no setup, and every cell that hit before still hits.dst_setup_crash_needs_its_aftermathpins seed 2, showing both behaviours.Index-build reach is structural
Index maintenance builds only missing indexes, and so does an
Optimize. A sampledOptimizebefore the recipe's index build therefore leftensure_indicesnothing to build. That had madeensure_indices.post_table_effectdark under its old recipe in all three census seeds, and it made the dead-staging setup miss. While a recipe still owes itsEnsureIndicesMain, the sampler now draws again instead of emitting anOptimize, so the first index build is anEnsureIndiceswith the fixture's unbuilt indexes. Over seeds 70001–70020, with the census's probe scenario:ensure_indices.post_table_effectcleanup.sweep_pre_dead_manifest_deleteOnly recipes containing
EnsureIndicesMain(theensure_indicesfamily and the dead-staging window) see a different op stream.Recipes
branch_merge.post_table_effectis crossed only when a merged table stages its own effect. A merge into an unchanged main isAdoptSourceState, which only switches pins;AdoptWithDeltaandAdoptPureInsertsneedadopt_advances_head, which no current branch satisfies. The window now shares the diverged-main recipe ofbranch_merge.rewrite_after_insert_pre_update, which forcesRewriteMerged.ensure_indices.post_table_effecttakes the family default (insert, then index build). Its old recipe named the milestone branch, but the harness runs every index build on main whatever branch the op names, so the branch was never the cause of it going dark; a sampledOptimizewas. TheEnsureIndicesBranchmilestone had no other user and is removed.milestone_stepsalso drives the nightly fleet's window arm, so these recipes apply there too.Smaller fixes
window_hitcheck, and the fleet arm's comment says a setup crash runs first.Evidence
Reach census (
cd crates/omnigraph-dst && cargo test --locked --test scenarios dst_window_reach_probe -- --ignored --nocapture).On main before this change (
fb984d21), which fails the old floor of 48:With this change:
Hunt (
cargo test --locked --test scenarios dst_hunt_crash_window_sweep -- --exact --ignored --nocapture):Fleet window arms (
DST_FLEET_SEEDS=1 DST_FLEET_SEED_BASE=<seed> cargo test --locked --test scenarios dst_fleet -- --exact --ignored --nocapture, seeds 10016, 10028 and 10039):Tests
cd crates/omnigraph-dst && cargo test --locked --no-fail-fast: lib 54 passed; integration 2 passed, 3 ignored; scenarios 56 passed, 28 ignored (main584f6f6emerged in)dst_setup_crash_needs_its_aftermath(scenarios) andcatalog::tests::expected_dark_windows_are_catalog_windows(lib).cargo fmt --all --check,cargo clippy -p omnigraph-dst --all-targets --locked -- -D warnings -W clippy::dbg_macro, the crate'sbanned_words_lint.cargo deny --locked checkpasses on the merged lockfile, which carries main'srustls-webpki0.103.16 andwebpki-roots/webpki-root-certs1.0.10 (RUSTSEC-2026-0381/0382/0383).