Batch eval-loop-round-2: resident GPU and symbolic followups - #409
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ae0b3cc to
5fcdd18
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (27)
crates/runmat-plot/src/plots/line.rs (1)
118-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win[low] Allowing empty line plots now exposes non-finite statistics ranges.
With empty data now valid, callers of
statistics()can receivex_range=(inf,-inf)andy_range=(inf,-inf), which can leak stale/non-finite state into UI/debug paths. Add an explicit empty-data fast path instatistics().💡 Proposed fix
pub fn statistics(&self) -> PlotStatistics { + if self.x_data.is_empty() || self.y_data.is_empty() { + return PlotStatistics { + point_count: 0, + x_range: (0.0, 0.0), + y_range: (0.0, 0.0), + memory_usage: self.estimated_memory_usage(), + }; + } let (min_x, max_x) = self .x_data .iter()As per path instructions,
**/*asks to prioritize stale state propagation findings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-plot/src/plots/line.rs` around lines 118 - 127, The statistics() method in the LinePlot struct needs to handle empty data to prevent returning non-finite ranges. Add an explicit check at the beginning of the statistics() method that detects when both x_data and y_data are empty, and return an appropriate early value (such as default/zero ranges or None) before any calculations that would produce infinite values. This prevents non-finite state from leaking into UI and debug paths when callers invoke statistics() on empty line plots.Source: Path instructions
crates/runmat-runtime/src/builtins/io/audio/mod.rs (1)
691-737: 🎯 Functional Correctness | 🟠 Major[high] PCM decoding reads incorrect bytes for 24-valid-in-32-bit-container samples.
When
bits_per_sample = 32andvalid_bits_per_sample = 24, the code computesbytes_per_channel = 4(line 638) correctly from the container width. However,decode_wave_sample()matches oneffective_bits_per_sample() = 24, triggering the(0x0001, 24)arm (line 724), which callssign_extend_24(bytes)on the 4-byte slice.The problem:
sign_extend_24(line 739–740) readsbytes[0], bytes[1], bytes[2], the low three bytes. In a 32-bit PCM container with 24 valid bits, the standard convention is left-justified: the 24 valid bits occupy bytes[1, 2, 3](little-endian), with byte[0]as zero-padding. Reading the low three bytes skips byte[3]and incorrectly includes padding, producing wrong amplitude and sign.The fix: for the
(0x0001, 24)case when the input slice is 4 bytes, read bytes[1, 2, 3]instead, or detect the 24-in-32 scenario explicitly and extract the upper 24 bits from the 32-bit container.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/io/audio/mod.rs` around lines 691 - 737, The decode_wave_sample() function incorrectly handles 24-valid-bits-in-32-bit-container PCM samples. In the (0x0001, 24) match arm, when the input slice contains 4 bytes (indicating a 32-bit container), the code calls sign_extend_24(bytes) which reads bytes [0, 1, 2] but should read bytes [1, 2, 3] to correctly extract the left-justified 24-bit valid bits from the 32-bit container. Fix this by checking if the input slice is 4 bytes long in the (0x0001, 24) case and extract the upper 24 bits from the 32-bit integer value, or pass the correct slice [1, 2, 3] to sign_extend_24 instead.Source: Path instructions
crates/runmat-runtime/src/builtins/math/elementwise/real.rs (1)
393-399: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[medium] Use the registered WGPU provider instead of ambient global state.
Ignoring the registration result and then calling
provider().unwrap()can run this test against a stale provider if registration fails or another test mutates global provider state.Proposed fix
fn real_wgpu_complex_matches_cpu() { - let _ = runmat_accelerate::backend::wgpu::provider::register_wgpu_provider( + let _guard = test_support::accel_test_lock(); + let Ok(provider) = runmat_accelerate::backend::wgpu::provider::register_wgpu_provider( runmat_accelerate::backend::wgpu::provider::WgpuProviderOptions::default(), - ); - let provider = runmat_accelerate_api::provider().unwrap(); + ) else { + return; + }; let complex = ComplexTensor::new(vec![(1.0, 2.0), (-3.0, 4.5)], vec![2, 1]).unwrap();As per path instructions, prioritize race conditions and stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/math/elementwise/real.rs` around lines 393 - 399, In the test function real_wgpu_complex_matches_cpu, the code is discarding the result of register_wgpu_provider and then calling the global provider().unwrap() which retrieves ambient global state. This can cause the test to run against a stale provider if registration fails or if another test mutates the global provider state. Instead, capture the registered provider directly from the register_wgpu_provider call and use that provider instance for the subsequent operations like gpu_helpers::upload_complex_tensor, eliminating reliance on the global provider() function.Source: Path instructions
crates/runmat-runtime/src/builtins/math/elementwise/gamma.rs (1)
520-526: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift[high] Carry the handle provider through the gather fallback.
When
complex_from_realfails, the recursion gathers from the original provider but reuploads host complex values through ambientprovider(). In multi-provider runs,"like"can return a GPU value on the wrong backend.Suggested direction
- convert_to_gpu_complex(gathered).await + convert_to_gpu_complex_with_provider(gathered, Some(handle_provider)).awaitThen have the
ComplexandComplexTensorbranches prefer that provider before falling back torunmat_accelerate_api::provider().As per path instructions, prioritize stale state propagation and async ordering issues.
Also applies to: 537-555
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/math/elementwise/gamma.rs` around lines 520 - 526, In the error handler for `complex_from_real` at the location where `gpu_helpers::gather_value_async` is called, the subsequent `convert_to_gpu_complex` call uses the ambient provider which can place GPU values on the wrong backend in multi-provider scenarios. Pass the `handle_provider` through to `convert_to_gpu_complex` by modifying its signature to accept a provider parameter, then update the Complex and ComplexTensor branches within `convert_to_gpu_complex` to prefer the passed provider before falling back to `runmat_accelerate_api::provider()`. Apply the same pattern to the related code at lines 537-555 that handles similar complex conversion logic.Source: Path instructions
crates/runmat-runtime/src/builtins/acceleration/gpu/gpuarray.rs (1)
714-754: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[high] Preserve the new precision after complex
single→doublereupload.This branch reuploads F32 complex handles as double, but the caller later prefers
incoming_precision, so the new handle can be stamped back to F32. Prefer the converted handle’s metadata before the old input metadata.Proposed fix
- let final_precision = requested_precision - .or(incoming_precision) - .unwrap_or(provider_precision); + let converted_precision = runmat_accelerate_api::handle_precision(&handle); + let final_precision = requested_precision + .or(converted_precision) + .or(incoming_precision) + .unwrap_or(provider_precision);As per path instructions, prioritize stale state propagation and use [high] for CI-blocking issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/acceleration/gpu/gpuarray.rs` around lines 714 - 754, The complex F32 handle reupload branch correctly converts the handle to double precision via upload_complex_host_value, but the new handle's precision metadata is being lost because the caller later reverts it to the original incoming_precision. Ensure that after the prepared handle is returned from upload_complex_host_value in the complex value branch, its converted precision metadata (double) is preserved and prioritized over the original incoming_precision, preventing any subsequent code from stamping the handle back to F32.Source: Path instructions
crates/runmat-runtime/src/builtins/math/signal/pwelch.rs (1)
1317-1323: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[medium] Keep the accel test lock alive.
Line 1323 shadows
_guard, which drops theaccel_test_lock()guard immediately after provider setup. The rest of the WGPU test can still race with parallel tests that mutate provider state.🐛 Suggested fix
- let _guard = crate::builtins::common::test_support::accel_test_lock(); + let _accel_guard = crate::builtins::common::test_support::accel_test_lock(); @@ - let _guard = runmat_accelerate_api::ThreadProviderGuard::set(Some(provider)); + let _thread_provider_guard = runmat_accelerate_api::ThreadProviderGuard::set(Some(provider));As per path instructions, prioritize race conditions and stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/math/signal/pwelch.rs` around lines 1317 - 1323, The variable name `_guard` is used twice in the test function, and the second assignment on line 1323 shadows the first assignment from line 1317, causing the accel_test_lock guard to be dropped immediately. Rename the second `_guard` variable (the one assigned from `ThreadProviderGuard::set`) to a different name like `_provider_guard` to ensure both guards remain in scope and the accel test lock stays alive for the entire test duration, preventing race conditions with parallel tests.Source: Path instructions
crates/runmat-runtime/src/builtins/array/creation/meshgrid.rs (2)
1289-1290: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Free the real GPU output after converting it to complex.
When a real GPU meshgrid output is gathered and re-uploaded as complex, the original provider output handle is no longer returned but is never freed. This leaks GPU buffers on complex-
likeGPU outputs.🧹 Suggested cleanup
} else { let tensor = gpu_helpers::gather_tensor_async(handle).await?; - to_complex_gpu_tensor_value(tensor_to_complex_tensor(tensor)?) + let result = to_complex_gpu_tensor_value(tensor_to_complex_tensor(tensor)?); + if let Some(provider) = runmat_accelerate_api::provider_for_handle(handle) { + provider.free(handle).ok(); + } + result }As per path instructions, prioritize rollback inconsistencies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/array/creation/meshgrid.rs` around lines 1289 - 1290, The code gathers a real GPU tensor using gpu_helpers::gather_tensor_async(handle) and then converts it to complex via tensor_to_complex_tensor and to_complex_gpu_tensor_value, but the original provider output handle is never freed, causing a GPU buffer leak. After the tensor is gathered and converted to complex, explicitly deallocate the original handle to prevent GPU memory leaks on complex-like GPU outputs in the meshgrid function.Source: Path instructions
722-736: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win[high] Accept scalar gathered GPU axes.
gather_value_asynccan return scalarValue::Num/Value::Complexfor GPU scalar axes, but this fallback only accepts tensor variants. That makes complex GPU scalar axes error even though host scalar axes are accepted above.🐛 Suggested fix
match gathered { Value::Tensor(tensor) => { if is_vector_shape(&tensor.shape) { *prefer_gpu = true; } axis_from_tensor(tensor, index) } + Value::LogicalArray(logical) => { + let tensor = tensor::logical_to_tensor(&logical)?; + axis_from_tensor(tensor, index) + } Value::ComplexTensor(tensor) => { if is_vector_shape(&tensor.shape) { *prefer_gpu = true; } axis_from_complex_tensor(tensor, index) } + Value::Num(n) => Ok(AxisData { + values: vec![(n, 0.0)], + len: 1, + is_complex: false, + gpu_real: None, + }), + Value::Complex(re, im) => Ok(AxisData { + values: vec![(re, im)], + len: 1, + is_complex: im != 0.0, + gpu_real: None, + }),As per path instructions, prioritize nullability bugs and stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/array/creation/meshgrid.rs` around lines 722 - 736, The match statement handling the gathered GPU value only accepts tensor variants (Value::Tensor and Value::ComplexTensor) but gather_value_async can also return scalar variants (Value::Num and Value::Complex). Add match arms for the scalar cases Value::Num and Value::Complex before the catch-all other arm, similar to how scalar GPU axes are handled for the host case above. For each scalar variant, call the appropriate axis conversion function (axis_from_tensor for numeric scalars, axis_from_complex_tensor for complex scalars) with the scalar value and index parameter.Source: Path instructions
crates/runmat-runtime/src/builtins/comms/pskmod.rs (2)
78-80: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Only use a provider that owns this GPU handle.
If
provider_for_handlemisses, the generic provider fallback may point at another device and then receivehandle.buffer_idfrom a foreign provider.🛡️ Suggested guard
let provider = runmat_accelerate_api::provider_for_handle(&handle) - .or_else(runmat_accelerate_api::provider) + .or_else(|| { + runmat_accelerate_api::provider() + .filter(|candidate| candidate.device_id() == handle.device_id) + }) .ok_or_else(|| pskmod_error("pskmod: no acceleration provider registered"))?;As per path instructions, prioritize stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/comms/pskmod.rs` around lines 78 - 80, The issue is that the code chain uses a fallback to a generic provider when provider_for_handle fails, which may point to a different GPU device than the one associated with the handle. This can cause handle.buffer_id to be used with the wrong provider. Remove the or_else clause that calls runmat_accelerate_api::provider in the provider resolution chain, so that only the provider specific to the handle (from provider_for_handle) is used. If provider_for_handle returns None, the operation should fail with the appropriate error rather than falling back to a potentially mismatched provider.Source: Path instructions
93-107: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Do not treat every provider modulation failure as fallback-safe.
if let Ok(out)drops all provider errors, so device loss or a kernel bug silently becomes host execution and re-upload. Fall back only for explicit unsupported/validation cases; propagate execution failures.🛡️ Suggested direction
- if let Ok(out) = provider.modulate_constellation(request).await { - return Ok(gpu_helpers::complex_gpu_value(out)); - } + match provider.modulate_constellation(request).await { + Ok(out) => return Ok(gpu_helpers::complex_gpu_value(out)), + Err(err) if provider_modulation_is_fallback_safe(&err) => {} + Err(err) => { + return Err(pskmod_error(format!( + "pskmod: GPU provider modulation failed: {err}" + ))); + } + }Apply the same handling to
modulate_bits_constellation.As per path instructions, prioritize async ordering issues and stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/comms/pskmod.rs` around lines 93 - 107, The code currently drops all errors from both provider.modulate_constellation and provider.modulate_bits_constellation method calls using if let Ok patterns, causing execution failures like device loss or kernel bugs to silently fall back to host execution. Instead of catching all errors uniformly, differentiate between validation or unsupported feature errors (which are safe to fallback on) and actual execution failures (which must be propagated). Replace the blanket if let Ok error handling with explicit error matching that only allows fallback for unsupported or validation error cases while propagating any execution failures from both modulation calls.Source: Path instructions
crates/runmat-runtime/src/builtins/math/signal/sinc.rs (1)
220-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[high] Preserve complex storage metadata on provider success.
For
ComplexInterleavedinput,unary_sincreturns a complex result, butresident_gpu_value(out)does not force the output handle back toComplexInterleaved. Providers that return a fresh handle without metadata will be gathered downstream as real data.🐛 Suggested fix
async fn sinc_gpu(handle: GpuTensorHandle) -> BuiltinResult<Value> { + let input_complex = + runmat_accelerate_api::handle_storage(&handle) == GpuTensorStorage::ComplexInterleaved; if let Some(provider) = runmat_accelerate_api::provider_for_handle(&handle) { match provider.unary_sinc(&handle).await { - Ok(out) => return Ok(gpu_helpers::resident_gpu_value(out)), + Ok(out) if input_complex + || runmat_accelerate_api::handle_storage(&out) + == GpuTensorStorage::ComplexInterleaved => + { + return Ok(gpu_helpers::complex_gpu_value(out)); + } + Ok(out) => return Ok(gpu_helpers::resident_gpu_value(out)),As per path instructions, prioritize stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/math/signal/sinc.rs` around lines 220 - 223, The `resident_gpu_value(out)` call in the success case of the `unary_sinc` provider match does not preserve complex storage metadata from the input handle. For ComplexInterleaved input handles, ensure that the output handle returned from the provider is explicitly marked or forced back to ComplexInterleaved storage type before being wrapped with resident_gpu_value. Modify the Ok(out) branch to propagate the storage metadata from the input handle parameter to the result to prevent downstream code from misinterpreting complex data as real data.Source: Path instructions
crates/runmat-runtime/src/builtins/math/linalg/ops/ctranspose.rs (2)
371-372: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Reject fallback providers that do not own the handle.
Both lookups can fall back to the current/global provider even when it has a different
device_idfromhandle. That can run transpose, gather fallback re-upload, or free operations against a foreign buffer ID.🛡️ Suggested provider guard
if let Some(provider) = - runmat_accelerate_api::provider_for_handle(&handle).or_else(runmat_accelerate_api::provider) + runmat_accelerate_api::provider_for_handle(&handle).or_else(|| { + runmat_accelerate_api::provider().filter(|candidate| { + candidate.device_id() == handle.device_id + }) + }) {Apply the same guarded lookup before the host-fallback re-upload.
As per path instructions, prioritize stale state propagation.
Also applies to: 459-460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/math/linalg/ops/ctranspose.rs` around lines 371 - 372, The provider lookup at the ctranspose operation is using an unsafe fallback that can execute operations on foreign buffers. The issue is in the `or_else(runmat_accelerate_api::provider)` call which falls back to a global provider even when it has a different device_id than the handle being processed. Remove this unsafe fallback and ensure the provider is only obtained from `provider_for_handle(&handle)` which guarantees ownership of the buffer. Apply the same guarded lookup pattern at the second location mentioned (line 459-460) in the host-fallback re-upload section to maintain consistency and prevent stale state propagation.Source: Path instructions
415-429: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win[critical] Do not free transpose aliases of the input handle.
transposed_handleis freed whenever it does not aliasconjugated, and also onunary_conjfailure. If the provider implements transpose/permute as a metadata alias ofhandle, this frees the caller-owned input and the fallback then gathers a stale handle.🛡️ Suggested alias guard
+ let aliases_input = transposed_handle.device_id == handle.device_id + && transposed_handle.buffer_id == handle.buffer_id; let aliases_transposed = conjugated.device_id == transposed_handle.device_id && conjugated.buffer_id == transposed_handle.buffer_id; - if !aliases_transposed { + if !aliases_transposed && !aliases_input { provider.free(&transposed_handle).ok(); } @@ } Err(err) => { - provider.free(&transposed_handle).ok(); + let aliases_input = transposed_handle.device_id == handle.device_id + && transposed_handle.buffer_id == handle.buffer_id; + if !aliases_input { + provider.free(&transposed_handle).ok(); + }As per path instructions, prioritize stale state propagation and rollback inconsistencies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/math/linalg/ops/ctranspose.rs` around lines 415 - 429, The code incorrectly frees transposed_handle in two places without checking if it aliases the caller-owned input handle. In the success path where aliases_transposed is false, and in the error handler after unary_conj failure, add a guard to ensure transposed_handle is not freed if it is an alias of the original input handle. Check whether transposed_handle device_id and buffer_id match the input handle (similar to the existing aliases_transposed check with conjugated), and only call provider.free on transposed_handle if it does not alias the input, preventing the caller-owned input from being prematurely freed and leaving stale handles for the fallback path.Source: Path instructions
crates/runmat-runtime/src/builtins/comms/qammod.rs (2)
91-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Do not swallow provider modulation failures.
Both provider calls fall through on any error. That hides device loss or shader failures behind a host fallback and can return a successful resident output after the GPU path actually failed.
🛡️ Suggested direction
- if let Ok(out) = provider.modulate_bits_constellation(request).await { - return Ok(gpu_helpers::complex_gpu_value(out)); - } + match provider.modulate_bits_constellation(request).await { + Ok(out) => return Ok(gpu_helpers::complex_gpu_value(out)), + Err(err) if provider_modulation_is_fallback_safe(&err) => {} + Err(err) => { + return Err(qammod_error(format!( + "qammod: GPU provider bit modulation failed: {err}" + ))); + } + }Apply the same pattern to
modulate_constellation.As per path instructions, prioritize async ordering issues and stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/comms/qammod.rs` around lines 91 - 105, The provider modulation calls modulate_constellation and modulate_bits_constellation are silently swallowed on error using the if let Ok pattern, which hides GPU failures and allows the function to return success despite provider errors. Instead of continuing execution when these provider calls fail, propagate the errors by using the ? operator (or explicitly returning the Err) so that GPU device loss or shader failures are properly surfaced to the caller rather than being masked by a fallback path.Source: Path instructions
76-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Only use a provider that owns this GPU handle.
The generic provider fallback can select a provider whose
device_iddoes not matchhandle.device_id, sending a foreign buffer ID into modulation, gather fallback, and re-upload.🛡️ Suggested guard
let provider = runmat_accelerate_api::provider_for_handle(&handle) - .or_else(runmat_accelerate_api::provider) + .or_else(|| { + runmat_accelerate_api::provider() + .filter(|candidate| candidate.device_id() == handle.device_id) + }) .ok_or_else(|| qammod_error("qammod: no acceleration provider registered"))?;As per path instructions, prioritize stale state propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-runtime/src/builtins/comms/qammod.rs` around lines 76 - 78, The issue is that the fallback to the generic provider via the or_else clause can select a provider whose device_id does not match the handle.device_id, causing foreign buffer IDs to be sent to modulation. Remove the or_else(runmat_accelerate_api::provider) fallback and only use the provider returned by provider_for_handle(&handle), ensuring that only a provider that owns the specific GPU handle is used. If no matching provider is found for the handle, the error should be returned rather than falling back to a potentially incompatible provider.Source: Path instructions
crates/runmat-accelerate/src/backend/wgpu/provider/ops/elementwise.rs (2)
93-99: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[medium] Preserve transpose metadata on complex constructor and binary outputs.
These paths produce lane-wise outputs from the input physical order but do not record existing transpose metadata. Downstream downloads/unary ops can then interpret transposed complex data in physical order.
🐛 Proposed fix shape
runmat_accelerate_api::set_handle_storage( &out, runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved, ); + if let Some(info) = runmat_accelerate_api::handle_transpose_info(real) { + runmat_accelerate_api::record_handle_transpose( + &out, + info.base_rows, + info.base_cols, + ); + } out @@ - Ok(handle) + if let Some(info) = runmat_accelerate_api::handle_transpose_info(a) { + runmat_accelerate_api::record_handle_transpose(&handle, info.base_rows, info.base_cols); + } + Ok(handle)As per path instructions,
**/*: “Prioritize catching stale state propagation.”Also applies to: 153-159, 1123-1124, 1239-1243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/backend/wgpu/provider/ops/elementwise.rs` around lines 93 - 99, The complex constructor and binary output paths are not preserving transpose metadata from input tensors when creating outputs. When setting the GpuTensorStorage for the output handle in these sections, also preserve any transpose metadata from the input by copying the transpose state to the output handle. This applies to multiple locations in the file where ComplexInterleaved storage is being set and should be fixed consistently across all affected code paths to prevent downstream operations from misinterpreting transposed complex data.Source: Path instructions
68-72: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[medium] Use effective storage for complex constructor input checks.
These gates still trust only handle sidecar storage. If
BufferEntry.storageis alreadyComplexInterleaved, the constructors can treat interleaved lanes as real inputs and produce corrupt double-interleaved output.🐛 Proposed fix
ensure!( - runmat_accelerate_api::handle_storage(real) - != runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved, + self.effective_storage_for_entry(real, &entry) + != runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved, "complex_from_real requires a real-valued input" ); @@ ensure!( - runmat_accelerate_api::handle_storage(real) - != runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved - && runmat_accelerate_api::handle_storage(imag) - != runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved, + self.effective_storage_for_entry(real, &entry_real) + != runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved + && self.effective_storage_for_entry(imag, &entry_imag) + != runmat_accelerate_api::GpuTensorStorage::ComplexInterleaved, "complex_from_real_imag requires real-valued inputs" );As per path instructions,
**/*: “Prioritize catching stale state propagation.”Also applies to: 109-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/backend/wgpu/provider/ops/elementwise.rs` around lines 68 - 72, The input validation for the complex_from_real constructor (and similar checks in the surrounding code) is using handle_storage which only checks sidecar storage and misses cases where BufferEntry.storage is already ComplexInterleaved. Replace the storage validation to check the effective storage from BufferEntry.storage instead of just handle_storage to prevent treating interleaved lanes as real inputs and producing corrupt output. This same fix should be applied to all similar validation checks in the function including the ones around line 109-115.Source: Path instructions
crates/runmat-accelerate/src/backend/wgpu/provider/ops/comms.rs (2)
249-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[high] Cap
bits_per_symbolto the shader accumulator width.The WGSL path stores
symbolinu32and shifts once per bit. Values above 32 can wrap the symbol and can also create extremely long per-invocation loops before validation returns.🐛 Proposed fix
ensure!( request.input_rows > 0 && request.bits_per_symbol > 0, "modulate_bits_constellation: invalid bit grouping" ); + ensure!( + request.bits_per_symbol <= u32::BITS as usize, + "modulate_bits_constellation: bits_per_symbol exceeds shader accumulator width" + ); ensure!( request.input_rows.is_multiple_of(request.bits_per_symbol), "modulate_bits_constellation: bit rows must be a multiple of bits_per_symbol" );As per path instructions,
**/*: “Prioritize catching async ordering issues” and “Be aggressive about medium/high/critical severity findings.”Also applies to: 171-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/backend/wgpu/provider/ops/comms.rs` around lines 249 - 265, The `bits_per_symbol` value needs to be capped to 32 to prevent overflow when used in the WGSL shader path, where the symbol is stored in a u32 and shifted once per bit. Add an additional ensure! check in the modulate_bits_constellation function validation block that verifies request.bits_per_symbol does not exceed 32, placing it with the other parameter validation checks. Apply the same constraint check to the other location mentioned at lines 171-184 where similar validation occurs.Source: Path instructions
36-61: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[medium] Bound all shader lane indexes to
u32.
logical_len <= u32::MAXis not enough because the shader writesidx * 2uand reads constellation lanes viasymbol * 2u. Reject requests where the interleaved output length or constellation lane count exceedsu32::MAX.🐛 Proposed fix
let order = request.constellation.len() / 2; ensure!( - order <= u32::MAX as usize, + request.constellation.len() <= u32::MAX as usize && order <= u32::MAX as usize, "modulate_constellation: constellation too large" ); @@ let out_len = logical_len .checked_mul(2) .ok_or_else(|| anyhow!("modulate_constellation: output length overflow"))?; + ensure!( + out_len <= u32::MAX as usize, + "modulate_constellation: output too large" + );As per path instructions,
**/*: “Be aggressive about medium/high/critical severity findings.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/backend/wgpu/provider/ops/comms.rs` around lines 36 - 61, The current validation in the modulate_constellation function checks that logical_len fits in u32, but this is insufficient because the shader performs operations using idx * 2u and constellation lane indexing with symbol * 2u. Add additional bounds checks to ensure that the doubled values do not overflow u32: after calculating out_len via the checked_mul operation, add an ensure statement to verify out_len does not exceed u32::MAX as usize, and before the order calculation, add an ensure statement to verify that request.constellation.len() * 2 does not exceed u32::MAX as usize (or equivalently that request.constellation.len() is at most u32::MAX as usize / 2) to prevent shader lane index overflow.Source: Path instructions
crates/runmat-accelerate/src/simple_provider.rs (7)
3915-3924: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[medium] Snapshot storage before releasing the registry data snapshot.
unary_real,unary_imag, andunary_conjclone data, release the registry lock, then read handle storage. A concurrentfreecan clear metadata in between and reclassify complex lane data as real.As per path instructions, "Prioritize catching: race conditions, stale state propagation, async ordering issues, auth edge cases, nullability bugs, rollback inconsistencies."
Proposed fix pattern
Box::pin(async move { + let storage = runmat_accelerate_api::handle_storage(a); let data = { let guard = registry().lock().unwrap(); guard .get(&a.buffer_id) .ok_or_else(|| anyhow::anyhow!("buffer not found: {}", a.buffer_id))? .clone() }; - if runmat_accelerate_api::handle_storage(a) != GpuTensorStorage::ComplexInterleaved { + if storage != GpuTensorStorage::ComplexInterleaved { return Ok(self.allocate_tensor(data, a.shape.clone())); }Also applies to: 3939-3947, 4017-4025
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 3915 - 3924, The code acquires a registry lock to retrieve buffer data, then releases the lock before calling runmat_accelerate_api::handle_storage to read the storage metadata. This creates a race condition where a concurrent free operation can clear or reclassify the buffer metadata between the lock release and the handle_storage call. Move the handle_storage call inside the lock guard scope in the unary_real, unary_imag, and unary_conj methods (at lines 3915-3924, 3939-3947, and 4017-4025) so that the metadata snapshot is captured while still holding the registry lock, preventing stale state propagation.Source: Path instructions
1801-1809: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[medium] Validate coefficient storage length before complex
polyint.The complex branch only checks even lane count, then derives coefficient count from the buffer length. If shape/storage metadata is stale, this silently integrates the wrong number of coefficients and returns the wrong logical shape.
As per path instructions, "Prioritize catching: race conditions, stale state propagation, async ordering issues, auth edge cases, nullability bugs, rollback inconsistencies."
Proposed fix
}; let storage = runmat_accelerate_api::handle_storage(polynomial); + ensure_storage_len("polyint", &coeffs, &polynomial.shape, &storage)?; match storage { GpuTensorStorage::Real => { let integrated = poly_integral_real(&coeffs, constant);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 1801 - 1809, The ComplexInterleaved branch in the storage matching block calls poly_integral_complex_interleaved with the coefficients buffer without first validating that the buffer length matches the expected coefficient count based on the polynomial's shape and storage metadata. Before calling poly_integral_complex_interleaved in the GpuTensorStorage::ComplexInterleaved arm, add validation to verify that the coeffs buffer length is consistent with the polynomial's expected dimensions. This prevents silently processing incorrect coefficients if the storage metadata is stale or mismatched.Source: Path instructions
6831-6847: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win[medium] Add independent edge-case expectations for complex trig.
These assertions use the same helper functions as the implementation, so they cannot catch helper bugs like large-imaginary
0 * infNaNs. Add literal edge-case checks after fixing the helpers.Proposed test addition
assert_complex_close( &provider, &tan, &values.map(|(re, im)| tan_complex_host(re, im)), &[1, 2], ); + + let edge = complex_handle(&provider, &[(0.0, 800.0)], &[1, 1]); + let sin_edge = block_on(provider.unary_sin(&edge)).expect("sin edge"); + let host = block_on(provider.download(&sin_edge)).expect("download sin edge"); + assert_eq!(host.shape, vec![1, 1]); + assert_eq!(host.storage, GpuTensorStorage::ComplexInterleaved); + assert!(!host.data[0].is_nan()); + assert_eq!(host.data[0], 0.0); + assert!(host.data[1].is_infinite());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 6831 - 6847, The current test assertions for sin, cos, and tan complex functions rely on the same helper functions (sin_complex_host, cos_complex_host, tan_complex_host) as the implementation being tested, making them unable to catch bugs within those helpers such as large-imaginary 0 * inf NaN issues. After the existing assert_complex_close calls for each of the three trigonometric functions, add explicit literal edge-case assertions that directly specify expected values for problematic inputs rather than computing them through the helper functions, allowing the tests to independently verify correct behavior for edge cases like very large imaginary components and special value combinations.
3704-3710: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win[high] Preserve complex storage in
unary_tanh.For
ComplexInterleavedinput,abufhas two lanes per logical element, but line 3710 marks the doubled output asRealwith the original shape. The next storage-aware op will see a shape/data-length mismatch.As per path instructions, "Prioritize catching: race conditions, stale state propagation, async ordering issues, auth edge cases, nullability bugs, rollback inconsistencies."
Proposed fix
Box::pin(async move { + let storage = runmat_accelerate_api::handle_storage(a); let guard = registry().lock().unwrap(); let abuf = guard .get(&a.buffer_id) .ok_or_else(|| anyhow::anyhow!("buffer not found: {}", a.buffer_id))?; - let out: Vec<f64> = abuf.iter().map(|&x| x.tanh()).collect(); + let out: Vec<f64> = if storage == GpuTensorStorage::ComplexInterleaved { + ensure!( + abuf.len() % 2 == 0, + "unary_tanh: complex-interleaved buffer has odd length" + ); + let mut out = Vec::with_capacity(abuf.len()); + for pair in abuf.chunks_exact(2) { + let two_re = 2.0 * pair[0]; + let two_im = 2.0 * pair[1]; + let inv_cosh = 1.0 / two_re.cosh(); + let denom = 1.0 + two_im.cos() * inv_cosh; + out.push(two_re.tanh() / denom); + out.push(two_im.sin() * inv_cosh / denom); + } + out + } else { + abuf.iter().map(|&x| x.tanh()).collect() + }; drop(guard); - Ok(self.allocate_tensor_with_storage(out, a.shape.clone(), GpuTensorStorage::Real)) + Ok(self.allocate_tensor_with_storage(out, a.shape.clone(), storage)) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 3704 - 3710, The unary_tanh function does not preserve the input tensor's storage type when processing ComplexInterleaved data. When the input buffer has ComplexInterleaved storage, the buffer contains two lanes per logical element (real and imaginary), but the output is incorrectly marked as GpuTensorStorage::Real with the original shape, causing a mismatch between the actual data length and the declared shape. Fix this by detecting the input tensor's storage type (check a.storage or similar), and when it is ComplexInterleaved, apply the tanh operation appropriately to preserve the complex structure and allocate the result tensor with the matching storage type (likely GpuTensorStorage::ComplexInterleaved) rather than forcing it to GpuTensorStorage::Real.Source: Path instructions
826-860: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[medium] Check the shape product before applying the lane factor.
product(&src_shape)andproduct(&kernel_dst_shape)can overflow before later checks run, so a stale or malformed shape can wrap the total and drive incorrect indexing/allocation.As per path instructions, "Prioritize catching: race conditions, stale state propagation, async ordering issues, auth edge cases, nullability bugs, rollback inconsistencies."
Proposed fix
- let total = product(&src_shape) + let logical_total = logical_len_for_shape("permute", &src_shape)?; + let total = logical_total .checked_mul(lane_factor) .ok_or_else(|| anyhow!("permute: shape/product exceeds supported size"))?; @@ - let dst_total = product(&kernel_dst_shape); + let dst_total = logical_len_for_shape("permute", &kernel_dst_shape)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 826 - 860, The code checks for overflow when computing the total size with lane_factor multiplication but does not check for overflow when computing the product of kernel_dst_shape. The product function call that calculates dst_total can overflow and wrap around, leading to incorrect memory allocation in the out vector. Add an overflow check using checked_mul or similar mechanism for the product(&kernel_dst_shape) calculation before it is used to allocate the output vector, ensuring that any overflow results in an error rather than silent wraparound.Source: Path instructions
118-132: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win[high] Guard complex trig helpers against
0 * infNaNs.For inputs like
0 + 800i, the exact zero trig factor is multiplied bysinh/cosh(large)(inf), producingNaNin lanes that should be signed zero or infinity.sinc_complex_hosthas the samenum_re/num_impattern.Proposed fix
+fn mul_zero_inf_safe(lhs: f64, rhs: f64) -> f64 { + if lhs == 0.0 && rhs.is_infinite() { + lhs * rhs.signum() + } else { + lhs * rhs + } +} + fn sinc_complex_host(re: f64, im: f64) -> (f64, f64) { if im == 0.0 { return (sinc_scalar_host(re), 0.0); } let scaled_re = std::f64::consts::PI * re; let scaled_im = std::f64::consts::PI * im; - let num_re = scaled_re.sin() * scaled_im.cosh(); - let num_im = scaled_re.cos() * scaled_im.sinh(); + let num_re = mul_zero_inf_safe(scaled_re.sin(), scaled_im.cosh()); + let num_im = mul_zero_inf_safe(scaled_re.cos(), scaled_im.sinh()); let denom_norm = scaled_re.mul_add(scaled_re, scaled_im * scaled_im); ( (num_re * scaled_re + num_im * scaled_im) / denom_norm, (num_im * scaled_re - num_re * scaled_im) / denom_norm, ) } fn sin_complex_host(re: f64, im: f64) -> (f64, f64) { - (re.sin() * im.cosh(), re.cos() * im.sinh()) + ( + mul_zero_inf_safe(re.sin(), im.cosh()), + mul_zero_inf_safe(re.cos(), im.sinh()), + ) } fn cos_complex_host(re: f64, im: f64) -> (f64, f64) { - (re.cos() * im.cosh(), -re.sin() * im.sinh()) + ( + mul_zero_inf_safe(re.cos(), im.cosh()), + -mul_zero_inf_safe(re.sin(), im.sinh()), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 118 - 132, The sin_complex_host, cos_complex_host, and sinc_complex_host functions can produce NaN values when exact zero factors (like sin(0) or cos(0)) are multiplied by infinite values (like cosh/sinh of large imaginary parts). Guard these functions against the 0 * inf case by checking when the real or imaginary components produce exact zeros, and handle the multiplication with large hyperbolic values explicitly to return properly signed zero or infinity instead of NaN. Apply this fix to the num_re and num_im computations in all three functions to ensure correct behavior for edge cases like inputs with zero real parts and large imaginary parts.
2810-2823: 🎯 Functional Correctness | 🔴 Critical[high] Bit-group symbol left-shift can overflow on unchecked bits_per_symbol.
The
symbol = (symbol << 1)operation lacks overflow protection. Ifbits_per_symbol >= 64on a 64-bit system (or >= 32 on 32-bit), the loop silently drops high bits. Since the function only validatessymbol < orderafter construction, an overflowed symbol might still pass that check and map to the wrong constellation point—silently corrupting the output.While
pskmod.rsderivesbits_per_symbolsafely viaorder.trailing_zeros(),simple_provider.rsaccepts it as untrusted input with only basic validation (> 0and divisibility checks). The overflow must be caught explicitly.Proposed fix
- symbol = (symbol << 1) | rounded as usize; + symbol = symbol + .checked_mul(2) + .and_then(|value| value.checked_add(rounded as usize)) + .ok_or_else(|| { + anyhow!( + "modulate_bits_constellation: bit group exceeds host symbol width" + ) + })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/simple_provider.rs` around lines 2810 - 2823, The symbol construction loop in modulate_bits_constellation uses an unchecked left-shift operation that can silently overflow when bits_per_symbol is too large (>= 64 on 64-bit systems or >= 32 on 32-bit), causing high bits to be dropped and the final symbol to be incorrect. Add explicit overflow protection to the bit-shifting loop by checking that the left-shift operation on the symbol variable will not overflow before performing it, either by validating that bits_per_symbol is within safe bounds at the function entry point or by using checked arithmetic during the symbol construction loop. This ensures that invalid inputs are caught early rather than silently producing corrupted constellation points.crates/runmat-accelerate/src/backend/wgpu/provider/ops/signal.rs (1)
27-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[medium] Compute fallible sizes before allocating GPU handles.
window,spectrum, orselectedcan already be resident when these checked length calculations returnErr, so invalid oversized spectral requests leak intermediate GPU buffers. Moveframed_len,selected_len, andps_lencomputation ahead of the upload/FFT work, or free the allocated handle on each early-return path. As per path instructions, prioritize rollback inconsistencies.🛠️ Suggested shape of the fix
+ let framed_len = request + .nfft + .checked_mul(request.frame_count) + .and_then(|len| len.checked_mul(2)) + .ok_or_else(|| anyhow!("uniform_spectral_estimate: frame too large"))?; + let rows = spectral_selected_frequency_len(request.nfft, request.range); + let range = spectral_range_shader_mode(request.range); + let selected_len = if matches!(request.range, ProviderSpectralRange::Twosided) { + None + } else { + Some( + rows.checked_mul(request.frame_count) + .and_then(|len| len.checked_mul(2)) + .ok_or_else(|| anyhow!("uniform_spectral_estimate: output too large"))?, + ) + }; + let ps_len = rows + .checked_mul(request.frame_count) + .ok_or_else(|| anyhow!("uniform_spectral_estimate: power output too large"))?; + let window_shape = [request.window.len(), 1usize]; let window = self.upload_exec(&HostTensorView { data: request.window, shape: &window_shape, })?; - - let framed_len = request - .nfft - .checked_mul(request.frame_count) - .and_then(|len| len.checked_mul(2)) - .ok_or_else(|| anyhow!("uniform_spectral_estimate: frame too large"))?;Also applies to: 79-82, 104-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/runmat-accelerate/src/backend/wgpu/provider/ops/signal.rs` around lines 27 - 37, The GPU buffers (such as window via self.upload_exec) are being allocated before size validations occur, causing potential GPU buffer leaks if the checked_mul calculations for framed_len, selected_len, or ps_len fail later. Move all the size computation and validation logic (the checked_mul calls for framed_len, selected_len, and ps_len) to the beginning of the function before any upload_exec or GPU allocation operations. This ensures all size checks fail fast before any GPU resources are allocated, preventing leaks on error paths, and applies this fix to all occurrences of this pattern in the signal operations.Source: Path instructions
| # OpenBLAS provides the BLAS and LAPACK symbols used by the Rust bindings. | ||
| & "$vcpkgRoot\vcpkg.exe" install "openblas:$triplet" | ||
| if ($triplet -eq "arm64-windows") { | ||
| & "$vcpkgRoot\vcpkg.exe" install "lapack-reference:$triplet" --allow-unsupported | ||
| } else { | ||
| & "$vcpkgRoot\vcpkg.exe" install "lapack-reference:$triplet" | ||
| } | ||
| Add-Content -Path $env:GITHUB_ENV -Value "VCPKGRS_DYNAMIC=1" | ||
| Add-Content -Path $env:GITHUB_PATH -Value "$vcpkgRoot\installed\$triplet\bin" | ||
|
|
||
| # Hint BLAS/LAPACK discovery for blas-sys/lapack-sys | ||
| Add-Content -Path $env:GITHUB_ENV -Value "BLAS_LIB_DIR=$vcpkgRoot\installed\$triplet\lib" | ||
| Add-Content -Path $env:GITHUB_ENV -Value "BLAS_LIBS=openblas" | ||
| Add-Content -Path $env:GITHUB_ENV -Value "LAPACK_LIB_DIR=$vcpkgRoot\installed\$triplet\lib" | ||
| Add-Content -Path $env:GITHUB_ENV -Value "LAPACK_LIBS=lapack;openblas" | ||
| Add-Content -Path $env:GITHUB_ENV -Value "LAPACK_LIBS=openblas" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify OpenBLAS-only workflow settings vs bootstrap provisioning packages
rg -n --no-heading 'openblas:\$triplet|LAPACK_LIBS=openblas|lapack-reference' \
.github/workflows/ci.yml \
.github/workflows/release.yml \
infra/scripts/bootstrap-windows-runner.ps1Repository: runmat-org/runmat
Length of output: 909
Align infra/scripts/bootstrap-windows-runner.ps1 to match the OpenBLAS-only contract in workflows.
Both CI and release workflows now use OpenBLAS (openblas:$triplet with LAPACK_LIBS=openblas), but the bootstrap script at line 200 still installs lapack-reference:$Triplet. This divergence creates stale-state inconsistency between self-hosted and hosted runner provisioning paths.
Update the bootstrap script to install OpenBLAS instead to maintain a consistent provisioning contract across all runner types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 310 - 319, The bootstrap script at
infra/scripts/bootstrap-windows-runner.ps1 around line 200 is installing
lapack-reference:$Triplet while the CI workflow is installing openblas:$triplet
and configuring environment variables for OpenBLAS-based BLAS/LAPACK discovery.
To align the provisioning contract across all runner types, replace the
lapack-reference installation command in the bootstrap script with an openblas
installation that matches the CI workflow configuration, ensuring consistent
environment variable setup for BLAS_LIB_DIR, BLAS_LIBS, LAPACK_LIB_DIR, and
LAPACK_LIBS pointing to OpenBLAS.
Source: Path instructions
| Write-Host "Cloning vcpkg and installing OpenBLAS" | ||
| $vcpkgRoot = Join-Path $pwd "vcpkg" | ||
| git clone https://github.com/microsoft/vcpkg.git $vcpkgRoot | ||
| & "$vcpkgRoot\bootstrap-vcpkg.bat" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify clone/install usage and check whether pinning exists
rg -n --no-heading 'git clone https://github.com/microsoft/vcpkg.git|openblas:\$triplet|git -C .*vcpkg.* checkout|builtin-baseline|x-builtin-baseline' \
.github/workflows/release.yml \
.github/workflows/ci.ymlRepository: runmat-org/runmat
Length of output: 547
Pin vcpkg to a fixed revision for reproducible Windows releases.
git clone https://github.com/microsoft/vcpkg.git without pinning makes release builds drift with upstream changes. Since this depends on OpenBLAS for LAPACK symbols, upstream port updates can break builds without any repository change.
Add a checkout after the clone:
Suggested fix
$vcpkgRoot = Join-Path $pwd "vcpkg"
git clone https://github.com/microsoft/vcpkg.git $vcpkgRoot
+ git -C $vcpkgRoot checkout <pinned-vcpkg-commit>
& "$vcpkgRoot\bootstrap-vcpkg.bat"Note: This pattern also exists in .github/workflows/ci.yml at line 297.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 272 - 275, The vcpkg repository
cloned in the git clone command is not pinned to a fixed revision, causing
Windows release builds to drift with upstream changes that can break OpenBLAS
and LAPACK dependencies. After the git clone
https://github.com/microsoft/vcpkg.git $vcpkgRoot line, add a git checkout
command with a specific commit hash to pin vcpkg to a stable, reproducible
revision. Apply the same fix to the identical pattern in
.github/workflows/ci.yml where vcpkg is cloned without pinning to ensure
consistency across both workflows.
Source: Path instructions
| let ps_frames = match provider.reshape(&estimate.ps, &[estimate.rows, segment_count, cols]) { | ||
| Ok(ps_frames) => ps_frames, | ||
| Err(err) => { | ||
| provider.free(&estimate.s).ok(); | ||
| provider.free(&estimate.ps).ok(); | ||
| return Err(pwelch_error_with_detail( | ||
| &PWELCH_ERROR_INTERNAL, | ||
| err.to_string(), | ||
| )); | ||
| } | ||
| }; | ||
| let ps_mean = match provider.reduce_mean_nd(&ps_frames, &[1]).await { | ||
| Ok(ps_mean) => ps_mean, | ||
| Err(err) => { | ||
| provider.free(&estimate.s).ok(); | ||
| provider.free(&estimate.ps).ok(); | ||
| provider.free(&ps_frames).ok(); | ||
| return Err(pwelch_error_with_detail( | ||
| &PWELCH_ERROR_INTERNAL, | ||
| err.to_string(), | ||
| )); | ||
| } | ||
| }; | ||
| let pxx = match provider.reshape(&ps_mean, &[estimate.rows, cols]) { | ||
| Ok(pxx) => pxx, | ||
| Err(err) => { | ||
| provider.free(&estimate.s).ok(); | ||
| provider.free(&estimate.ps).ok(); | ||
| provider.free(&ps_frames).ok(); | ||
| provider.free(&ps_mean).ok(); | ||
| return Err(pwelch_error_with_detail( | ||
| &PWELCH_ERROR_INTERNAL, | ||
| err.to_string(), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| provider.free(&estimate.s).ok(); | ||
| provider.free(&estimate.ps).ok(); | ||
| provider.free(&ps_frames).ok(); | ||
| if ps_mean.buffer_id != pxx.buffer_id { | ||
| provider.free(&ps_mean).ok(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
[high] Make cleanup alias-aware for reshaped handles.
ps_frames may alias estimate.ps, and pxx may alias ps_mean. The success and error paths free some of these handles independently, so metadata-only reshape providers can double-free the same buffer.
🛡️ Suggested cleanup helper
+fn same_gpu_handle(
+ a: &runmat_accelerate_api::GpuTensorHandle,
+ b: &runmat_accelerate_api::GpuTensorHandle,
+) -> bool {
+ a.device_id == b.device_id && a.buffer_id == b.buffer_id
+}
+
+fn free_if_distinct(
+ provider: &dyn runmat_accelerate_api::AccelProvider,
+ handle: &runmat_accelerate_api::GpuTensorHandle,
+ freed: &[&runmat_accelerate_api::GpuTensorHandle],
+) {
+ if !freed.iter().any(|other| same_gpu_handle(handle, other)) {
+ provider.free(handle).ok();
+ }
+}Use this for estimate.ps/ps_frames and ps_mean/pxx on both success and rollback paths.
As per path instructions, prioritize rollback inconsistencies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/runmat-runtime/src/builtins/math/signal/pwelch.rs` around lines 392 -
434, The reshape operations may create aliases where ps_frames points to the
same buffer as estimate.ps, and pxx points to the same buffer as ps_mean.
Currently, the code attempts to free both original and reshaped handles
independently in error paths and the success path, which can cause double-free
errors. Add buffer ID comparisons (similar to the existing check for ps_mean and
pxx) before freeing estimate.ps in both error paths and before freeing ps_mean
in all paths. Only free the original handle if its buffer_id differs from the
reshaped handle's buffer_id to prevent double-freeing the same underlying
buffer, and ensure this alias-aware cleanup is applied consistently across all
error and success paths.
Source: Path instructions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 13193fd. Configure here.
| }} | ||
| let element = idx / 2u; | ||
| let freq = (element / {inner_stride}u) % {transform_len}u; | ||
| Out.data[idx] = Input.data[idx] * analytic_multiplier(freq); |
There was a problem hiding this comment.
Hilbert mask frequency indexing
High Severity
The Hilbert analytic mask derives FFT bin index from flat interleaved element index divided by inner_stride. That matches a vector layout but not row-major FFT output along a matrix dimension, so bin multipliers apply to the wrong frequencies when hilbert runs down columns or rows of a 2-D tensor.
Reviewed by Cursor Bugbot for commit 13193fd. Configure here.
a723b5f
into
codex/eval-loop-round-2-batch-2
2 participants
Summary
eval-loop-round-2, stacked on batch 2.39fbb011b^..a5b81f3c3.vpa/digits/int, controlzero, plotting/audio followups, and array constructor additions.Stack
codex/eval-loop-round-2-batch-2Verification
git diff --name-only codex/eval-loop-round-2-batch-2..codex/eval-loop-round-2-batch-3 | wc -l=>134.git diff --quiet eval-loop-round-2 codex/eval-loop-round-2-batch-3passed.cargo testwas run and failed inrunmat-accelerateWGPU tests:transpose_complex_interleaved_feeds_downstream_abs_in_transposed_orderwgpu_complex_unary_trig_large_imag_edges_are_not_nansignal_hilbert_provider_matches_row_cosinesignal_hilbert_provider_operates_down_columnsSummary by CodeRabbit
Release Notes
New Features
fminuncusing quasi-Newton BFGS.int), variable-precision arithmetic (vpa), and digit control (digits).audioread) with format support.Enhancements
infandnanarray constructors with GPU fallback.Note
High Risk
Large WGPU behavioral surface (complex layout, Hilbert/envelope numerics) plus CI dependency changes; known test failures on accelerate WGPU paths increase regression risk before merge.
Overview
This batch extends the GPU accelerate provider with resident complex-interleaved storage end-to-end: construction (
complex_from_real/real+imag), unary/binary/broadcast math, scalar ops, repmat/permute/transpose/reshape/download/gradient/polyint, and dedicated comms shaders for symbol and bit-stream constellation modulation (CPU fast path + GPU validation).Signal processing gains provider APIs and WGPU implementations for
signal_envelope(analytic, analytic FIR, RMS) andsignal_hilbert, plus refactored spectral framing (ColumnSliding) and shared signal shaders. The publicrunmat-accelerate-apitrait adds matching hooks with request validation (including envelope shape guards).Windows build/CI stops using
lapack-reference(and arm64--allow-unsupported) in favor of OpenBLAS + CLAPACK, pins a vcpkg revision, fails fast on bootstrap/install errors, and setsLAPACK_LIBS=lapack;libf2c;openblas(release provisioning optionally omitslibf2cwhen absent).Minor:
BuiltinDocdropsreferencesfrom TS bindings;Cargo.lockpicks upnum-bigintdeps on builtins/runtime crates.Note: PR verification reported failing WGPU tests (complex transpose/abs, complex trig edges, Hilbert column/row cases) at the time of the stack description.
Reviewed by Cursor Bugbot for commit ce3807f. Bugbot is set up for automated code reviews on this repo. Configure here.