fix(physics): memory safety + FFI hardening - #5
Conversation
Three FFI hygiene issues addressed together because they all live in the gravitas-wasm shim and shared the same root cause (the SAB pointer contract was never written down). The shadow-curve writer was unbounded and overflowed the PHYSICS block into TELEMETRY by 15 f32 slots, corrupting FPS and frame-index reads on the main thread. Cap the writer at SHADOW_CURVE_MAX_POINTS (56) with a const-time assert that the curve fits and a debug_assert per write. attach_sab accepted any pointer JS handed it. While today's code path runs on an internal Rust-owned buffer, future callers shouldn't be able to feed a null or misaligned address. Make it return Result with runtime checks, and document the SAFETY contract on the unsafe block in tick_sab. Also reword the CONTROL[1..3] consume-on-read comment so future readers understand the small race window (one frame's input lost worst case at 60-75 Hz).
The Rust panic hook in gravitas-wasm was exported but never invoked, so any panic inside the worker terminated it silently with the browser-generic "worker terminated" message instead of propagating the actual stack trace. Call init_hooks immediately after the dynamic import resolves and before constructing PhysicsEngine.
renormalize_null projects p_r onto the null-cone via a quadratic in p_r, so it relies on sqrt(B^2 - 4AC). Floating-point rounding over long integration runs can push that discriminant slightly negative; the previous code simply skipped the renormalize when it saw a negative value, leaving the caller with stale state on the wrong side of the null cone. Split the discriminant into three bands. Nonneg passes through unchanged. Values in [-1e-12, 0) clamp to zero (rounding noise, the geodesic is on the cone within machine precision). Anything below -1e-12 returns a NormalizationError; the integrator maps that to a new NormalizationFailure termination reason so trajectory consumers see a clean failure instead of integrating garbage.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request introduces error handling for renormalization failures in geodesic integration. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: caa096d37e
ℹ️ 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".
| return Trajectory { | ||
| final_state: state, | ||
| termination: TerminationReason::NormalizationFailure, | ||
| steps_taken: steps, | ||
| max_hamiltonian_drift: max_drift, |
There was a problem hiding this comment.
This return path runs after a full integrator step has already mutated state, but it exits before steps += 1 and before appending to path. As a result, when TerminationReason::NormalizationFailure occurs, final_state is one step ahead of both steps_taken and the recorded path (so path.last() can differ from final_state when record_path is enabled), which breaks trajectory bookkeeping for downstream analysis and debugging.
Useful? React with 👍 / 👎.
1 participant
Three small fixes to the physics engine and the worker bridge.
SAB shadow-curve overflow. The shadow-curve writer in
gravitas-wasmcould write 64 (x, y) points plus a 128-slot clear loop into the PHYSICS block, which only holds 128 f32 slots and reserves 16 for scalars. The 64-point write overflowed by 15 slots into TELEMETRY, corrupting the FPS counter and frame-index reads on the main thread. Capped at 56 points with a const-time assert and a debug_assert per write.FFI safety contract.
attach_sabaccepted any pointer JS handed it. Added null + 4-byte alignment checks (returnsResult) and documented the SAFETY contract on the unsafe block intick_sabso the invariants the caller must uphold are visible at the callsite.Worker panic hook.
init_hookswas exported fromgravitas-wasmbut never invoked, so any Rust panic in the worker was swallowed as a generic "worker terminated" message. Now called immediately after the WASM module loads.Renormalize discriminant.
renormalize_nullsolved a quadratic in p_r and silently no-op'd when the discriminant went slightly negative (rounding noise after long integration runs), leaving the caller with stale state on the wrong side of the null cone. Replaced with a three-band guard: nonneg passes through; values in[-1e-12, 0)clamp to zero; anything more negative returnsNormalizationError. The integrator maps the error to a newNormalizationFailuretermination reason.Test plan
cargo test --manifest-path physics-engine/Cargo.toml— cleanbun run test— 326/326 passbun run build:wasm— cleanphysics-engine/gravitas-wasm/tests/sab_bounds.rs(4 cases, layout invariants)physics-engine/gravitas-core/tests/normalize.rs(4 cases, three-band coverage)Summary by CodeRabbit
Bug Fixes
Improvements