fix(rb-09): decoder propagates read and decode errors
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) <noreply@anthropic.com>
This commit is contained in:
@@ -3,6 +3,7 @@ use std::path::Path;
|
|||||||
|
|
||||||
use symphonia::core::audio::SampleBuffer;
|
use symphonia::core::audio::SampleBuffer;
|
||||||
use symphonia::core::codecs::DecoderOptions;
|
use symphonia::core::codecs::DecoderOptions;
|
||||||
|
use symphonia::core::errors::Error as SymphoniaError;
|
||||||
use symphonia::core::formats::FormatOptions;
|
use symphonia::core::formats::FormatOptions;
|
||||||
use symphonia::core::io::MediaSourceStream;
|
use symphonia::core::io::MediaSourceStream;
|
||||||
use symphonia::core::meta::MetadataOptions;
|
use symphonia::core::meta::MetadataOptions;
|
||||||
@@ -13,6 +14,12 @@ use kon_core::types::AudioSamples;
|
|||||||
|
|
||||||
/// Decode an audio file to mono f32 PCM samples.
|
/// Decode an audio file to mono f32 PCM samples.
|
||||||
/// Supports all formats symphonia handles: mp3, aac, flac, wav, ogg, etc.
|
/// 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<AudioSamples> {
|
pub fn decode_audio_file(path: &Path) -> Result<AudioSamples> {
|
||||||
let file = File::open(path)
|
let file = File::open(path)
|
||||||
.map_err(|e| KonError::AudioDecodeFailed(format!("Cannot open file: {e}")))?;
|
.map_err(|e| KonError::AudioDecodeFailed(format!("Cannot open file: {e}")))?;
|
||||||
@@ -23,9 +30,16 @@ pub fn decode_audio_file(path: &Path) -> Result<AudioSamples> {
|
|||||||
hint.with_extension(ext);
|
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<AudioSamples> {
|
||||||
let probed = symphonia::default::get_probe()
|
let probed = symphonia::default::get_probe()
|
||||||
.format(
|
.format(
|
||||||
&hint,
|
hint,
|
||||||
mss,
|
mss,
|
||||||
&FormatOptions::default(),
|
&FormatOptions::default(),
|
||||||
&MetadataOptions::default(),
|
&MetadataOptions::default(),
|
||||||
@@ -53,31 +67,35 @@ pub fn decode_audio_file(path: &Path) -> Result<AudioSamples> {
|
|||||||
.map_err(|e| KonError::AudioDecodeFailed(format!("Codec error: {e}")))?;
|
.map_err(|e| KonError::AudioDecodeFailed(format!("Codec error: {e}")))?;
|
||||||
|
|
||||||
let mut samples: Vec<f32> = Vec::new();
|
let mut samples: Vec<f32> = Vec::new();
|
||||||
let mut decode_errors = 0u32;
|
|
||||||
|
|
||||||
loop {
|
loop {
|
||||||
let packet = match format.next_packet() {
|
let packet = match format.next_packet() {
|
||||||
Ok(p) => p,
|
Ok(p) => p,
|
||||||
Err(symphonia::core::errors::Error::IoError(ref e))
|
Err(SymphoniaError::IoError(ref e))
|
||||||
if e.kind() == std::io::ErrorKind::UnexpectedEof =>
|
if e.kind() == std::io::ErrorKind::UnexpectedEof =>
|
||||||
{
|
{
|
||||||
|
// Normal end of stream — symphonia signals EOF via UnexpectedEof.
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
Err(symphonia::core::errors::Error::ResetRequired) => break,
|
Err(SymphoniaError::ResetRequired) => {
|
||||||
Err(_) => break,
|
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 {
|
if packet.track_id() != track_id {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
let decoded = match decoder.decode(&packet) {
|
let decoded = decoder
|
||||||
Ok(d) => d,
|
.decode(&packet)
|
||||||
Err(_) => {
|
.map_err(|e| KonError::AudioDecodeFailed(format!("packet decode failed: {e}")))?;
|
||||||
decode_errors += 1;
|
|
||||||
continue;
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
let spec = *decoded.spec();
|
let spec = *decoded.spec();
|
||||||
let channels = spec.channels.count();
|
let channels = spec.channels.count();
|
||||||
@@ -96,13 +114,118 @@ pub fn decode_audio_file(path: &Path) -> Result<AudioSamples> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if samples.is_empty() {
|
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()));
|
return Err(KonError::AudioDecodeFailed("No audio data decoded".into()));
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(AudioSamples::new(samples, sample_rate, 1))
|
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<u8> {
|
||||||
|
let path = temp_path("kon_decode_tmp_for_bytes.wav");
|
||||||
|
let samples: Vec<f32> = (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<Vec<u8>>,
|
||||||
|
fail_after_bytes: u64,
|
||||||
|
bytes_read: u64,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl Read for FlakyCursor {
|
||||||
|
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
|
||||||
|
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<u64> {
|
||||||
|
self.inner.seek(pos)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
impl MediaSource for FlakyCursor {
|
||||||
|
fn is_seekable(&self) -> bool {
|
||||||
|
true
|
||||||
|
}
|
||||||
|
fn byte_len(&self) -> Option<u64> {
|
||||||
|
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<f32> = (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:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -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-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 |
|
| 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 |
|
| # | 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-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-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-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-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 |
|
| 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 |
|
| # | 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`. |
|
| 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
|
## Dependencies
|
||||||
|
|||||||
@@ -4,6 +4,29 @@
|
|||||||
**Path:** `crates/audio/src/decode.rs:58-79`
|
**Path:** `crates/audio/src/decode.rs:58-79`
|
||||||
**Source:** [2026-04-22 code review](../code-review-2026-04-22.md)
|
**Source:** [2026-04-22 code review](../code-review-2026-04-22.md)
|
||||||
**Labels:** release-blocker, major, audio, data-integrity
|
**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
|
## Problem
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user