Skip to content

fix(mlxcel-core): make audio-path MLX ops fallible at the FFI boundary#384

Merged
inureyes merged 2 commits into
mainfrom
fix/issue-382-audio-mlx-ffi-fallible
Jun 21, 2026
Merged

fix(mlxcel-core): make audio-path MLX ops fallible at the FFI boundary#384
inureyes merged 2 commits into
mainfrom
fix/issue-382-audio-mlx-ffi-fallible

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

The audio synthesis and transcription forward paths build MLX graphs through mlxcel-core cxx ops declared as -> UniquePtr<MlxArray> (not Result). When the underlying MLX C++ throws (shape mismatch caught eagerly at graph build, allocation failure at eval), cxx hard-aborts the process via std::terminate. That is not a Rust panic, so the catch_unwind backstop in src/server/audio_worker.rs (run_guarded) cannot intercept it regardless of the release panic setting. This implements option (a) from the issue: add Result-returning variants of the MLX ops that dominate the audio forward path and thread them through the Kokoro/Whisper code, so a thrown MLX exception becomes a recoverable per-request Err instead of a process abort.

What changed

  • src/lib/mlxcel-core/src/lib.rs, cpp/mlx_cxx_bridge.h, cpp/mlx_cxx_bridge.cpp: add two fallible FFI variants. try_matmul(a, b) -> Result<UniquePtr<MlxArray>> mirrors matmul; because it is declared -> Result, cxx wraps the call in try/catch and converts a thrown MLX exception into a Rust Err. try_array_to_raw_bytes(arr) -> Result<Vec<u8>> mirrors array_to_raw_bytes (contiguous + eval + copy-out) inside one cxx try/catch boundary. The non-fallible siblings are unchanged, so no other model path is affected.
  • src/models/kokoro/ops.rs: add a fallible try_matmul wrapper, and rewrite to_vec_f32 (the single point where the lazy Kokoro graph is forced) to materialize through try_array_to_raw_bytes. This replaces the previous try_eval followed by a second, non-fallible eval inside array_to_raw_bytes, closing a real abort hole (an allocation failure making the contiguous copy of a large tensor was uncaught) and removing a redundant eval.
  • src/models/kokoro/mod.rs: route the two alignment-expansion matmuls (en = d_t @ aln, asr = t_en @ aln) through ops::try_matmul. Their inner dimension is the frame count T, derived from the runtime duration prediction, so these are the data-dependent construction ops on the synthesis path.
  • src/lib/mlxcel-core/src/ffi_tests.rs: regression tests for the new fallible ops.

Ops made fallible and why this set

MLX is lazy for the heavy ops, so the entire forward computation (every conv, the LSTM, the matmuls) and the allocation of the large T-dependent tensors are deferred to a single eval. Both audio paths force that graph at one materialization point: Kokoro at to_vec_f32, Whisper at argmax_with_masks. Routing the materialization through a fallible boundary therefore catches the eval-time fault set (allocation failure on the large data-dependent tensors, and any deferred op error) across the whole graph. MLX additionally validates some shapes eagerly at op construction, so the data-dependent alignment matmuls (whose inner dim comes from runtime data) get the fallible try_matmul variant to catch a graph-build throw too. This is the "minimize the number of new fallible variants" path the issue suggests: two new variants cover the realistic fault set on the synthesis path. Construction throws on the remaining weight-fixed ops require a checkpoint/config mismatch and are not reachable on the guarded forward path (input capped to 4096 chars, vocab-restricted g2p, 510-token truncation, finite/positive speed, per-token frame counts clamped to [1,100], whitelisted voices).

Transcription (Whisper) already forces its entire encoder and decoder graph at the single argmax_with_masks point through the pre-existing try_eval, so its eval-time faults are already recoverable. Its construction shapes are fixed by the 30 s mel window and the checkpoint config, and its matmuls live in shared mlxcel_core::layers used by every model, so they are intentionally left unchanged to keep this scoped to the audio path and avoid cross-cutting churn.

