Repository navigation
Conversation
The sweep sized the holders with grow_if_free on the first batch it received, while the csv producer, on its own blocking thread, was still releasing that slice's charges. Whether the tables were charged depended on that race, so a later query_memory_bytes refusal reported a different actual in two replays of one seed. The source now runs the sizing on the partition that publishes the row estimate, before that slice's batches leave it.
|
Recommendation: revise. A DST replay of one seed must report one history. Issue #1160 shows two This pull request moves unique-holder sizing onto the csv partition. Impact
Blocking
Optional
Tradeoff and liability
The new callback pins one optional charge to the producer. That charge is empty for the edge load that fails here. The write-load pair in #1160 differs by 456 bytes (399198 − 398742), which matches one holder table. This host ran Five more operators would add five observers. One DST-selected branch inside Pinned Lance 13.0.0 ValidationHost: Linux 6.12.94+, x86_64, 4 CPUs, rustc 1.97.1. Debug
Merge base
Head
Every attempt outcome was Ok. The 25600 step refused and the 32768 step admitted on every run. The failure is the cross-replay compare of Unit tests,
Limitations
Inline comment, Inline comment, Inline comment, |
WorkMemory::blocking_body polls every graph operator's body on a spawn_blocking thread, which DST's seeded current-thread runtime does not order. A csv load's producer and its sweep charged one memory pool from two such threads, so a query_memory_bytes refusal reported a different actual on two replays of one seed. A new thread-local seam, omnigraph_core::dst_blocking, makes blocking_body poll the body as one of the installing runtime's tasks; the DST universe installs it beside its clock and IDs. Production keeps spawn_blocking. This replaces sizing unique holders on the producing partition, which left the edge load's race untouched.
|
Revised as asked, at 62285bf, and the description is rewritten. Blocking 1 (the edge load keeps more than one actual). Agreed: the edge load has no unique holders, so the observer could not touch it. The observer is gone; Blocking 2 (the schedule belongs in At this head, with your loop at seed 0 for 60 runs: 0 mismatches among the 33 runs that completed, against your measured 16 of 60 on main. The other 27 exceeded the 10-second budget on an overloaded shared host; the same case on the plain Optional 1–4. All four concerned the observer. 1, 2 and 4 are moot now that it is removed. For 3, the mechanism test is replaced by Left open, stated in the description. The concurrent harness's actor threads do not install the seam; extending it to them is a separate decision for that harness. And the write-load pair from #1160 is not claimed. |
|
Recommendation: revise. A DST replay of one seed must report one Prior findingsBlocking 1 (the edge load keeps more than one actual): open. The actuals are the same pair. The path that produces them is new. See blocking finding 1. Blocking 2 (the schedule belongs in Optional 1–4: addressed. Each one described the observer. The observer is removed. The replacement test does not lock the actual. See optional finding 1. Blocking
Quiet loop, seed 0, no extra processes. Artifact timestamps:
That quiet result agrees with the reply (0 mismatches in 33 completed runs). The reply also reports 27 timeouts. A timeout is not a match. The loop that found this bug keeps three CPUs busy. The reply does not report that load. Busy loop, seed 0, three
Every attempt was Ok. The 25600 step refused. The 32768 step admitted. The failure is the compare. Both limits sit on one side of both actuals, so one GQT run passes when the two replays happen to match. The seam does put every pool charge on the universe thread. One busy mismatch was traced with a temporary log (isolated worktree, edits restored, nothing pushed). All 1500 pool lines used 25189 + 1096 = 26285. 25581 + 1096 = 26677. The gap is 392 bytes. Those bytes are
Optional
Tradeoff and liabilityThe seam is one primitive beside the three existing seams. The branch is a second mode. It skips the checkpoint hook, because that hook can wait on a condvar and the universe thread is the only thread. It holds the blocking guard across awaits. A panic unwinds, and the production path maps a The seam does not own wakes from other runtimes. Lance 13.0.0 still has a CPU runtime. No open pull request replaces that runtime. A later Lance deterministic mode can move The concurrent harness does not install the seam. That harness states no replay-identity claim. Leave that harness for a separate change.
ValidationHost: Linux, 4 CPUs, rustc 1.97.1, debug Command, once per run:
The base binary has no CI on this commit, run 40477751365: Local clippy was not run. The new unit test was not run. LimitationsThe busy sample is 40 runs. The mismatch count moved (15 at the base, 21 at this head). The actual set did not: 26677 and 26285. The quiet merge base also showed 26349 once. The head samples did not. The wake source was not isolated by disabling Lance's CPU runtime. No limit in the case sits between 26285 and 26677. The charge trace was a temporary log. Those edits are restored. Nothing was pushed. Recommendation: revise. Inline, Inline, Inline, Inline, Inline, |
|
Closing. #1187 fixed #1160 at the memory-admission design. A csv overwrite now admits its memory once and charges each parse partition against its own share in source order, so a refusal's The DST blocking seam is parked on #1161 as a ready implementation, in case a future failing case needs it. The branch stays on the fork at |
Closes #1160. Since #1158,
GQT (dst)fails intermittently withreplay_mismatch. Two replays of one seed report differentactualsizes for the samequery_memory_bytesrefusal: 26677, 26285 and 26349 across 60 runs ofload_csv_integer_endpoint_memoryat seed 0, with 16 of 60 mismatching atda1c4636. Every graph operator polls its body throughWorkMemory::blocking_body, which runs each poll intokio::task::spawn_blocking. DST seeds a current-thread runtime, but a blocking thread is outside it. A csv load's producer and itsValidateExecsweep therefore charge and release one memory pool from two OS threads, and a refusal reports whatever the pool held at an instant the seed does not order. This is the class #1161 describes, and this change removes it at that seam rather than at one operator, replacing this PR's first approach (sizing unique holders on the producer), which the review showed did not touch the edge load.omnigraph_core::dst_blockingis a new thread-local seam besidedst_gate,dst_clockanddst_ids. Without thedstfeature itson_runtime()inlines tofalse, so production polls exactly as before. The DST universe (omnigraph_dst::run_universe, the path GQT's DST runner takes) installsOnSeededRuntimeon its harness thread next to the logical clock and seeded IDs. When it is installed,blocking_bodypolls the body as one of the seeded runtime's own tasks. It keeps the cancellation check before every poll, the compute timer, the blocking-work probe and the cancellation disarm. It installs no worker checkpoint hook, because that hook is keyed by thread and every body now shares one;checkpoint()then only checks, as it does with no probe installed.spawn_blockingremains the production path and the only one outside DST: no other engine source calls it, soNeedleRows::build, the scan runtime filter and every other operator body are covered by the same branch. Lance's own CPU pool (spawn_cpu) stays outside the seed, as before; DST already runs it as one FIFO worker underLANCE_CPU_THREADS=1.blocking_bodies_run_on_the_seeded_runtime_under_the_dst_seam_issue_1160pins the mechanism: without the seam a body runs on a blocking thread, and with it the body, and a body it nests, run on the installing thread. Locally, at this head, 60 runs ofomnigraph-gqt load_csv_integer_endpoint_memory.gqt --target omnigraph-engine-dst --storage in-memory-object-store --seed 0: 0 replay mismatches among the 33 runs that completed. At the review's measured rate on main (about 27% of runs mismatch), seeing none in 33 has a probability of about 3 in 100,000. The other 27 exceeded the case's 10-second budget on a host shared with other sessions' builds and GQT runs (load average 10 to 13 during the loop). Later, at a load average of about 50, the same case on the plainomnigraph-enginetarget, which this change does not reach, timed out the same way, so the timeouts are the host's and CI's GQT jobs are the timing evidence. The DST pinned suite passes, including the storage-cost golden, with no regenerated line. Both clippy shapes pass on the pinned 1.97.1 toolchain: the shipped engine withoutdst, and the engine withdstplusomnigraph-dst.For review: the decision to challenge is running every operator body on the one DST thread. A body that blocked its thread synchronously would now stall the universe instead of one blocking worker; I found no such wait in engine sources outside tests. Unquiesced fleet runs lose engine-level body overlap too, which they made no replay claim about; they keep Lance's thread overlap. The concurrent harness's actor threads (
omnigraph-dst/src/concurrent.rs) install their own slots and do not install this one, so their bodies still run on blocking threads; extending the seam there is a separate decision for that harness. The write-load pair in #1160 (398742 and 399198) did not reproduce on the reviewer's host in 90 runs, so this PR claims only the edge load; it shares the same seam.