[azimafroozeh]

A DST replay that disagrees with its first attempt fails as replay_mismatch: <environment> seed=<n>; both reports retained, which only says that the two reports differ; finding the field means downloading the invocation artifact and diffing two JSON documents. #1178 went red once on exactly this line.

The line now names the first field whose JSON text differs (code, phase, input_digest, result, then observations and evidence entry by entry) and shows both values around their first differing character, or the two lengths when one list is a prefix of the other. The comparison is unchanged: a field differs exactly when its serialized text differs, the test the runner applied to the whole report before, so no replay changes verdict.

Three unit tests pin the field order and line shapes, the agreement with the byte comparison over eight variants (negative zero and object key order among them), and the window at and past the bound.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-26T16:05:04.274909Z We couldn’t complete this review. Comment @codex review to try again.	bbc1bbd	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[ragnorc]

Recommendation: ready.

A DST replay that disagrees with its first run used to print only replay_mismatch: <environment> seed=<n>; both reports retained. The reader had to download two JSON reports to find the field. Pull request 1178 failed once on that line.

The line now names the first field whose JSON text differs. The order is code, phase, input_digest, result, then observations and evidence, one entry at a time. The line shows both values around the first different character. When one list is a prefix of the other, the line shows the two lengths.

The match result is the same for the current report. A field differs when its serialized text differs. That is the same test as the old whole-report byte comparison. The comparison still skips measurements. Three unit tests pin the field order, the byte agreement, and the window.

The practical effect is a mismatch you can read without the artifact files. A matching replay still passes. The helper is about 90 lines in dst_runner.rs. The tests sit in the existing action_tests module. No other crate compares these reports. The workspace and serde_json 1.0.150 have no JSON first-difference helper. This file is the right owner.

Blocking

None.

Optional

1. Keep the serialized bytes as the verdict. replay_difference repeats the report fields by hand. The walk matches json of the stripped report for every current field. A new field must be copied in comparable, or the crate does not compile. Nothing requires that field in the walk. A difference in that field alone prints replay matched. Compare the two json(&comparable(...)) buffers first. Build the field text only when those buffers differ. The common match path then stays one serialization per report. The recorded-replay door also keeps the 256 MiB bound inside json. replay_attempts calls replay_difference directly, so a rerun whose JSON exceeds that bound no longer fails as report_failed: encode.

2. comparable used to return those bytes. It now returns the struct. The two concurrent replay tests compare it with ==. PartialEq for serde_json::Value treats -0.0 and 0.0 as equal, and it ignores object key order. The serialized text differs in both cases. serde_json/preserve_order is on because cedar-policy-core enables it. The workspace manifest requests only float_roundtrip. Those two tests no longer lock the relation the runner uses. Compare json(&comparable(...)) again.

3. Commit 6c8f5953 (oops) ignores the new zstd advisories and moves zstd-sys from 2.0.16 to 2.1.2. The bundled C library stays zstd 1.5.7. zstd 0.13.3 and zstd-safe 7.2.4 do not move, so the bump does not clear the advisories. Remove the lockfile edit, or record why it is required. The ignore rows match the call sites I checked. The only direct dependents are arrow-ipc 58.3.0 and lance-encoding 13.0.0. Neither calls with_context. copy_decode builds a fresh Decoder. The summary sentence is narrower than the code. Production also reuses one bulk Compressor across chunks and calls zstd_safe::compress_bound. A unit test calls Encoder::new. Neither path is with_context. Name those calls in the comment. The pull request body does not mention this commit.

Checked questions

The per-field walk matches the whole-report bytes for the current report shape. I compared 17 report pairs at the merge base and at bbc1bbd. The byte result was the same on every row. The rows cover object key order, negative zero, 0 versus 0.0, an extra object key, a list prefix, an empty list, a combining character, a 4-byte character at the window, an 8000 character tail, and a result change beside a later list change.

A mismatch line always names a field. replay_difference returns Some only from a format string that includes the field name. A difference the walk misses becomes replay matched. It does not become a mismatch with no name. When code and result both differ, the line names code. When result and a later list both differ, the line names result.

The window indexes Unicode scalar values, not bytes. A difference on U+1F525 stays one character in the line. A JSON \u00XX escape can still split. A NUL after 238 q characters rendered as \u and then the ellipsis. The verdict still uses the full text. A very long value does not change the verdict. The 8000 character pair differed, and the line was 124 characters.

Tradeoff

This removes the artifact diff for the first field. It adds a second copy of the report schema. Five more report fields would each need a line in the walk. A missed line turns a real mismatch into a pass. The serialized document is already the one comparison. Derive the line from that document.

The current walk is correct for the fields that exist. The cost is the extra list, not the file it lives in. A shared JSON helper would have one caller and the same list problem unless the bytes stay the verdict. I did not measure speed. On a match, the walk serializes every evidence entry. The old code serialized each report once.