Test plan

  • cargo check -p mlxcel-core --lib (runs build.rs, compiles the C++ shim, validates the cxx bridge): clean.
  • cargo check -p mlxcel --lib --tests (server-side threading): clean.
  • cargo test -p mlxcel-core --lib ffi_tests::try_: 3 passed. try_matmul_returns_err_on_shape_mismatch asserts a deliberate (2,3) x (4,5) mismatch returns Err (graph-build throw caught, not a process abort); try_matmul_ok_matches_matmul asserts the happy path; try_array_to_raw_bytes_round_trips_f32 asserts the fallible readback round-trips.
  • cargo clippy -p mlxcel-core --lib --tests -- -D warnings and cargo clippy -p mlxcel --lib -- -D warnings: clean.
  • cargo fmt --check: clean.
  • No measurable throughput regression: the fallible variants add no eval/sync (try_matmul builds a lazy node; to_vec_f32 now does one fewer eval), so no per-op hot-path cost is expected. A full release build and the audio throughput bench were not run under the implementation watchdog; deferred to the release CI / maintainer.

Closes #382

An MLX C++ exception thrown from a non-Result cxx op aborts the process via std::terminate, which the catch_unwind backstop in the audio worker cannot intercept regardless of the release panic setting. Add Result-returning variants of the MLX ops that dominate the audio synthesis forward path and route the Kokoro path through them, so a thrown MLX exception becomes a recoverable per-request Err instead of a process abort.

try_matmul mirrors matmul but is declared -> Result, so cxx catches MLX's eager graph-build shape-mismatch exception (and any other throw) at the FFI boundary. The Kokoro alignment-expansion matmuls (en, asr), whose inner dimension is derived from the runtime duration prediction, are the data-dependent construction ops on the synthesis path and now use it. MLX validates shapes eagerly at op construction, so this catches a throw at graph build, not only at eval.

try_array_to_raw_bytes mirrors array_to_raw_bytes but is declared -> Result, so the contiguous copy, eval, and copy-out run inside one cxx try/catch. Kokoro's to_vec_f32 (the single point where the lazy graph is forced) now materializes through it, replacing the previous try_eval followed by a second, non-fallible eval inside array_to_raw_bytes. This closes a real abort hole (an allocation failure making the contiguous copy of a large data-dependent tensor was uncaught) and removes a redundant eval, so there is no added per-op hot-path overhead.

Transcription (Whisper) already forces its entire encoder and decoder graph at the single argmax_with_masks materialization point through try_eval, so its eval-time faults are already recoverable; its construction shapes are fixed by the 30 s mel window and the checkpoint config, and its matmuls live in shared mlxcel_core::layers used by every model, so they are intentionally left unchanged to keep the change scoped to the audio path.

Add FFI regression tests beside the bridge: try_matmul on mismatched shapes returns Err (the graph-build throw is caught instead of aborting the process), the happy path matches matmul, and try_array_to_raw_bytes round-trips a host buffer.
@inureyes inureyes added type:security Security vulnerability or fix priority:low Low priority area:models Model architectures, weights, loading, metadata area:inference Generation, sampling, decoding (incl. speculative, DRY) status:review Under review labels Jun 21, 2026
@inureyes

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

