From b48d39bfb1dd17ebece34ae7891768292650bd59 Mon Sep 17 00:00:00 2001 From: Jake Date: Wed, 22 Apr 2026 10:24:18 +0100 Subject: [PATCH] fix(rb-09): decoder propagates read and decode errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit decode_audio_file's packet loop was `Err(_) => break`, so any non-EOF read error during playback dropped out silently with whatever samples had accumulated. Per-packet decode errors were tallied and skipped, contributing to the same outcome. A corrupt or truncated input therefore came back as `Ok(partial_samples)` — no way for callers to distinguish a clean decode from a compromised one. Every SymphoniaError other than the explicit EOF (`IoError(UnexpectedEof)`) now maps to `AudioDecodeFailed`. Decoder errors bubble via `?` rather than being counted. `ResetRequired` promotes to an error rather than a silent break. Extracted an internal `decode_media_stream(mss, hint)` so tests can inject a custom `MediaSource`. Added `FlakyCursor` — a seekable cursor that returns a synthetic I/O error after N bytes — and a regression test that confirms mid-stream read failure surfaces as `Err` instead of returning partial audio. Happy-path and missing-file tests added for coverage. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/audio/src/decode.rs | 157 ++++++++++++++++-- docs/issues/README.md | 4 +- docs/issues/decoder-partial-audio-on-error.md | 23 +++ 3 files changed, 165 insertions(+), 19 deletions(-) diff --git a/crates/audio/src/decode.rs b/crates/audio/src/decode.rs index e054bdb..e2a4936 100644 --- a/crates/audio/src/decode.rs +++ b/crates/audio/src/decode.rs @@ -3,6 +3,7 @@ use std::path::Path; use symphonia::core::audio::SampleBuffer; use symphonia::core::codecs::DecoderOptions; +use symphonia::core::errors::Error as SymphoniaError; use symphonia::core::formats::FormatOptions; use symphonia::core::io::MediaSourceStream; use symphonia::core::meta::MetadataOptions; @@ -13,6 +14,12 @@ use kon_core::types::AudioSamples; /// Decode an audio file to mono f32 PCM samples. /// Supports all formats symphonia handles: mp3, aac, flac, wav, ogg, etc. +/// +/// Any read- or decode-side error is propagated as `KonError::AudioDecodeFailed`. +/// A previous implementation `break`ed out of the packet loop on any read +/// error and skipped per-packet decode errors, so a truncated or corrupt +/// input silently returned `Ok` with whatever had decoded before the +/// failure — flagged by the 2026-04-22 review (RB-09). pub fn decode_audio_file(path: &Path) -> Result { let file = File::open(path) .map_err(|e| KonError::AudioDecodeFailed(format!("Cannot open file: {e}")))?; @@ -23,9 +30,16 @@ pub fn decode_audio_file(path: &Path) -> Result { hint.with_extension(ext); } + decode_media_stream(mss, &hint) +} + +/// Decode from an already-constructed `MediaSourceStream`. Split out so +/// tests can inject a custom `MediaSource` (for example, one that +/// returns a mid-stream I/O error) to verify error propagation. +fn decode_media_stream(mss: MediaSourceStream, hint: &Hint) -> Result { let probed = symphonia::default::get_probe() .format( - &hint, + hint, mss, &FormatOptions::default(), &MetadataOptions::default(), @@ -53,31 +67,35 @@ pub fn decode_audio_file(path: &Path) -> Result { .map_err(|e| KonError::AudioDecodeFailed(format!("Codec error: {e}")))?; let mut samples: Vec = Vec::new(); - let mut decode_errors = 0u32; loop { let packet = match format.next_packet() { Ok(p) => p, - Err(symphonia::core::errors::Error::IoError(ref e)) + Err(SymphoniaError::IoError(ref e)) if e.kind() == std::io::ErrorKind::UnexpectedEof => { + // Normal end of stream — symphonia signals EOF via UnexpectedEof. break; } - Err(symphonia::core::errors::Error::ResetRequired) => break, - Err(_) => break, + Err(SymphoniaError::ResetRequired) => { + return Err(KonError::AudioDecodeFailed( + "decoder reset required mid-stream — input contains a discontinuity".into(), + )); + } + Err(e) => { + return Err(KonError::AudioDecodeFailed(format!( + "packet read failed: {e}" + ))); + } }; if packet.track_id() != track_id { continue; } - let decoded = match decoder.decode(&packet) { - Ok(d) => d, - Err(_) => { - decode_errors += 1; - continue; - } - }; + let decoded = decoder + .decode(&packet) + .map_err(|e| KonError::AudioDecodeFailed(format!("packet decode failed: {e}")))?; let spec = *decoded.spec(); let channels = spec.channels.count(); @@ -96,13 +114,118 @@ pub fn decode_audio_file(path: &Path) -> Result { } if samples.is_empty() { - if decode_errors > 0 { - return Err(KonError::AudioDecodeFailed(format!( - "No audio decoded ({decode_errors} packets failed — file may be corrupt)" - ))); - } return Err(KonError::AudioDecodeFailed("No audio data decoded".into())); } Ok(AudioSamples::new(samples, sample_rate, 1)) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::wav::write_wav; + use std::io::{Cursor, Read, Seek, SeekFrom}; + use symphonia::core::io::MediaSource; + + fn temp_path(name: &str) -> std::path::PathBuf { + let mut p = std::env::temp_dir(); + p.push(name); + let _ = std::fs::remove_file(&p); + p + } + + fn valid_wav_bytes(sample_count: usize) -> Vec { + let path = temp_path("kon_decode_tmp_for_bytes.wav"); + let samples: Vec = (0..sample_count).map(|i| (i as f32) / 1000.0).collect(); + let audio = AudioSamples::mono_16khz(samples); + write_wav(&path, &audio).unwrap(); + let bytes = std::fs::read(&path).unwrap(); + std::fs::remove_file(&path).ok(); + bytes + } + + /// A `MediaSource` that wraps a byte buffer and returns an injected + /// I/O error once more than `fail_after_bytes` total bytes have been + /// returned successfully. Simulates real-world disk or network read + /// failure mid-stream. + struct FlakyCursor { + inner: Cursor>, + fail_after_bytes: u64, + bytes_read: u64, + } + + impl Read for FlakyCursor { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + if self.bytes_read >= self.fail_after_bytes { + return Err(std::io::Error::other("injected mid-stream read error")); + } + let n = self.inner.read(buf)?; + self.bytes_read = self.bytes_read.saturating_add(n as u64); + Ok(n) + } + } + + impl Seek for FlakyCursor { + fn seek(&mut self, pos: SeekFrom) -> std::io::Result { + self.inner.seek(pos) + } + } + + impl MediaSource for FlakyCursor { + fn is_seekable(&self) -> bool { + true + } + fn byte_len(&self) -> Option { + Some(self.inner.get_ref().len() as u64) + } + } + + #[test] + fn decodes_valid_wav_successfully() { + let path = temp_path("kon_decode_valid.wav"); + let samples: Vec = (0..4_000).map(|i| (i as f32) / 1000.0).collect(); + write_wav(&path, &AudioSamples::mono_16khz(samples)).unwrap(); + + let loaded = decode_audio_file(&path).expect("valid WAV must decode"); + assert_eq!(loaded.sample_rate(), 16_000); + assert!(!loaded.samples().is_empty()); + + std::fs::remove_file(&path).ok(); + } + + #[test] + fn missing_file_surfaces_error() { + let path = temp_path("kon_decode_missing.wav"); + let result = decode_audio_file(&path); + assert!(result.is_err(), "missing file must error, got: {result:?}"); + } + + // RB-09 regression: once probe has succeeded, any mid-stream I/O + // error must surface as `Err(AudioDecodeFailed)` rather than being + // silently swallowed and returning whatever was decoded so far. + // + // Pre-fix behaviour: the packet loop had `Err(_) => break`, so an + // I/O error during `format.next_packet()` dropped out of the loop + // and the function returned `Ok` with partial samples. + #[test] + fn mid_stream_io_error_propagates_instead_of_returning_partial_audio() { + let bytes = valid_wav_bytes(16_000); + // Fail after ~1 KiB — probe has seen the RIFF/WAVE header by then, + // so probing succeeds. The packet loop hits our injected error + // before the stream reaches its natural EOF. + let flaky = FlakyCursor { + inner: Cursor::new(bytes), + fail_after_bytes: 1024, + bytes_read: 0, + }; + let mss = MediaSourceStream::new(Box::new(flaky), Default::default()); + let mut hint = Hint::new(); + hint.with_extension("wav"); + + let result = decode_media_stream(mss, &hint); + assert!( + result.is_err(), + "mid-stream I/O error must surface, got: {result:?}" + ); + } +} diff --git a/docs/issues/README.md b/docs/issues/README.md index a54a3b4..3abdef4 100644 --- a/docs/issues/README.md +++ b/docs/issues/README.md @@ -19,7 +19,7 @@ should be mirrored as real GitHub issues on `jakejars/kon`. | RB-02 | [c3-migrations-atomicity.md](c3-migrations-atomicity.md) | `crates/storage/migrations.rs` | medium | | RB-03 | [c4-transcript-profile-fk.md](c4-transcript-profile-fk.md) | `crates/storage/migrations.rs` + `database.rs` | large | -## MAJOR (8 open, 1 resolved) +## MAJOR (7 open, 2 resolved) | # | File | Area | Fix scope | |---|---|---|---| @@ -28,7 +28,6 @@ should be mirrored as real GitHub issues on `jakejars/kon`. | RB-06 | [native-capture-worker-join.md](native-capture-worker-join.md) | `src-tauri/commands/audio.rs` | medium | | RB-07 | [runtime-capabilities-accelerators.md](runtime-capabilities-accelerators.md) | `src-tauri/commands/models.rs` | medium | | RB-08 | [power-assertion-macos-objc2.md](power-assertion-macos-objc2.md) | `src-tauri/commands/power.rs` | medium | -| RB-09 | [decoder-partial-audio-on-error.md](decoder-partial-audio-on-error.md) | `crates/audio/decode.rs` | medium | | RB-10 | [llm-prompt-preflight.md](llm-prompt-preflight.md) | `crates/llm/lib.rs` | medium | | RB-11 | [keystore-thread-safety.md](keystore-thread-safety.md) | `crates/cloud-providers/keystore.rs` | medium | @@ -36,6 +35,7 @@ should be mirrored as real GitHub issues on `jakejars/kon`. | # | File | Area | Resolution | |---|---|---|---| +| RB-09 | [decoder-partial-audio-on-error.md](decoder-partial-audio-on-error.md) | `crates/audio/decode.rs` | Packet-loop now propagates all non-EOF `SymphoniaError`s as `AudioDecodeFailed`; per-packet decode errors bubble via `?`. Mock-`MediaSource` regression test confirms mid-stream I/O errors surface instead of returning partial audio. | | RB-12 | [hotkey-linux-device-filter.md](hotkey-linux-device-filter.md) | `crates/hotkey/linux.rs` | Extracted `device_supports_combo` helper; `try_attach_device` now reads the configured `HotkeyCombo` from the watch channel and checks support for that trigger key. Four regression tests land in `linux::tests`. | ## Dependencies diff --git a/docs/issues/decoder-partial-audio-on-error.md b/docs/issues/decoder-partial-audio-on-error.md index c930b25..cb3c99e 100644 --- a/docs/issues/decoder-partial-audio-on-error.md +++ b/docs/issues/decoder-partial-audio-on-error.md @@ -4,6 +4,29 @@ **Path:** `crates/audio/src/decode.rs:58-79` **Source:** [2026-04-22 code review](../code-review-2026-04-22.md) **Labels:** release-blocker, major, audio, data-integrity +**Status:** RESOLVED (2026-04-22) + +## Resolution + +`decode_audio_file` now propagates every `SymphoniaError` other than the +explicit end-of-stream `UnexpectedEof`: + +- `SymphoniaError::ResetRequired` → error (mid-stream discontinuity). +- Any other packet-read error → `KonError::AudioDecodeFailed`. +- `decoder.decode(&packet)` errors → bubble via `?` instead of + counter-then-skip. + +The decode logic was refactored into an internal +`decode_media_stream(mss, hint)` so tests can inject a custom +`MediaSource`. The regression test `FlakyCursor` returns a valid WAV +header followed by an injected `io::Error` after 1024 bytes; the +`mid_stream_io_error_propagates_instead_of_returning_partial_audio` test +asserts the caller receives `Err`, not an `Ok` with a truncated samples +vector. Companion tests cover the happy path and the +file-does-not-exist path. + +The optional `decode_audio_file_best_effort` variant suggested in the +original issue was not added — no caller needs it today. ## Problem