Other open pull requests

No other open pull request edits dst_runner.rs. 1178, 1180, 1176, 1168, and 1181 do not touch this file. 1178 is the failure this line is for. 1180 changes the GQT build script only. 1176 and 1181 can change stored evidence. They do not change this comparison. A later Lance release does not change the walk. The zstd exemption stays until Arrow and Lance leave zstd 0.13.

Validation

Merge base: da1c46368c33876d61615add0ceb6778f9ce2b58.
Tests ran at bbc1bbdde8a0d6714acbcd91896d0627c83c628d.
Reviewed head: 6c8f5953b0363dcc7ffd115b490414876ee02859.
git diff bbc1bbd 6c8f595 -- crates/omnigraph-gqt/src/dst_runner.rs is empty. The second commit edits deny.toml and Cargo.lock only.

At the first head, with a temporary matrix that was removed and not pushed:

CARGO_TARGET_DIR=/tmp/og-target cargo test -p omnigraph-gqt --locked --lib -- replay_difference review_probe concurrent_replay -- --test-threads=8

Result: 6 passed, 0 failed. That run is the three new tests, the two concurrent replay tests, and the matrix.

Same tree with replay_difference forced to return None: 4 failed, 0 passed. replay_difference_names_the_first_differing_field_and_entry expected Some("result differs: ...") and got None. replay_difference_agrees_with_the_serialized_report_comparison got walker false and byte difference true.

At the merge base, same filter, temporary matrix removed and not pushed: 3 passed, 0 failed. The 17 byte rows match the head rows. negative_zero and key_order are bytes=diff and partial=eq at both commits.

CI on bbc1bbd: GQT ordinary, GQT dst, GQT clippy, rustfmt, and Test Workspace passed. CI Gate failed because cargo deny reported the zstd advisories. That commit did not change the lockfile.

CI on 6c8f595 when this review was posted: Dependency Guard passed. GQT dst-clippy passed. Other jobs were still running. I did not rerun the unit tests after the zstd-sys bump.

The Codex comment on this pull request is a failed review with no findings. There were no other reviews and no inline comments.

Limits

I did not run the full GQT corpus on this machine. GQT cannot express this diagnostic line. The Rust tests are the right owner. I did not build a report larger than 256 MiB. I did not time the match path. The temporary matrix is not in the branch.

Inline comment on crates/omnigraph-gqt/src/dst_runner.rs line 494: This walk repeats the report fields. It matches json of the stripped report for the fields that exist now. A new field copied in comparable is not in this list. A difference in that field alone becomes replay matched. Compare json(&comparable(first)) with json(&comparable(second)) first. Build this text only when the bytes differ.

Inline comment on crates/omnigraph-gqt/src/dst_runner.rs line 2738: This == is PartialEq of the struct. It treats -0.0 and 0.0 as equal, and it ignores object key order. The runner compares serialized text, and that text differs in both cases. Compare json(&comparable(...)) here, as these tests did before this change.

Inline comment on deny.toml line 31: lance-encoding 13.0.0 also reuses one bulk Compressor across chunks (compress_to_buffer in encodings/physical/block.rs) and calls zstd_safe::compress_bound. A unit test in that file calls Encoder::new. I found no call to with_context, Seekable, or around_pos in arrow-ipc 58.3.0 or lance-encoding 13.0.0. Name the bulk reuse and the test encoder here so the exemption matches the call sites. The zstd-sys 2.0.16 to 2.1.2 lock bump does not change zstd 0.13.3 or zstd-safe 7.2.4, and it does not clear these advisories.

[cursor]

Recommendation: ready.

The diagnostic change from the review of 6c8f5953 is unchanged. This note covers the new commit only.

Why

A DST replay of one seed must report one actual for one query_memory_bytes refusal. On 6c8f5953, GQT (dst) failed load_csv_integer_endpoint_memory.gqt. Run 0 reported actual 26285. Run 1 reported actual 26677. The limit was 25600 in both. The gap is 392 bytes.

That 392 is the endpoint key set for the 8 Person keys in the case. IntKeySetLayout for 8 keys uses 16 slots. The scan input is 64 bytes. The retained set is 200 bytes. The build scratch is 128 bytes. Those three charges sum to 392.

AdmittedSets already states the contract. The loader admits every byte of that set before the parse. The scan input and the build scratch stay until the parse ends. A refusal then replays one actual. ValidateExec::execute broke the contract. It called input.execute before admit_sets.

What the new commit does

d5b81ffb (fix flaky) moves source.execute to after admit_sets. The key-set charges finish before the CSV source charges. A refused set still precedes the parse.

This is production behavior. It is not a retry, a sleep, or a test-only change. A load that fits the pool still returns the same rows. The error name stays query_memory_bytes. The stable actual for this case is 26677, which includes the key set.

Cause

CsvSourceExec::execute charges the source bytes and then starts the producer. DataFusion 54.0.0 RecordBatchReceiverStreamBuilder::spawn calls JoinSet::spawn, so the producer runs before execute returns. On 6c8f5953 that start happened before admit_sets. The parse could charge the pool before the key set, or after it. SpillHeadroomPool sets actual to reserved + additional at the refused try_grow. The key set is in that sum only when admission wins the race.

The new order closes that window in ValidateExec, which owns the admission. The CSV producer now starts after admit_sets returns. admit_sets includes Dataset::count_rows(None). In Lance 13.0.0 that count uses the fragment row count when the manifest stores it. It does not scan column values in that case. The key read in load_sets still runs beside the drain.

What this commit does not fix

size_unique_holders still calls grow_if_free when the first batch arrives. The producer can still release that slice on its own task. PR 1174 names that race. Its gap is 456 bytes, one holder table of 8 buckets. This commit does not move that charge. write_max_bytes_removed_id_charge can still disagree across two replays.

Other open pull requests

PR 1174 edits this execute. It installs an estimate observer and then calls input.execute before admit_sets. That is the order this commit removes. The two fixes fit in one order. Admit the sets. Install the observer. Then execute. A rebase that calls execute first brings the 392-byte race back.

PR 1178 also edits validate.rs. At c976c592 it still calls input.execute before admit_sets. A rebase must keep admission first.

No other open pull request edits dst_runner.rs.

Prior optional findings

These three are not addressed. git diff 6c8f5953 d5b81ffb does not change dst_runner.rs, deny.toml, or Cargo.lock.

1. The field walk can miss a future report field. The walk is still a hand list. A new field in comparable is still absent from the walk.
2. The two concurrent replay tests still compare the struct with ==. That is still PartialEq, not the serialized bytes.
3. The zstd-sys 2.0.16 to 2.1.2 bump still does not clear the advisories. zstd 0.13.3 and zstd-safe 7.2.4 do not move.

Optional

1. The note in load_csv_integer_endpoint_memory.gqt says the refusal is 26,289 bytes. At this head the case reports actual 26677 on the local engine and on both DST replays. Update the note.

2. The commit message and the pull request body do not say that load admission changed. This is engine behavior inside a diagnostic pull request. Name it in the body, or move the commit to the pull request that owns the load-memory race. If it stays, PR 1174 must not put execute back before admit_sets.

Tradeoff

The change removes a schedule-dependent actual. It adds one wait. The CSV producer starts after the endpoint row count and the three key-set charges. Five more charges of this kind belong before source.execute, in this same function. A new type would not hold that order more firmly than the call order does. The liability removed is a replay that can pass or fail with no data change. The liability added is a rebase hazard with PR 1174 and PR 1178.

Validation

Merge base da1c46368c33876d61615add0ceb6778f9ce2b58. Reviewed head d5b81ffbec2e9ac539e2fc8c69a0eec1010157ce. The parent of the new commit is 6c8f5953b0363dcc7ffd115b490414876ee02859.

Unit test engine::operators::validate::tests::the_sets_are_charged_before_the_source_partitions_are_executed at the head:

cargo test -p omnigraph-engine --lib engine::operators::validate::tests::the_sets_are_charged_before_the_source_partitions_are_executed -- --exact

Result: 1 passed.

The same assertion on the 6c8f5953 execute, grafted in an isolated checkout and not pushed, failed. The charge ledger shows CSV source bytes and CSV decoding before load integer endpoint keys.

load_csv_integer_endpoint_memory.gqt, 24 parallel processes, same command at both trees:

cargo test -p omnigraph-gqt --test gq_logic_tests load_csv_integer_endpoint_memory -- --test-threads=1

On the 6c8f5953 operator sources: 4 of 24 failed. Each failure is observations[7] with actual 26677 against actual 26285. The passing runs used both values.

On d5b81ffb: 0 failed. 24 local attempts and 48 DST attempts all report actual 26677.

CI on 6c8f5953: GQT (dst) in run 40362830307 failed this case with that same pair. CI on d5b81ffb: GQT (dst) in run 40373710420 passed. The other checks on that head later completed with success. The tag jobs were skipped.

Limits

I did not run the full GQT corpus. I did not run write_max_bytes_removed_id_charge. I did not time count_rows on a large endpoint table. The grafted assertion was removed. Nothing was pushed.

Inline comment on crates/omnigraph/src/engine/operators/validate.rs line 165: source.execute starts the CSV producer. DataFusion 54.0.0 starts that task with JoinSet::spawn before execute returns. Keep admit_sets before this call.

PR 1174 and PR 1178 both call input.execute before admit_sets. A rebase that restores that order brings back the 392-byte actual race (26285 against 26677). Admit the sets, install the PR 1174 observer, then execute.