Make the audio synthesis/transcription forward path robust to MLX FFI C++ exceptions by adding Result-returning try_* cxx variants, so a C++ throw becomes a recoverable per-request Err instead of a std::terminate process abort (issue #382, option a).

Findings Addressed

None requiring a fix. No CRITICAL or HIGH findings.

Verification

  • All stated requirements implemented
  • No placeholder/mock code remaining
  • Integrated into project code flow
  • Project conventions followed
  • Existing modules reused where applicable
  • No unintended structural changes
  • Tests pass

Detail

  1. cxx Result mapping is correct. try_matmul and try_array_to_raw_bytes are declared -> Result in the bridge and implemented as non-noexcept C++ functions, so cxx wraps the call in its catch (const std::exception&) shim. MLX raises std::exception-derived types (std::invalid_argument on eager matmul shape validation, allocation failures at eval), which the shim converts to a Rust Err. try_matmul_returns_err_on_shape_mismatch (a deliberate (2,3)x(4,5) mismatch) is a genuine regression test that the throw is caught, not aborted.

  2. Error propagation is clean: ops::try_matmul(...).map_err(|e| anyhow!(e))? and to_vec_f32(...).map_err(|e| anyhow!(e))? on the real forward path (kokoro/mod.rs:230,234,254), then KokoroEngine::synthesize maps the anyhow error to AudioModelError::Inference (kokoro_tts.rs:166), with run_guarded catch_unwind as defense-in-depth. No .unwrap()/.expect() on the fallible calls in production (only in test code).

  3. Whisper exclusion is justified. Eval-time faults are already routed through the pre-existing try_eval at the single argmax_with_masks materialization point (decoding.rs:105), which forces the whole encoder+decoder graph. Construction-time shapes are fixed: input is always normalized to the 30s window (pad_audio_to_window_multiple + padded_window -> [1, N_FRAMES, n_mels]), token-embedding gather indices are bounded by argmax over n_vocab, and the decoder positional slice is bounded by offset + seq <= n_text_ctx (loop guard + max_new = n_text_ctx/2). The only residual construction throws require a checkpoint/config mismatch, which is the same load-time exclusion the issue grants the synthesis path. Unlike Kokoro, the Whisper frame dimension is not data-dependent, so there is no equivalent data-dependent construction op to wrap.

  4. No new hot-path overhead. try_matmul builds a lazy node identically to matmul. to_vec_f32 now materializes through one fallible try_array_to_raw_bytes (contiguous + eval + copy) instead of try_eval followed by a second eval inside array_to_raw_bytes, removing a redundant eval. The C++ contiguous(arr.inner) default matches the prior contiguous(f, false) (allow_col_major=false), so the readback bytes are byte-identical. No per-op sync/copy added.

  5. Conventions: tests in ffi_tests.rs, non-fallible siblings left intact for all other models, Closes #382 present, no AI attribution, no em dashes.

Remaining Items

  • Acceptance criterion 3 (no measurable throughput regression) is empirically deferred to release CI per the PR test plan; the static argument (no added eval/sync, one fewer eval) supports it but no audio throughput bench was run.

ADR 0003 Consequences section described the MLX C++ FFI exception path
(issue #382) as an open residual abort vector. PR #384 closes it by
routing the Kokoro alignment-expansion matmuls and the final PCM readback
through try_matmul / try_array_to_raw_bytes, both declared as Result<..>
in the cxx bridge so MLX throws become recoverable per-request Errs.

Update the Consequences bullet and the References line in ADR 0003, and
replace the open-residual paragraph in docs/audio-api.md with the current
containment status and its scope limits (Whisper unaffected by fixed-shape
invariants; non-std::exception throws still terminate).
@inureyes

Copy link
Copy Markdown
Member Author

PR Finalization

Tests: No new tests added. The three tests in src/lib/mlxcel-core/src/ffi_tests.rs (error on shape mismatch, Ok-matches-matmul, readback round-trip) cover the FFI behavior completely. The Kokoro ops wrappers (ops::try_matmul, ops::to_vec_f32) are one-liner format wrappers with no branching logic worth a separate test.

Documentation: Two files updated (commit 46a482912):

Lint/Format: cargo fmt --check -p mlxcel-core clean; cargo clippy -p mlxcel-core --lib --tests -- -D warnings clean; cargo clippy -p mlxcel --lib -- -D warnings clean.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Jun 21, 2026
@inureyes
inureyes merged commit 09e5c3d into main Jun 21, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-382-audio-mlx-ffi-fallible branch June 21, 2026 15:37
inureyes added a commit that referenced this pull request Jun 25, 2026
`try_conv1d` had an error-path test but no happy-path parity test. Add `try_conv1d_ok_matches_conv1d` mirroring `try_conv2d_ok_matches_conv2d`: valid NLC input [1,10,2] with weight [4,3,2] at stride 1 / no padding, verifying output shape, value identity against `conv1d`, and expected sum (192.0). Also apply `cargo fmt` to the existing `try_conv2d_ok_matches_conv2d` test (one `.expect()` chain was too wide for the line limit).

Update `docs/audio-api.md` and `docs/adr/0003-release-panic-unwind-with-core-thread-abort.md` to note that PR #434 extends the `try_matmul` / `try_array_to_raw_bytes` fallible-FFI pattern from PR #384 to `try_conv2d` / `try_conv1d` (issue #427, Gemma 4 Conformer audio encoder path).
inureyes added a commit that referenced this pull request Jun 25, 2026
…ults don't abort the server (#434)

* fix: make conv2d/conv1d fallible at the FFI boundary so conv shape faults don't abort the server

The Gemma 4 Conformer audio encoder calls conv2d/conv1d, which are declared in the cxx bridge returning UniquePtr<MlxArray> (not Result). An MLX shape-mismatch throw from a non-Result cxx op is converted to std::terminate and aborts the whole server process instead of failing the single request. This extends the try_* fallible-op pattern established by #384 (try_matmul) to convolution so a conv fault on the audio path degrades to a recoverable per-request error.

- Add try_conv2d/try_conv1d to the cxx bridge: Rust decls in lib.rs and C++ decls/impls in mlx_cxx_bridge.h/.cpp. The bodies are identical to conv2d/conv1d but the Rust signature is -> Result, so cxx wraps the call in try/catch and surfaces MLX's eager graph-build throw as a Rust Err instead of letting it cross the FFI boundary uncaught.
- Route the two data-dependent audio-encoder conv calls (SSCPConvBlock conv2d, ConformerLightConv1d depthwise conv1d) through the fallible variants and thread Result up the encoder forward chain (SSCPConvBlock, SubSampleConvProjection, ConformerLightConv1d, ConformerBlock, AudioEncoder::forward).
- Make the Gemma 4 audio embedding builders get_input_embeddings_with_audio and get_input_embeddings_with_audio_and_cache return Result and propagate the error at all request-path call sites (server/model_worker.rs, multimodal/vlm_runtime.rs, commands/generate_vlm.rs). The vision-only and video builders keep their infallible signatures because they pass audio_features = None, so the fallible audio branch is never reached.
- Leave statically-shaped conv calls unchanged (the Mamba fused conv1d in mlx_cxx_kernels.cpp, avg_pool2d's internal conv2d, and vision patch-embed conv, whose shapes are fixed by config), matching #384's scoping rationale of converting only the data-dependent audio-path calls.
- Add FFI tests beside the bridge mirroring the try_matmul tests: try_conv2d/try_conv1d return Err on a channel mismatch (the real Gemma 4 crash shapes for conv2d), and the conv2d happy path matches the non-fallible op in shape and value.

This is a defense-in-depth change; the happy path is byte-identical to the existing ops, so the FFI Err test (not a real checkpoint) is the correct validation that the crash is isolated.

Closes #427

* test: add try_conv1d ok parity test and extend fallible-conv docs

`try_conv1d` had an error-path test but no happy-path parity test. Add `try_conv1d_ok_matches_conv1d` mirroring `try_conv2d_ok_matches_conv2d`: valid NLC input [1,10,2] with weight [4,3,2] at stride 1 / no padding, verifying output shape, value identity against `conv1d`, and expected sum (192.0). Also apply `cargo fmt` to the existing `try_conv2d_ok_matches_conv2d` test (one `.expect()` chain was too wide for the line limit).

Update `docs/audio-api.md` and `docs/adr/0003-release-panic-unwind-with-core-thread-abort.md` to note that PR #434 extends the `try_matmul` / `try_array_to_raw_bytes` fallible-FFI pattern from PR #384 to `try_conv2d` / `try_conv1d` (issue #427, Gemma 4 Conformer audio encoder path).
inureyes added a commit that referenced this pull request Jun 25, 2026
* fix: route nemotron omni audio-encoder convs through fallible FFI

The Nemotron-H Nano Omni Parakeet/Conformer audio encoder built its data-dependent conv2d/conv1d ops through the non-fallible `mlxcel_core::conv2d`/`conv1d`. MLX validates conv shapes eagerly at graph-build time, so a crafted audio request whose runtime-derived post-subsampling shape mismatches the conv weights throws an uncaught C++ exception across the cxx boundary and `std::terminate`-aborts the whole server for all clients. This is the same remote-DoS class addressed for the Gemma 4 Conformer in #427/#434 and for the audio-synthesis matmul in #384.

Route the four data-dependent conv calls in `src/audio/nemotron_h_nano_omni/encoder.rs` through the already-existing `try_conv2d`/`try_conv1d` wrappers (added in #434), which are declared `-> Result` so cxx catches MLX's eager shape throw and returns it as a Rust `Err`: the subsampling conv2d stack and the convolution module's pointwise1, depthwise, and pointwise2 conv1d calls.

Thread `Result<_, String>` up the encoder forward chain so the error reaches a request boundary instead of an unwrap: `SubsamplingConv2DLayer::forward`, `ParakeetSubsamplingConv2D::forward`, `ParakeetConvModule::forward`, `ParakeetEncoderBlock::forward`, and the public `NemotronOmniSoundEncoder::forward` now return `Result`. The forward methods that never transitively call a conv (LayerNorm, BatchNorm, FeedForward, Attention, GLU/norm helpers) stay infallible.

The VLM embedding builder `NemotronHNanoOmniVlModel::extract_audio_features` already returns `Result`, so it propagates the encoder error with `?`; its request-path call site in `compute_nemotron_h_nano_omni_audio_embeddings` already maps it to an `anyhow` request error, so a conv shape fault now surfaces as a per-request `Err` rather than a process abort.

Add an encoder-level abort-safety test next to the encoder (`abort_safety_tests`) asserting that `SubsamplingConv2DLayer::forward` with a channel-mismatched weight returns `Err`, complementing the existing `try_conv2d`/`try_conv1d` FFI Err tests from #434. Update the three existing encoder-test call sites for the new `Result` return.

Closes #435

* test(audio): add conv1d abort-safety test and extend fault isolation docs

Add `conv_module_pointwise1_returns_err_on_channel_mismatch` to the abort_safety_tests module in the Nemotron-H Nano Omni encoder. It directly constructs a `ParakeetConvModule` with a 4-channel pointwise1 weight and feeds a 2-channel input, confirming the `try_conv1d` path returns `Err` on a shape fault rather than reaching `std::terminate`. Complements the existing conv2d test in the same module.

Extend the Fault isolation section of `docs/audio-api.md` and the residual-abort-vector entry in `docs/adr/0003-release-panic-unwind-with-core-thread-abort.md` to record that PR #439 (issue #435) applied `try_conv2d`/`try_conv1d` coverage to the Nemotron-H Nano Omni Conformer/Parakeet encoder, covering all four data-dependent conv calls on that path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:inference Generation, sampling, decoding (incl. speculative, DRY) area:models Model architectures, weights, loading, metadata priority:low Low priority status:done Completed type:security Security vulnerability or fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(server): make audio synthesis robust to MLX FFI exceptions (cxx std::terminate bypasses the panic backstop)

1 participant