Skip to content

fix: preserve Rust Whisper audio until flush succeeds - #13075

Merged
kodjima33 merged 3 commits into
BasedHardware:mainfrom
beyondlong:bounty/rust-whisper-flush
Sep 8, 2026
Merged

kodjima33 merged 3 commits into
BasedHardware:mainfrom
beyondlong:bounty/rust-whisper-flush

Conversation

@beyondlong

@beyondlong beyondlong commented Sep 8, 2026

Copy link
Copy Markdown

What changed and why

Rust Whisper callers cannot transcribe a final audio buffer shorter than five seconds, and a runner error currently discards a full buffer before the caller can retry. Add a public flush() that clears audio only after success and share that path with append_pcm. Related to #13011; this PR covers only the Rust Whisper buffer, not the full cross-language issue.

Product invariants affected

none

How it was verified

From sdks/device/rust, cargo test --features stt-whisper passed all 10 tests, cargo fmt --check passed, and cargo clippy --features stt-whisper -- -D warnings passed. The README example was compiled and executed against the production library; its injected runner received the full 160,000-byte batch and then the 640-byte final tail.

Default-feature Cargo tests also passed (3 tests). The original API regression was run against the original production implementation before changing it: the retry returned Ok(None) instead of the retained batch's transcript. A separate public API executable reproduces that failure against the baseline and succeeds against the patch. No Bluetooth hardware, real Whisper model, or live provider was exercised.

scripts/pr-preflight --lane local --metadata-only --pr-body-file <draft> exited 0; its 90-day failure-class guard skipped because the checkout is shallow. Full make preflight did not pass: the workflow apt-bounds wrapper tried to provision the canonical backend Python environment and stopped at a missing backend/.python-version in this sparse checkout. Running the underlying apt-bounds tests directly with the available Python/PyYAML passed all 18 tests. This is not a claim that the full repository gate passed; rerun it in a provisioned checkout before submission.

Tests

Seven feature-gated tests cover failed-batch retention, empty flush, final-tail delivery and subsequent appends, empty-transcript clearing, full batch plus tail, explicit retry for partial/full buffers, and oversized-append compatibility. Exact PCM bytes and runner call counts are asserted on the error/retry paths.

Failure class (fixes)

Failure-Class: none

This is a localized in-memory SDK buffer lifetime fix. The existing durable-store replacement and teardown-finalizer classes have different owners and prevention primitives; this change does not alter their registries or claim to close the cross-language lifecycle work.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread sdks/device/rust/src/lib.rs Outdated
@beyondlong

Copy link
Copy Markdown
Author

Addressed in 9a1723c.

The review finding was valid: Vec::clear() reset the length but retained the peak allocation. flush() now takes ownership of the buffer before invoking the runner, restores the exact buffer on error, and lets the allocation drop after success. I added a regression that processes a 1,600,000-byte append and verifies the successful path releases the buffer capacity.

Verified in sdks/device/rust:

  • cargo test --features stt-whisper — 11 passed
  • cargo fmt --all -- --check
  • cargo clippy --all-targets --features stt-whisper -- -D warnings

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed the Rust Whisper buffering change in sdks/device/rust — a well-scoped fix for the audio-loss pattern from #13011: a runner error used to discard a full 5-second batch via ? with no retry path, and a shorter final tail could never be transcribed at all.

What I verified on 9a1723c:

  • src/lib.rs — flush() takes the buffer before invoking the runner (std::mem::take), restores it verbatim on error, and lets the allocation drop on success. append_pcm now shares that path, so a failed full batch stays buffered and a retry (flush(), or an append that re-crosses the threshold) resubmits exactly the retained audio. The empty-buffer short-circuit prevents double submission on repeated flushes, and the transcriber stays usable after a successful flush. The earlier review finding about Vec::clear() retaining the peak allocation is resolved by this take-and-restore shape, with successful_oversized_append_releases_peak_allocation covering it.
  • The eight new whisper_tests are hermetic (injected runners, no model/hardware) and cover the failure/retention matrix well: failed batch retention, empty flush not reaching the runner, tail-flush-once plus append-after-flush, empty-transcript clearing, full-batch + tail separation, and oversized single-call behavior.
  • README.md is accurate against the code and the shared STT contract: the 160,000-byte (5 s PCM16 LE mono) threshold, oversized appends not being split, error-retention semantics, and the distinction from sdks/rust/omi-device all check out. The example compiles and runs against the crate and prints the documented output.
  • Ran in an isolated container: cargo test --features stt-whisper (11 passed), default-feature cargo test --lib (3 passed), cargo fmt --all -- --check, and cargo clippy --lib --features stt-whisper -- -D warnings — all clean.

One non-blocking note for a maintainer: flush() is caller-driven, so when the Rust SDK later gains the stop/close lifecycle being discussed in #13011, wiring flush() into it would avoid losing final words; keeping that cross-language contract out of this PR is the right scope call.

Thanks @beyondlong — clear description, honest verification notes (including what was NOT run), and good test discipline. No security or product-fit concerns; leaving formal merge sign-off to a human maintainer.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added the positive-signal Automation verified a genuine fix/quality contribution label Sep 8, 2026

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust SDK bug fix, confidence 5/5: adds flush() so failed/short final buffers aren't discarded, scoped diff, 10 new tests covering retry/empty/tail cases, confirmed not already on main.

@kodjima33
kodjima33 merged commit b25b313 into BasedHardware:main Sep 8, 2026
@beyondlong

Copy link
Copy Markdown
Author

Thank you for reviewing and merging #13075, @kodjima33.

Could you confirm whether the US$25 bounty proposed in #13011 (comment) is approved for this merged Rust Whisper fix? The contribution guide directs payment claims to team@basedhardware.com with the bounty link and PayPal account. I will keep payout details private and follow that process if eligibility is confirmed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

positive-signal Automation verified a genuine fix/quality contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants