engine: reconcile mid-run getValue across VM and wasm (#625)#632
Conversation
After a runTo(t) that stops mid-interval, both executors' integration
loops break on curr[TIME] > target *after* an advance, so the live curr
chunk held the resting-point stocks and time but stale non-stock slots:
the VM's wholesale curr.copy_from_slice(next) (and chunk-ring advance)
left flow/aux/constants stale -- a mid-run getValue of a constant read 0 --
and the wasm advance copies only the stock offsets, leaving flow/aux one
step behind. A mid-run getValue therefore agreed between backends only for
stocks and the reserved time vars.
Re-evaluate root flows once at the resting curr after the loop, on both
backends, so the live curr chunk is fully self-consistent ("the value at
the current time") and identical VM-vs-wasm for every variable.
The re-eval touches only the live curr chunk: every results row was
already saved (and the VM's get_series reads chunks [0, curr_chunk),
excluding this one), a resumed run_to re-evaluates from scratch, and a
full runToEnd reads the last results row -- so saved series, resume, and
full-run getValue are unchanged. It does not re-snapshot prev_values, so a
resumed run's PREVIOUS still sees the last completed step. The full-run
and reset break paths already leave a freshly-evaluated curr, so the
re-eval is idempotent there (a resumed run_to on a full slab stays a
no-op).
Tests: a Rust mid-run parity + self-consistency test (a flow-phase aux
doubled = level*2 exposes the one-step lag), and the TS AC2.2 parity test
widened from stocks+time to every variable.
![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: 82193345f7
ℹ️ 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".
| // to resume, and to a full run. It does NOT re-snapshot `prev_values`, so a | ||
| // resume's `PREVIOUS` still sees the last completed step. | ||
| { | ||
| let (curr, next) = borrow_two(&mut data, n_slots, self.curr_chunk, self.next_chunk); |
There was a problem hiding this comment.
The new unconditional post-loop borrow_two call can panic when run_to(end) is invoked with end beyond the simulation horizon (a supported clamp case): in save_advance!, the chunk-exhaustion branch sets curr_chunk = next_chunk and breaks before advancing next_chunk, so borrow_two is later called with equal indices and slices out of bounds. This turns a previously successful run_to(very_large_time) into a runtime crash in the VM path.
Useful? React with 👍 / 👎.
|
Review: mid-run getValue parity (#625) Verdict: not correct — the new VM post-loop re-eval panics when [P1]
The new re-eval block calls Minor: the "idempotent for the full-run/run/reset paths because they break with a freshly-evaluated curr already" note (vm.rs:867-869, module.rs:1252-1253) is imprecise: on a full |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #632 +/- ##
==========================================
- Coverage 83.58% 83.57% -0.02%
==========================================
Files 275 276 +1
Lines 75395 75419 +24
==========================================
+ Hits 63019 63029 +10
- Misses 12376 12390 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Both PR #632 reviewers flagged a panic: the post-loop root-flow re-eval added for #625 called borrow_two(curr_chunk, next_chunk) unconditionally, but run_to(target) with target past FINAL_TIME exits the loop via the chunk-ring exhaustion break in save_advance!, which sets curr_chunk = next_chunk before breaking. borrow_two with two equal chunk indices slices left[a*n_slots..(a+1)*n_slots] out of a left of length a*n_slots and panics -- aborting across the FFI boundary on what was previously a graceful clamp (e.g. an unclamped simlin_sim_run_to). Guard the re-eval on curr_chunk != next_chunk. The exhausted-slab case is exactly the one a mid-interval read never reaches (a full slab means time has reached FINAL_TIME, not mid-interval), so skipping the re-eval there is correct and restores the pre-#625 graceful behavior. Every normal run_to (mid-interval included) breaks via the curr[TIME] > end path with distinct chunks, so the re-eval still runs and #625 stays fixed. Also correct the imprecise "idempotent for the full-run path" note: on the VM a full run leaves curr on the next-written overshoot chunk (stale flow/aux), so the re-eval does change it there -- but that chunk is beyond the saved results, so it stays invisible to a full run. The wasm note is tightened too (it does break with a consistent curr, and has no chunk aliasing since curr is the fixed CURR_BASE region). Adds a VM regression test (run_to 10x past the end returns Ok, not panic). The new test pushed vm.rs over the 6000-line file lint, so the vm_reset_and_run_initials_tests module is extracted to a sibling file included via #[path] (the existing vm_set_value_tests.rs pattern), so use super::* still resolves vm's private items. The residual VM-vs-wasm divergence for run_to(past FINAL_TIME) (VM overshoots the slab; wasm clamps) is tracked as #634 -- not reachable via in-range scrubbing.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Code reviewI traced both backends' break paths and the [P2] wasm post-loop
|
PR #632 review (Claude) found a wasm-only regression: the post-loop flows(0) re-eval added for #625 ran unconditionally, including after a full / at-stop run. A full run breaks via the slab-exhaustion path, which does NOT advance curr -- curr is the just-saved t=stop row, and prev_values was already snapshotted to that same row (the per-step snapshot runs after the step's flows). The re-eval then resolved PREVIOUS(x) against curr's own snapshot, yielding x(stop) instead of x(stop-dt), corrupting the live curr that the host's getValue reads. So getValue of a PREVIOUS-using variable after a full run diverged from the committed series and from the VM (which reads the last results row). Guard the re-eval on saved < n_chunks: skip it exactly on the break paths that do not advance curr (the full-slab exhaustion break and the resumed full-slab top guard), where curr is already the correct last-saved row. This mirrors the VM's curr_chunk != next_chunk guard ("re-eval only when curr was advanced"). The mid-interval case (saved < n_chunks, curr advanced) still re-evals, and there prev_values holds the genuinely previous step, so PREVIOUS resolves correctly -- #625 stays fixed, including for PREVIOUS auxes. Skipping also makes a resumed run_to on a full slab a strict no-op rather than an idempotent re-eval. The VM needs no change here: its full run advances curr (it breaks via curr[TIME] > end after the advance, distinct chunks), so its re-eval reads a genuine previous snapshot, and getValue after run_to_end reads the results row regardless. Tests: a new full-run regression (a PREVIOUS aux's live curr must equal the last committed series row and the VM, = level at t-dt, not t), and the mid-run parity test's fixture gains a PREVIOUS aux so the all-variable mid-interval comparison covers it too.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Review (Claude)Reviewed the mid-run
Confirmed full-run getValue parity is unaffected: The residual Overall correctness: correct. No blocking issues found. |
1 participant
Summary
Closes #625. After a
runTo(t)that stops mid-interval, the bytecode VM and the wasm blob left different content in their livecurrchunk for non-stock variables, so a mid-rungetValue(name)agreed between backends only for stocks + reserved time vars. Verified still live onmain(post-#630) and actually worse than the issue described -- the VM returns 0 for constants mid-run:getValuegetValueteacup_temperatureroom_temperaturecharacteristic_timeheat_loss_to_roomdtbehind)Per the chosen resolution (reconcile, not document), this makes mid-run
getValueself-consistent and identical across backends for every variable.Root cause
Both integration loops break on
curr[TIME] > targetafter the save+advance tail. The advance steps time and integrates stocks but does not recompute flow/aux/constants for the advanced time:curr.copy_from_slice(next)(and the chunk-ring advance) landscurron a chunk whose non-stock slots are stale -- 0 for a constant the overshoot chunk never held.emit_save_advancecopies only the stock offsetsnext -> curr, leaving flow/aux one step behind the advanced time + stocks.So
curr's stocks/time are at the resting point but its non-stock slots are not -- internally inconsistent on both, and divergent between them.Fix
After the
run_toloop, re-evaluate root flows once at the restingcurron both backends (vm.rsreuses the sameStepPart::Flowseval the RK paths already use;wasmgen/module.rsemits oneflows(0)call after the loop). The livecurrchunk becomes fully self-consistent -- "the value at the current time" -- and byte-identical VM-vs-wasm for every variable.Well-scoped, by construction:
get_seriesreads chunks[0, curr_chunk), excluding the resting chunk.runToEndreads the last results row, and its break path already leaves a freshly-evaluatedcurr, so the re-eval is idempotent (a resumedrun_toon a full slab stays the engine: move wasm sim live-state ownership into the blob #630 no-op).run_tore-evaluates from scratch, and the re-eval does not re-snapshotprev_values, soPREVIOUSstill sees the last completed step.Applies uniformly to Euler/RK2/RK4. No TS/host change -- the host just reads
curr.Tests (TDD)
wasmgen/module.rs:mid_run_curr_is_self_consistent_and_matches_vm-- a flow-phase auxdoubled = level * 2exposes the one-step lag; asserts every var's mid-runcurrequals the VM'sget_value_now, and thatdoubled == level * 2at the resting time (20, not the lagged 16). Watched fail first (inflow_ratewasm=2 vs vm=0).wasm-backend.test.ts: AC2.2 widened from stocks+time to every variable. Watched fail first (constant diff of 10) against the pre-fix wasm, green after the rebuild.Full pre-commit gate passes:
cargo test --workspace(incl. the always-on corpuswasm_parity_hook),pnpm -C src/engine test(412), clippy, tsc, pysimlin.