feat(recorder): the WASAPI capture sidecar - #9
Conversation
Closes plan tasks 4.1-4.5. 64 Rust tests; the three TDD tasks were built RED first. The device sits behind a `CaptureSource` trait, so everything that can be wrong without a symptom — track alignment, the silence that stands in for frames the API never sends, the pause arithmetic, the time map — is tested on every platform, and WASAPI is a thin edge. Two bugs the TDD caught before they shipped: - A recorded instant landing exactly on a pause belongs to the segment that *resumed*, not the one that ended. Written the other way, with four ten-second pauses, a citation landed ten seconds off — in the component the plan calls "lies with confidence". - The first session pump padded the track to "now" *and* appended the second of audio covering that same second, producing a track twice the length of the meeting. From the two reviews on this branch, which between them found nineteen issues. The three that meant the binary could not record on Windows: - The loopback stream was opened as (Render, Render). The crate derives AUDCLNT_STREAMFLAGS_LOOPBACK from the *pair* — device Render, stream Capture — so the flag was never set, the client initialised as a plain playback stream, and asking it for an IAudioCaptureClient failed, taking the sidecar down at launch. The device direction and the stream direction are two decisions and were one function. - `open_stream` started the stream, so the first `start` request hit AUDCLNT_E_NOT_STOPPED and dropped the session that had just been built. - Capture was drained only when an RPC line arrived, one packet at a time, from a buffer of `min_period`. Continuous capture would have needed ~300 requests/second and could not exceed ~50. Everything missed was backfilled with manufactured silence, so an hour of nothing reported the right length, the right frame count and a healthy time map. Each device is now drained on a thread of its own — opened *on* that thread, because a WASAPI client is COM and not `Send`. And the rest: a failed reopen no longer leaves a track dead forever; a read failure is treated as a reason to reopen, which is how a device change usually announces itself; pause resets the stream so pre-pause audio does not land after the resume; the time map is never extended past what the tracks hold; `first_frames` counts real frames rather than manufactured silence, which had it null in every realistic recording; one failing device no longer freezes the other track; WAV write failures are counted rather than discarded; a failing header no longer costs the whole manifest; the 4 GiB a WAV header can describe is guarded; and the manifest is on one time base instead of two indistinguishable ones. The workspace's `unsafe_code = "deny"` did not need lifting: the wasapi crate holds the unsafe, so this crate has none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
📝 WalkthroughWalkthroughThe PR replaces the recorder stub with a JSON-lines recording sidecar. It adds synchronized microphone and system capture, pause and resume handling, time mapping, WAV output, manifests, RPC handling, threaded capture, and Windows WASAPI integration. ChangesRecorder sidecar
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Stdin
participant Main
participant Service
participant Session
participant CaptureSource
Stdin->>Main: Send JSON-lines request
Main->>Service: Parse and handle request
Service->>Session: Start, pause, resume, stop, or report status
Service->>CaptureSource: Poll microphone and system capture
Session->>Session: Update tracks and time map
Service-->>Main: Return JSON response
Main-->>Stdin: Flush response line
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (10)
crates/recorder/src/rpc.rs (1)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape the error text in the
renderfallback.Line 80 interpolates the
serde_json::Errordisplay text directly into a JSON string literal. If that text ever contains"or\, the fallback emits invalid JSON — which is the one thing this fallback exists to prevent.The current
PayloadandStatusPayloadtypes cannot realistically fail to serialize, so this is not reachable today. Building the fallback throughserde_jsonkeeps the guarantee true if the payload types grow.♻️ Proposed fix
pub fn render(response: &Response) -> String { serde_json::to_string(response).unwrap_or_else(|e| { - format!("{{\"ok\":\"false\",\"error\":\"could not serialise the response: {e}\"}}") + // Build the fallback through serde so the error text is escaped. + serde_json::to_string(&Response::Err { + error: format!("could not serialise the response: {e}"), + }) + .unwrap_or_else(|_| { + r#"{"ok":"false","error":"could not serialise the response"}"#.to_string() + }) }) }🤖 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/recorder/src/rpc.rs` around lines 78 - 82, Update the render fallback to construct the error response through serde_json serialization rather than interpolating serde_json::Error directly into a JSON string; ensure any quotes, backslashes, or other special characters in the error text are escaped while preserving the existing fallback message and return type.crates/recorder/tests/rpc.rs (1)
89-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a render test for
Payload::Done.
Payloadhas three variants. These tests coverStatusandDevices, but notDone.Doneis the responseservice.rsreturns forstop, which is the one response that reports whether the recording was written.
Payloadis#[serde(untagged)]nested inside an internally taggedResponse, so the flattened key layout is worth pinning for every variant.💚 Proposed test
#[test] fn a_finished_recording_renders_as_done() { let parsed: serde_json::Value = serde_json::from_str(&render(&Response::Ok(Payload::Done { done: true }))) .expect("valid json"); assert_eq!(parsed["ok"], "true"); assert_eq!(parsed["done"], true); }🤖 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/recorder/tests/rpc.rs` around lines 89 - 95, Add a render test alongside the existing `Status` and `Devices` tests that constructs `Response::Ok(Payload::Done { done: true })`, parses the rendered JSON, and asserts the flattened `ok` and boolean `done` fields. Keep the test focused on pinning the serialized layout for the `Done` variant.crates/recorder/src/pump.rs (1)
113-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport repeated poll errors instead of retrying silently forever.
On a poll error the thread sleeps 5 ms and retries, with no counter and no upper bound. If the device fails persistently, this loop runs about 200 times per second for the whole recording and nothing reports it. The session sees only
Poll::Idleand pads the track.The comment is right that the thread should not give up. Count the consecutive errors and publish the count or the last error text, so
statuscan show that capture is failing. Increasing the backoff also avoids a busy loop on a permanently broken device.🤖 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/recorder/src/pump.rs` around lines 113 - 117, Update the poll-error handling loop in the recorder pump to track consecutive errors without terminating the thread, publish the count or latest error text through the existing status mechanism, and increase the retry backoff to avoid a tight loop during persistent device failure. Reset the consecutive-error state after a successful poll and preserve normal recovery behavior.crates/recorder/src/wasapi_source.rs (1)
382-389: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
draininstead of a per-bytepop_frontloop, and derive the sample width fromBITS.Lines 383-385 move
wholebytes one at a time.self.pendingholds at leastwholeelements, sounwrap_or(0)never fires and hides nothing.VecDeque::drainexpresses the same operation and copies in bulk. For 500 ms of 48 kHz stereof32this loop handles about 768,000 bytes per drain.Line 387 hard-codes the sample width as
4. That matchesBITS = 32at line 29 andSampleType::Floatat line 247, but the two are coupled implicitly.♻️ Proposed refactor
+/// Bytes per sample, from the format requested at line 244. +const SAMPLE_BYTES: usize = BITS / 8;- let mut bytes = Vec::with_capacity(whole); - for _ in 0..whole { - bytes.push(self.pending.pop_front().unwrap_or(0)); - } - let samples = bytes - .chunks_exact(4) - .map(|b| f32::from_le_bytes([b[0], b[1], b[2], b[3]])) - .collect(); + let bytes: Vec<u8> = self.pending.drain(..whole).collect(); + let samples = bytes + .chunks_exact(SAMPLE_BYTES) + .map(|b| f32::from_le_bytes([b[0], b[1], b[2], b[3]])) + .collect();🤖 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/recorder/src/wasapi_source.rs` around lines 382 - 389, Update the byte extraction in the recorder method around self.pending and samples to drain whole bytes from the VecDeque in bulk instead of repeatedly calling pop_front, preserving the existing ordering and length. Replace the hard-coded 4-byte chunk width with a value derived from the existing BITS constant so sample decoding remains coupled to the configured sample size.crates/recorder/src/capture.rs (1)
13-14: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign
wall_nswith its actual use.Session::applyignores the device timestamp and uses the session clock, while real producers emit0. Update the doc comment or remove the unused field and its callers.🤖 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/recorder/src/capture.rs` around lines 13 - 14, Align the Frames variant’s wall_ns contract with Session::apply: either update its documentation to state that the value is ignored and the session clock is used, or remove wall_ns and update all constructors and callers, including real producers that currently pass 0.crates/recorder/tests/track.rs (1)
165-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the new WAV paths.
This file covers frames and padding well. It does not cover
attach_wav,finalize,failed_samples, orat_size_limit, which this PR adds.at_size_limitis pure arithmetic and cheap to test at the boundary.attach_wavfollowed byfinalizein a temp directory would pin the "buffered samples move into the file" behaviour that lines 58-61 oftrack.rsdescribe. I can draft these tests if you want.🤖 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/recorder/tests/track.rs` around lines 165 - 173, Add focused tests in the track test module for the new WAV paths: verify at_size_limit at its boundary, cover failed_samples behavior, and use a temporary directory to test attach_wav followed by finalize, confirming buffered samples are moved into the output file as described by TrackWriter.crates/recorder/src/service.rs (1)
168-192: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRecord the failed-sample count in the manifest.
framescomes fromframes_written, which counts samples the WAV refused.finalize_filesreports those in thestopresponse, but the response is not persisted. Stage 4.7 and 4.11 readmanifest.jsonalone, so they cannot tell that the file is shorter than the declared frame count. Add the failed count per track toTrackInfo, or subtract it fromframes.🤖 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/recorder/src/service.rs` around lines 168 - 192, Update manifest_of so each TrackInfo records the number of samples actually accepted by the WAV, rather than relying on frames_written alone. Use the existing failed-sample count from the session/finalization state for both mic and system tracks, either by adding that field to TrackInfo or subtracting it from frames, while preserving the manifest’s per-track data.crates/recorder/tests/session.rs (1)
307-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the failing-source coverage to
resume.
BrokenSourceonly fails onpoll. A source that fails onstartis the case that exposes the asymmetricresumepath flagged incrates/recorder/src/session.rslines 303-329. Add a variant whosestartreturnsErr, then assert that a failedresumeleaves both devices stopped and the state unchanged.🤖 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/recorder/tests/session.rs` around lines 307 - 320, Extend the test fixtures around BrokenSource with a source variant whose start method returns Err, then add resume coverage using it. Assert that when resume fails, both devices remain stopped and the session state is unchanged, while preserving the existing poll-failure tests.crates/recorder/src/session.rs (1)
142-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse one arithmetic form for recorded nanoseconds.
recorded_nsat lines 136-139 promotes tou128before multiplying.recorded_now_nsusessaturating_mul(1_000_000_000)on au64, which saturates instead of producing a correct value, and it derives the position fromexpected_frames_atrather thanframes_written. The two therefore report different offsets for the same instant. ADeviceChange.recorded_nsstamped from the second form does not line up with therecorded_msthatstatusreports. Compute both through the same helper.🤖 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/recorder/src/session.rs` around lines 142 - 149, Update recorded_now_ns to use the same arithmetic and frame-position source as recorded_ns: derive the offset from frames_written, promote to u128 before multiplying by 1_000_000_000, then divide by the sample rate and convert safely to u64. Reuse the existing recorded_ns helper or centralize the calculation so DeviceChange timestamps and status recorded_ms remain aligned.crates/recorder/src/track.rs (1)
151-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the anchor field to match the clock the callers pass.
Session::start,Session::resume, andSession::pad_toall pass monotonic readings intobegin_segment,expected_frames_at, andpad_to, while the parameter and the field are namedwall_start_nsandwall_ns. The comment insession.rslines 45-52 states that mixing two_nsbases under one name is the defect this crate already paid for once. Rename these tomono_*or a neutralat_nsto keep the two bases distinguishable.Also applies to: 168-177
🤖 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/recorder/src/track.rs` around lines 151 - 156, Rename the segment anchor timestamp parameter and field used by begin_segment and related expected_frames_at/pad_to paths from wall_* to mono_* or a neutral at_ns, preserving the monotonic readings supplied by Session::start, Session::resume, and Session::pad_to. Update all corresponding references, including SegmentAnchor construction and access, so wall-clock and monotonic nanosecond bases remain distinguishable.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/recorder/src/main.rs`:
- Around line 85-112: Change the recorder loop around service.pump() so capture
data is drained on a fixed timer independently of stdin requests, using a
dedicated pumping thread or timed stdin-read approach. Ensure pumping continues
while the parent sends no requests and occurs before rendering responses so
status reflects current session state rather than the previous pump.
In `@crates/recorder/src/pump.rs`:
- Line 24: Replace the unbounded channel used by the recorder capture thread
with a bounded sync_channel sized for a few seconds of packets. In the capture
send loop around the channel producer, use try_send and increment a shared
thread_dropped counter when the queue is full; update lost_frames() to add this
counter to the source’s loss count so the manifest reports discarded audio.
- Around line 81-85: Update crates/recorder/src/pump.rs lines 81-85 in the
capture startup handling to store the error returned by source.start() in shared
state instead of discarding it, and update ThreadedSource::start around lines
212-216 to wait briefly for and return that error so service.rs rejects failed
starts. Also update crates/recorder/src/pump.rs lines 193-195 to distinguish
TryRecvError::Disconnected from Empty; after returning any accumulated samples,
report a terminal CaptureError for a disconnected capture thread rather than
falling through to Poll::Idle.
- Around line 180-191: Update ThreadedSource::poll to retain a DeviceChanged
event when preceding samples must be returned first, instead of dropping the
consumed event. Add and initialize a pending device-change field in
ThreadedSource::spawn, then have poll return that pending event before draining
later frames so the documented ordering and device-change reporting are
preserved.
In `@crates/recorder/src/service.rs`:
- Around line 74-79: The sequential capture startup paths lack rollback when
system capture fails. In crates/recorder/src/service.rs lines 74-79, stop the
microphone and call session.finalize_files() before returning the startup error;
in crates/recorder/src/session.rs lines 303-329, stop the microphone before
returning the system.start() error while preserving the existing state and open
PauseInterval.
In `@crates/recorder/src/track.rs`:
- Around line 204-212: Update append to truncate the input slice to a whole
number of interleaved frames before calling emit: calculate the usable sample
count from frames.len() and the effective channel count, write only that prefix,
and increment frames_written by the matching frame count. Preserve the existing
no-segment and empty-input early return behavior, and avoid emitting any partial
trailing frame.
- Around line 85-88: Update the recording flow in Service::pump to check
Session::at_size_limit() after pumping frames and stop or rotate the session
when it returns true, ensuring no further frames are written beyond the WAV size
threshold. Keep Session::at_size_limit() as the single size-limit calculation
and preserve normal pumping when the limit has not been reached.
In `@crates/recorder/src/wasapi_source.rs`:
- Around line 334-346: Throttle the default-device comparison in the
event-timeout branch around default_device_changed() to at most once per second
by storing and checking the last comparison time in the source state. Keep the
existing reopen and DeviceChanged behavior when the check detects a change,
while allowing timeout polls between checks to return Poll::Idle; leave the
needs_reopen path unchanged.
- Around line 303-305: Align the WASAPI discontinuity count with the public API
contract by renaming the `lost_frames` method and all propagation/manifest
consumers to `lost_events`, preserving the value as an event count rather than
presenting it as frames. Update the relevant trait, `WasapiSource`,
`ThreadedSource`, and manifest recording paths consistently; do not estimate
frame counts since WASAPI does not provide them.
- Around line 397-411: Update the `start` and `stop` methods to maintain the
`running` field: set it to true only after `start_stream()` succeeds, and set it
to false when stopping the stream. Preserve the existing error and closed-stream
behavior so `reopen()` can use `running` to restart previously active streams.
- Around line 192-196: Update reopen to recreate the stream when needs_reopen is
set even if default_device_changed reports the same device id, rather than
clearing the flag and returning immediately. Preserve the existing device-switch
behavior, and set self.changed = Some(name) only when the default device
actually moved so in-place stream recovery does not report a device change.
- Around line 256-271: Update the shared event-driven StreamMode::EventsShared
configuration in the recorder initialization flow to pass buffer_duration_hns as
zero, rather than deriving it from min_period and BUFFER_DURATION_HNS. Leave the
autoconvert setting and initialize_client call unchanged.
- Around line 19-22: Update the COM setup around initialize_mta in the affected
recorder entry points to require successful MTA initialization, propagating or
handling RPC_E_CHANGED_MODE as failure rather than accepting it; remove any
“safe to call twice” assumption and call wasapi::deinitialize() exactly once for
each successful initialization, including cleanup on early exits.
---
Nitpick comments:
In `@crates/recorder/src/capture.rs`:
- Around line 13-14: Align the Frames variant’s wall_ns contract with
Session::apply: either update its documentation to state that the value is
ignored and the session clock is used, or remove wall_ns and update all
constructors and callers, including real producers that currently pass 0.
In `@crates/recorder/src/pump.rs`:
- Around line 113-117: Update the poll-error handling loop in the recorder pump
to track consecutive errors without terminating the thread, publish the count or
latest error text through the existing status mechanism, and increase the retry
backoff to avoid a tight loop during persistent device failure. Reset the
consecutive-error state after a successful poll and preserve normal recovery
behavior.
In `@crates/recorder/src/rpc.rs`:
- Around line 78-82: Update the render fallback to construct the error response
through serde_json serialization rather than interpolating serde_json::Error
directly into a JSON string; ensure any quotes, backslashes, or other special
characters in the error text are escaped while preserving the existing fallback
message and return type.
In `@crates/recorder/src/service.rs`:
- Around line 168-192: Update manifest_of so each TrackInfo records the number
of samples actually accepted by the WAV, rather than relying on frames_written
alone. Use the existing failed-sample count from the session/finalization state
for both mic and system tracks, either by adding that field to TrackInfo or
subtracting it from frames, while preserving the manifest’s per-track data.
In `@crates/recorder/src/session.rs`:
- Around line 142-149: Update recorded_now_ns to use the same arithmetic and
frame-position source as recorded_ns: derive the offset from frames_written,
promote to u128 before multiplying by 1_000_000_000, then divide by the sample
rate and convert safely to u64. Reuse the existing recorded_ns helper or
centralize the calculation so DeviceChange timestamps and status recorded_ms
remain aligned.
In `@crates/recorder/src/track.rs`:
- Around line 151-156: Rename the segment anchor timestamp parameter and field
used by begin_segment and related expected_frames_at/pad_to paths from wall_* to
mono_* or a neutral at_ns, preserving the monotonic readings supplied by
Session::start, Session::resume, and Session::pad_to. Update all corresponding
references, including SegmentAnchor construction and access, so wall-clock and
monotonic nanosecond bases remain distinguishable.
In `@crates/recorder/src/wasapi_source.rs`:
- Around line 382-389: Update the byte extraction in the recorder method around
self.pending and samples to drain whole bytes from the VecDeque in bulk instead
of repeatedly calling pop_front, preserving the existing ordering and length.
Replace the hard-coded 4-byte chunk width with a value derived from the existing
BITS constant so sample decoding remains coupled to the configured sample size.
In `@crates/recorder/tests/rpc.rs`:
- Around line 89-95: Add a render test alongside the existing `Status` and
`Devices` tests that constructs `Response::Ok(Payload::Done { done: true })`,
parses the rendered JSON, and asserts the flattened `ok` and boolean `done`
fields. Keep the test focused on pinning the serialized layout for the `Done`
variant.
In `@crates/recorder/tests/session.rs`:
- Around line 307-320: Extend the test fixtures around BrokenSource with a
source variant whose start method returns Err, then add resume coverage using
it. Assert that when resume fails, both devices remain stopped and the session
state is unchanged, while preserving the existing poll-failure tests.
In `@crates/recorder/tests/track.rs`:
- Around line 165-173: Add focused tests in the track test module for the new
WAV paths: verify at_size_limit at its boundary, cover failed_samples behavior,
and use a temporary directory to test attach_wav followed by finalize,
confirming buffered samples are moved into the output file as described by
TrackWriter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4f79be2-cbfa-469d-84f9-97635ca52386
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/recorder/Cargo.tomlcrates/recorder/src/capture.rscrates/recorder/src/clock.rscrates/recorder/src/lib.rscrates/recorder/src/main.rscrates/recorder/src/manifest.rscrates/recorder/src/pump.rscrates/recorder/src/rpc.rscrates/recorder/src/service.rscrates/recorder/src/session.rscrates/recorder/src/timemap.rscrates/recorder/src/track.rscrates/recorder/src/wasapi_source.rscrates/recorder/tests/rpc.rscrates/recorder/tests/service.rscrates/recorder/tests/session.rscrates/recorder/tests/timemap.rscrates/recorder/tests/track.rsdocs/stack.mdplans/open-wiki.md
| // A buffer measured in hundreds of milliseconds, not the device minimum. | ||
| // The minimum is a few milliseconds, and anything the drain loop does not | ||
| // collect inside that window WASAPI overwrites — which the track then | ||
| // backfills with silence that looks, in every number this program | ||
| // reports, exactly like a healthy recording. | ||
| let buffer_duration_hns = min_period.max(BUFFER_DURATION_HNS); | ||
|
|
||
| // Shared mode with autoconvert: the meeting is not the only thing using the | ||
| // sound card, and exclusive mode would take it from whatever is. | ||
| let mode = StreamMode::EventsShared { | ||
| autoconvert: true, | ||
| buffer_duration_hns, | ||
| }; | ||
| client | ||
| .initialize_client(&wave, &which.stream_direction(), &mode) | ||
| .map_err(|e| err("could not initialise the stream", e))?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
wasapi crate StreamMode EventsShared buffer_duration_hns IAudioClient Initialize shared mode event callback hnsBufferDuration must be 0
💡 Result:
In the context of the Windows WASAPI API, the requirement that hnsBufferDuration must be 0 for shared-mode streams using event-driven buffering is a specific constraint imposed by the underlying IAudioClient::Initialize method [1][2]. According to official Microsoft documentation, for a shared-mode stream that uses the AUDCLNT_STREAMFLAGS_EVENTCALLBACK flag (event-driven buffering), the caller must set both the hnsPeriodicity and hnsBufferDuration parameters to 0 [1][2]. When these are set to 0, the Initialize method automatically determines the appropriate buffer size based on the audio engine's scheduling period [1][2]. In the Rust wasapi crate, the StreamMode::EventsShared variant allows you to specify a buffer_duration_hns [3][4]. While the crate provides this field, it is important to note that the WASAPI backend implementation will generally pass these values to the underlying IAudioClient::Initialize call [4][5]. If you are targeting Windows requirements for shared event-driven streams, you should be aware that passing a non-zero value may not behave as expected or could lead to initialization errors depending on how the underlying Windows audio engine interprets that specific value for a shared-mode request [1][2]. For optimal shared-mode performance and to follow recommended Windows practices, developers often use 0 to allow the system to handle buffer allocation [6]. If you require specific latency, the audio engine's default period (obtained via IAudioClient::GetDevicePeriod) is often used as a reference, but for strictly compliant event-driven shared-mode initialization as defined by Microsoft, setting the buffer duration to 0 is the documented standard [1][2][6].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-initialize
- 2: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/audioclient/nf-audioclient-iaudioclient-initialize.md
- 3: https://docs.rs/wasapi/latest/wasapi/enum.StreamMode.html
- 4: https://github.com/HEnquist/wasapi-rs/blob/master/src/api.rs
- 5: https://docs.rs/wasapi/latest/src/wasapi/api.rs.html
- 6: https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-getdeviceperiod
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(wasapi_source\.rs|pump\.rs|Cargo\.toml|Cargo\.lock)$|wasapi'
printf '%s\n' '--- relevant symbols and dependency declarations ---'
rg -n -C 4 'BUFFER_DURATION_HNS|EventsShared|initialize_client|wasapi' crates Cargo.toml Cargo.lock 2>/dev/null || trueRepository: protonspy/open-wiki
Length of output: 7557
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- wasapi_source.rs relevant implementation ---'
sed -n '1,70p;220,290p' crates/recorder/src/wasapi_source.rs
printf '%s\n' '--- pump.rs ---'
sed -n '1,240p' crates/recorder/src/pump.rs
printf '%s\n' '--- wasapi 0.23.0 source ---'
python3 - <<'PY'
import io, tarfile, urllib.request
url = "https://crates.io/api/v1/crates/wasapi/0.23.0/download"
data = urllib.request.urlopen(url, timeout=20).read()
with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as t:
for name in t.getnames():
if name.endswith(("src/api.rs", "src/stream.rs", "src/lib.rs")):
print(f"--- {name} ---")
text = t.extractfile(name).read().decode()
for i, line in enumerate(text.splitlines(), 1):
if ("EventsShared" in line or "buffer_duration_hns" in line or
"initialize_client" in line or "Initialize" in line or
"EVENTCALLBACK" in line):
lo, hi = max(1, i-8), min(len(text.splitlines()), i+12)
lines = text.splitlines()
print("\n".join(f"{n}: {lines[n-1]}" for n in range(lo, hi+1)))
print()
PYRepository: protonspy/open-wiki
Length of output: 16458
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
curl -kfsSL https://crates.io/api/v1/crates/wasapi/0.23.0/download -o "$tmp/wasapi.tgz"
tar -xzf "$tmp/wasapi.tgz" -C "$tmp"
src="$tmp/wasapi-0.23.0"
printf '%s\n' '--- source files ---'
find "$src/src" -maxdepth 2 -type f -print
printf '%s\n' '--- StreamMode and initialization definitions ---'
rg -n -C 12 'enum StreamMode|EventsShared|initialize_client|Initialize\(' "$src/src"
printf '%s\n' '--- read-only semantic verifier ---'
python3 - "$src" <<'PY'
from pathlib import Path
import re, sys
root = Path(sys.argv[1])
files = list(root.glob("src/**/*.rs"))
text = "\n".join(p.read_text(errors="replace") for p in files)
for needle in ("EventsShared", "buffer_duration_hns", "IAudioClient::Initialize", "Initialize("):
print(f"{needle}: {text.count(needle)} occurrence(s)")
m = re.search(r"EventsShared\s*\{.*?buffer_duration_hns.*?\}", text, re.S)
print("EventsShared declaration/call context found:", bool(m))
if m:
print(m.group(0)[:1000])
PYRepository: protonspy/open-wiki
Length of output: 259
🌐 Web query:
HEnquist wasapi-rs 0.23.0 src api.rs EventsShared buffer_duration_hns
💡 Result:
In the wasapi-rs crate, specifically within the StreamMode enum, EventsShared is a variant used to configure a WASAPI audio client for shared mode using event-driven timing [1][2]. The buffer_duration_hns field within this variant represents the desired device buffer duration measured in units of 100 nanoseconds [1][2]. When initializing an audio client with StreamMode::EventsShared, you provide a structure containing [2]: - autoconvert: A boolean indicating whether automatic format conversion should be enabled [2]. - buffer_duration_hns: An i64 value specifying the buffer duration in 100-nanosecond units [2]. In this mode, the audio engine determines the actual period, and this cannot be modified by the application [1][2]. Developers typically use the value returned by audio_client.get_device_period (specifically the default or minimum period) to populate the buffer_duration_hns field when initializing the client [3][4]. For example, a duration of 20 milliseconds is represented as 200,000 in this field (since 20ms = 20,000,000ns = 200,000 units of 100ns) [5][6].
Citations:
- 1: https://github.com/HEnquist/wasapi-rs/blob/master/src/api.rs
- 2: https://docs.rs/wasapi/latest/wasapi/enum.StreamMode.html
- 3: https://github.com/HEnquist/wasapi-rs/blob/master/examples/playsine_events.rs
- 4: https://github.com/HEnquist/wasapi-rs/blob/master/examples/loopback.rs
- 5: https://docs.rs/wasapi/latest/src/wasapi/api.rs.html
- 6: https://docs.rs/wasapi/latest/wasapi/struct.AudioClient.html
🌐 Web query:
site:github.com/HEnquist/wasapi-rs "buffer_duration_hns" "Initialize"
💡 Result:
In the context of the wasapi-rs Rust library, buffer_duration_hns is a parameter used to specify the audio device buffer duration in units of 100 nanoseconds [1]. It is not a standalone function but a field within the StreamMode enum variants (such as PollingShared, PollingExclusive, and EventsShared), which defines how an audio stream is configured [1]. To initialize a stream with this parameter, you include it within a StreamMode struct passed to the initialize_client method of an AudioClient [2][3]. Example initialization: let (def_time, min_time) = audio_client.get_device_period.unwrap; let mode = StreamMode::EventsShared { autoconvert: true, buffer_duration_hns: def_time, // Or min_time, depending on latency requirements }; audio_client.initialize_client(&desired_format, &Direction::Render, &mode).unwrap; The value for buffer_duration_hns is typically obtained from the audio device's capabilities via get_device_period, which returns the default and minimum supported periods [2][3]. Using the minimum period results in lower latency but requires more frequent processing [2].
Citations:
- 1: https://github.com/HEnquist/wasapi-rs/blob/master/src/api.rs
- 2: https://github.com/HEnquist/wasapi-rs/blob/master/examples/loopback.rs
- 3: https://github.com/HEnquist/wasapi-rs/blob/master/examples/playsine_events.rs
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
python3 - "$tmp/wasapi.tgz" <<'PY'
import ssl, sys, urllib.request
ctx = ssl._create_unverified_context()
with urllib.request.urlopen(
"https://crates.io/api/v1/crates/wasapi/0.23.0/download",
context=ctx,
timeout=30,
) as response:
open(sys.argv[1], "wb").write(response.read())
PY
tar -xzf "$tmp/wasapi.tgz" -C "$tmp"
src="$tmp/wasapi-0.23.0"
printf '%s\n' '--- exact source contexts ---'
rg -n -C 16 'enum StreamMode|EventsShared|buffer_duration_hns|initialize_client|Initialize\(' "$src/src"
printf '%s\n' '--- read-only semantic verifier ---'
python3 - "$src" <<'PY'
from pathlib import Path
import sys
root = Path(sys.argv[1])
for path in root.glob("src/**/*.rs"):
lines = path.read_text(errors="replace").splitlines()
for i, line in enumerate(lines):
if any(x in line for x in ("EventsShared", "buffer_duration_hns", "initialize_client", "Initialize(")):
print(f"{path.relative_to(root)}:{i+1}: {line.strip()}")
PYRepository: protonspy/open-wiki
Length of output: 30600
🌐 Web query:
site:learn.microsoft.com IAudioClient Initialize shared mode AUDCLNT_STREAMFLAGS_EVENTCALLBACK hnsBufferDuration 0
💡 Result:
When calling IAudioClient::Initialize in shared mode with the AUDCLNT_STREAMFLAGS_EVENTCALLBACK flag, setting the hnsBufferDuration parameter to 0 is a standard practice to achieve the lowest possible latency [1][2]. In this configuration, the audio engine automatically determines the buffer size based on its own scheduling period [1]. When hnsBufferDuration is set to 0, the Initialize method assumes the client's processing period matches the audio engine's period and allocates a buffer sized for the minimum latency [2]. Effectively, any hnsBufferDuration value provided between 0 and the sum of the audio engine's period and device latency will result in the same minimum buffer size [2]. Key requirements for this implementation include: 1. Shared Mode: The hnsPeriodicity parameter must be set to 0 in shared mode [3][1]. 2. Event Callback: Because the AUDCLNT_STREAMFLAGS_EVENTCALLBACK flag is used, you must call IAudioClient::SetEventHandle after initializing the client but before starting the stream to provide the event handle that the audio engine will signal when a buffer is ready [4][5]. 3. Processing: Even with event-driven buffering, the client must still call IAudioClient::GetCurrentPadding inside its processing loop to determine the amount of data available to read (for capture) or space available to write (for rendering) [1].
Citations:
- 1: https://learn.microsoft.com/en-us/previous-versions/ms678736(v=vs.85)
- 2: https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-getdeviceperiod
- 3: https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-initialize
- 4: https://learn.microsoft.com/en-us/windows/win32/coreaudio/audclnt-streamflags-xxx-constants
- 5: https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-seteventhandle
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ssl, urllib.request
from html.parser import HTMLParser
url = "https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-initialize"
html = urllib.request.urlopen(url, context=ssl._create_unverified_context(), timeout=30).read().decode("utf-8", "replace")
text = HTMLParser().unescape(html)
for needle in ("hnsBufferDuration", "EVENTCALLBACK", "shared-mode", "AUDCLNT_E_INVALID_DEVICE_PERIOD"):
print(f"--- {needle} ---")
start = 0
found = False
while True:
i = text.find(needle, start)
if i < 0:
break
found = True
print(" ".join(text[max(0, i-500):i+1000].split()))
start = i + len(needle)
if not found:
print("not found")
PYRepository: protonspy/open-wiki
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import html, re, ssl, urllib.request
url = "https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-initialize"
raw = urllib.request.urlopen(url, context=ssl._create_unverified_context(), timeout=30).read().decode("utf-8", "replace")
text = html.unescape(re.sub(r"<[^>]+>", " ", raw))
text = re.sub(r"\s+", " ", text)
for needle in ("hnsBufferDuration", "EVENTCALLBACK", "shared-mode", "AUDCLNT_E_INVALID_DEVICE_PERIOD"):
print(f"--- {needle} ---")
start = 0
count = 0
while True:
i = text.find(needle, start)
if i < 0:
break
print(text[max(0, i-500):i+1200])
start = i + len(needle)
count += 1
if not count:
print("not found")
PYRepository: protonspy/open-wiki
Length of output: 50375
Pass zero duration to shared event-driven WASAPI streams. wasapi 0.23.0 passes buffer_duration_hns directly to IAudioClient::Initialize with hnsPeriodicity = 0 and AUDCLNT_STREAMFLAGS_EVENTCALLBACK. Windows requires both values to be 0 in shared event-driven mode. Set buffer_duration_hns to 0; otherwise initialize_client can fail and prevent recording.
🤖 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/recorder/src/wasapi_source.rs` around lines 256 - 271, Update the
shared event-driven StreamMode::EventsShared configuration in the recorder
initialization flow to pass buffer_duration_hns as zero, rather than deriving it
from min_period and BUFFER_DURATION_HNS. Leave the autoconvert setting and
initialize_client call unchanged.
| fn lost_frames(&self) -> u64 { | ||
| self.discontinuities | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
lost_frames() returns a count of discontinuity events, not a count of frames.
Line 372 increments discontinuities by one per data_discontinuity flag. Line 304 returns that value from lost_frames().
The trait contract at capture.rs lines 52-54 defines the return value as "How many frames the device reported it overwrote". The field doc at lines 152-154 correctly describes a count of occurrences. The two units disagree, and the trait boundary is the one consumers read. pump.rs line 107 propagates the value unchanged into ThreadedSource::lost_frames.
A single discontinuity can cover thousands of frames. Any consumer that converts lost_frames() into a duration, or reconciles it against frames_written in the manifest, produces a number that understates the loss by orders of magnitude. The stated purpose of the field is that a lossy recording can say so.
WASAPI does not report the overwritten frame count, so an exact figure is not available. Align the units instead: rename the trait method to lost_events and record the event count in the manifest, or estimate frames from the gap between the device position and the frames received.
🤖 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/recorder/src/wasapi_source.rs` around lines 303 - 305, Align the
WASAPI discontinuity count with the public API contract by renaming the
`lost_frames` method and all propagation/manifest consumers to `lost_events`,
preserving the value as an event count rather than presenting it as frames.
Update the relevant trait, `WasapiSource`, `ThreadedSource`, and manifest
recording paths consistently; do not estimate frame counts since WASAPI does not
provide them.
| if stream.event.wait_for_event(20).is_err() { | ||
| // The stream may have died with the device. Check before giving up. | ||
| if self.default_device_changed().is_some() { | ||
| // A failure here is not fatal: `needs_reopen` makes the next | ||
| // poll try again rather than leaving the track dead. | ||
| let _ = self.reopen(); | ||
| return Ok(self | ||
| .changed | ||
| .take() | ||
| .map_or(Poll::Idle, |device| Poll::DeviceChanged { device })); | ||
| } | ||
| return Ok(Poll::Idle); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Throttle the default-device check on the event timeout; it runs on the common path.
Line 334 waits 20 ms for the event. Line 336 then calls default_device_changed(), which builds a new DeviceEnumerator, fetches the default device, and reads its id — three COM calls per invocation, per default_device at lines 229-236.
For the loopback source this timeout is the normal state, not the exception. capture.rs lines 15-17 state that a silent machine produces nothing at all. So during a quiet meeting this runs about 50 times per second for each source, which is roughly 180,000 device enumerations per hour per source, on the capture thread.
A device change does not need 20 ms detection latency. Record the last check time and run the comparison at most once per second. The needs_reopen path at line 364 still catches an invalidated device immediately, because that path does not depend on this poll.
🤖 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/recorder/src/wasapi_source.rs` around lines 334 - 346, Throttle the
default-device comparison in the event-timeout branch around
default_device_changed() to at most once per second by storing and checking the
last comparison time in the source state. Keep the existing reopen and
DeviceChanged behavior when the check detects a change, while allowing timeout
polls between checks to return Poll::Idle; leave the needs_reopen path
unchanged.
| fn stop(&mut self) { | ||
| if let Some(stream) = self.stream.as_mut() { | ||
| let _ = stream.client.stop_stream(); | ||
| } | ||
| } | ||
|
|
||
| fn start(&mut self) -> Result<(), CaptureError> { | ||
| match self.stream.as_mut() { | ||
| Some(stream) => stream | ||
| .client | ||
| .start_stream() | ||
| .map_err(|e| err("could not restart the stream", e)), | ||
| None => Err(CaptureError("the stream is closed".into())), | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
self.running is never set, so a stream reopened after a device change never restarts.
running is declared at line 156 and initialised to false at line 179. No code assigns it after that. start() starts the stream but does not set running = true. stop() stops the stream but does not set running = false.
The only reader is line 219 in reopen(). That guard restarts the freshly opened stream only when running is true. Because running is always false, reopen() installs a stream that is never started. poll() then finds no packets, the session pads both tracks with silence, and the recording reports as healthy for the rest of the meeting.
That is the failure the comment at lines 197-201 describes and the case plan 4.2 exists to survive. Maintain the flag in start and stop.
🐛 Proposed fix
fn stop(&mut self) {
if let Some(stream) = self.stream.as_mut() {
let _ = stream.client.stop_stream();
}
+ self.running = false;
}
fn start(&mut self) -> Result<(), CaptureError> {
match self.stream.as_mut() {
- Some(stream) => stream
- .client
- .start_stream()
- .map_err(|e| err("could not restart the stream", e)),
+ Some(stream) => {
+ stream
+ .client
+ .start_stream()
+ .map_err(|e| err("could not restart the stream", e))?;
+ // `reopen` restarts the new stream only when this is set.
+ self.running = true;
+ Ok(())
+ }
None => Err(CaptureError("the stream is closed".into())),
}
}A test that reopens a source while it is started would have caught this. The running field has no coverage today.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn stop(&mut self) { | |
| if let Some(stream) = self.stream.as_mut() { | |
| let _ = stream.client.stop_stream(); | |
| } | |
| } | |
| fn start(&mut self) -> Result<(), CaptureError> { | |
| match self.stream.as_mut() { | |
| Some(stream) => stream | |
| .client | |
| .start_stream() | |
| .map_err(|e| err("could not restart the stream", e)), | |
| None => Err(CaptureError("the stream is closed".into())), | |
| } | |
| } | |
| fn stop(&mut self) { | |
| if let Some(stream) = self.stream.as_mut() { | |
| let _ = stream.client.stop_stream(); | |
| } | |
| self.running = false; | |
| } | |
| fn start(&mut self) -> Result<(), CaptureError> { | |
| match self.stream.as_mut() { | |
| Some(stream) => { | |
| stream | |
| .client | |
| .start_stream() | |
| .map_err(|e| err("could not restart the stream", e))?; | |
| // `reopen` restarts the new stream only when this is set. | |
| self.running = true; | |
| Ok(()) | |
| } | |
| None => Err(CaptureError("the stream is closed".into())), | |
| } | |
| } |
🤖 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/recorder/src/wasapi_source.rs` around lines 397 - 411, Update the
`start` and `stop` methods to maintain the `running` field: set it to true only
after `start_stream()` succeeds, and set it to false when stopping the stream.
Preserve the existing error and closed-stream behavior so `reopen()` can use
`running` to restart previously active streams.
… dropping it Taking it out of the channel and then returning the audio that preceded it let the event fall out of scope: the recording carried on with no record that the device had moved, which is the one thing 4.2 exists to write down. The comment claimed it stayed queued; it did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
Not merging this yet — outstanding review findingsThe Critical is fixed (a But CodeRabbit raised five Majors on the capture-thread code that I added late in this branch, and they share a failure mode this component must not have — losing audio while reporting success. Merging with them open would contradict the reason the threading exists. Listed here so they are not lost:
Plus two minors: truncate Also still outstanding, and not something CI can close: the three manual checks on an hour-long recording that this group's notes make an acceptance criterion. Two of the three defects that meant the binary could not record on Windows were only findable by reading the source — a device is the only thing that can confirm the rest. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/recorder/src/pump.rs (1)
186-211: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve timestamps and idle boundaries while batching.
The
Pollcontract incrates/recorder/src/capture.rs:12-22requires frame timestamps and usesIdleto represent silence. This code discards the inputwall_ns, ignoresIdleevents, and returnswall_ns: 0. A queue containingFrames, Idle, Framesbecomes one contiguous packet, which removes silence and corrupts time mapping.Stop batching at non-frame events, retain them as deferred events, and return the first frame timestamp.
🐛 Preserve packet metadata and event boundaries
- Ok(Poll::Frames { samples: more, .. }) => samples.extend_from_slice(&more), + Ok(Poll::Frames { + wall_ns: first_wall_ns, + samples: more, + }) => { + wall_ns.get_or_insert(first_wall_ns); + samples.extend_from_slice(&more); + } ... - Ok(Poll::Idle) => {} + Ok(event @ Poll::Idle) => { + if samples.is_empty() { + return Ok(event); + } + self.deferred = Some(event); + break; + } ... - wall_ns: 0, + wall_ns: wall_ns.unwrap_or(0),🤖 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/recorder/src/pump.rs` around lines 186 - 211, Update the batching logic in the receiver loop around self.rx.try_recv() to stop when encountering Poll::Idle or Poll::DeviceChanged, preserving those events for the next call instead of consuming them. Retain the wall_ns from the first Poll::Frames event and return it with the accumulated samples, so frame batches preserve timestamps and do not cross idle or device-change boundaries.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@crates/recorder/src/pump.rs`:
- Around line 186-211: Update the batching logic in the receiver loop around
self.rx.try_recv() to stop when encountering Poll::Idle or Poll::DeviceChanged,
preserving those events for the next call instead of consuming them. Retain the
wall_ns from the first Poll::Frames event and return it with the accumulated
samples, so frame batches preserve timestamps and do not cross idle or
device-change boundaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fe9254e-c490-4dc1-8281-e62f2dd01620
📒 Files selected for processing (2)
crates/recorder/src/pump.rscrates/recorder/tests/session.rs
The previous commit claimed the dropped `DeviceChanged` was fixed and it was not: `cargo fmt` had reformatted the block, so the edit that adds `self.deferred = Some(...)` never matched and only the field and the `take()` landed. The event was still consumed and discarded. The regression test written this round is what caught it — the earlier commit had none, which is why the claim went unchallenged. Also, from the same review: - The capture queue is bounded. Unbounded, the threads pushed ~384 KB/s per device for as long as the session went without polling, and nothing capped it. `try_send`, never `send`, because a blocked drain is exactly how WASAPI comes to overwrite frames nobody collected — the oldest audio is lost instead, and counted. - The root cause of that queue filling is gone too: stdin now has a thread of its own and the loop pumps on a 50 ms timer. Blocking on `lines()` and pumping once per request meant capture only advanced when the parent spoke, and the parent has no reason to say anything between `start` and `stop`. - A capture thread that dies is visible. The only channel back carried `Poll`, so a thread that failed and returned looked exactly like loopback with nothing playing. `status` now carries the fault, the dropped-sample count and the discontinuity count: silence nobody flagged, presented as a healthy hour, is the failure this sidecar exists to avoid. - A stream invalidated on the *still-default* device is reopened. `reopen` only acted when the device id differed and cleared the retry flag when it did not, so a sleep/resume or a driver reset left the track dead for the rest of the meeting. - A half-started pair is rolled back, instead of leaving one client running for the next start to trip over. - `lost_frames` is `discontinuities`, which is what it counts: Windows reports that a gap happened, not how large it was. - `append` truncates to whole frames, so a partial frame cannot reach the file uncounted and skew every later `pad_to`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/recorder/src/capture.rs (1)
1-33: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve
Poll::DeviceChangedwhen the audio queue is full.A full bounded queue discards
Poll::DeviceChangedwithout recording it. The session can then receive frames from the new device without the device-change boundary. Store control events in a deferred or separate priority queue. Add a regression test that fills the audio queue before sendingPoll::DeviceChanged.🤖 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/recorder/src/capture.rs` around lines 1 - 33, The bounded audio-queue handling must preserve Poll::DeviceChanged instead of dropping it when capacity is exhausted. Update the queue/send logic around Poll and DeviceChanged to defer control events or route them through a separate priority queue, ensuring the session observes the device-change boundary before subsequent frames. Add a regression test that fills the audio queue, sends Poll::DeviceChanged, and verifies the event is recorded.
🧹 Nitpick comments (1)
crates/recorder/src/capture.rs (1)
1-33: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the unused
Poll::Frames.wall_nsfield.Sessionignores it and uses its shared monotonic clock for alignment.🤖 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/recorder/src/capture.rs` around lines 1 - 33, Remove the wall_ns field from the Poll::Frames variant and update its documentation to omit the wall-clock timestamp. Adjust all construction and pattern-matching sites for Poll::Frames to use only the samples payload, while preserving Session’s shared monotonic-clock alignment.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/recorder/src/capture.rs`:
- Around line 64-68: Make the source health contract required and update the
capture implementation’s health() to return the shared worker fault instead of
always returning Ok. In the Command::Start handling in pump.rs, propagate
source.start() failures into the shared fault state so a failed resume is
reported by Service::status(); add coverage for a second start() failure and its
health result.
- Around line 52-63: Scope loss counters to each recording by capturing
per-source baselines when handling Request::Start, or resetting both counters
only after both sources start successfully, and have Service::status() report
deltas for the active recording. Ensure retained sources and
ThreadedSource::start do not carry prior recording losses into later results,
and add a regression test covering sequential recordings.
---
Outside diff comments:
In `@crates/recorder/src/capture.rs`:
- Around line 1-33: The bounded audio-queue handling must preserve
Poll::DeviceChanged instead of dropping it when capacity is exhausted. Update
the queue/send logic around Poll and DeviceChanged to defer control events or
route them through a separate priority queue, ensuring the session observes the
device-change boundary before subsequent frames. Add a regression test that
fills the audio queue, sends Poll::DeviceChanged, and verifies the event is
recorded.
---
Nitpick comments:
In `@crates/recorder/src/capture.rs`:
- Around line 1-33: Remove the wall_ns field from the Poll::Frames variant and
update its documentation to omit the wall-clock timestamp. Adjust all
construction and pattern-matching sites for Poll::Frames to use only the samples
payload, while preserving Session’s shared monotonic-clock alignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b23e577-9f4e-4bfb-95f7-94982369514b
📒 Files selected for processing (9)
crates/recorder/src/capture.rscrates/recorder/src/main.rscrates/recorder/src/pump.rscrates/recorder/src/rpc.rscrates/recorder/src/service.rscrates/recorder/src/track.rscrates/recorder/src/wasapi_source.rscrates/recorder/tests/rpc.rscrates/recorder/tests/service.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/recorder/tests/rpc.rs
- crates/recorder/tests/service.rs
- crates/recorder/src/track.rs
- crates/recorder/src/main.rs
- crates/recorder/src/service.rs
- crates/recorder/src/pump.rs
- crates/recorder/src/wasapi_source.rs
| /// How many times the device reported it had overwritten frames nobody | ||
| /// collected. An **event** count, not a frame count — Windows says that a | ||
| /// gap happened, not how big it was. | ||
| fn discontinuities(&self) -> u64 { | ||
| 0 | ||
| } | ||
| /// Samples that reached this process and were then thrown away because the | ||
| /// queue was full. A recording that lost audio must be able to say so | ||
| /// rather than presenting manufactured silence as the real thing. | ||
| fn dropped_samples(&self) -> u64 { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 \
'AtomicU64|fn start|dropped_samples|discontinuities|Request::Start' \
crates/recorder/src/pump.rs \
crates/recorder/src/service.rs \
crates/recorder/src/capture.rsRepository: protonspy/open-wiki
Length of output: 17735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/recorder/src/pump.rs
ast-grep outline crates/recorder/src/service.rs
ast-grep outline crates/recorder/src/session.rs 2>/dev/null || true
printf '\n--- pump.rs ---\n'
sed -n '55,225p' crates/recorder/src/pump.rs
sed -n '260,292p' crates/recorder/src/pump.rs
printf '\n--- service.rs ---\n'
sed -n '54,145p' crates/recorder/src/service.rs
printf '\n--- session references ---\n'
rg -n -C 8 'struct Session|impl.*Session|fn start|fn stop|fn state|mic\(|system\(' crates/recorder/srcRepository: protonspy/open-wiki
Length of output: 29734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- service construction and tests ---'
sed -n '1,55p' crates/recorder/src/service.rs
rg -n -C 12 'Service::new|ThreadedSource::spawn|Request::Start|dropped_samples|discontinuities|sequential|recording' crates/recorder --glob '*.rs'
printf '%s\n' '--- source implementations and counter updates ---'
rg -n -C 10 'discontinu|lost|dropped_samples|dropped\.|AtomicU64' crates/recorder/src --glob '*.rs'
printf '%s\n' '--- service and pump tests ---'
fd -i -t f 'test|tests' crates/recorder
rg -n -C 8 '#\[test\]' crates/recorder/src crates/recorder/tests 2>/dev/null || trueRepository: protonspy/open-wiki
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- WASAPI counter implementation ---'
rg -n -C 14 'discontinu|lost|dropped|capture_lost|GetNextPacketSize|GetBuffer' \
crates/recorder/src/wasapi_source.rs
printf '%s\n' '--- focused lifecycle verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
pump = Path("crates/recorder/src/pump.rs").read_text()
service = Path("crates/recorder/src/service.rs").read_text()
spawn = pump[pump.index("pub fn spawn"):pump.index("impl CaptureSource for ThreadedSource")]
start = pump[pump.index(" fn start(&mut self) -> Result<(), CaptureError>"):pump.index("impl Drop for ThreadedSource")]
status = service[service.index(" pub fn status"):service.index(" }\n}", service.index(" pub fn status")) + 5]
handle = service[service.index(" pub fn handle"):service.index(" pub fn status")]
checks = {
"counters initialized in spawn": "AtomicU64::new(0)" in spawn,
"start has no counter reset": not re.search(r"(lost|dropped).*(store|swap|fetch_sub)", start, re.S),
"start only sends Start": "send(Command::Start)" in start,
"status reads raw source counters": "self.mic.dropped_samples()" in status
and "self.system.dropped_samples()" in status
and "self.mic.discontinuities()" in status
and "self.system.discontinuities()" in status,
"session is cleared on stop": "self.session = None;" in handle,
"capture sources are not recreated on stop": "ThreadedSource::spawn" not in handle,
}
for name, result in checks.items():
print(f"{name}: {result}")
assert all(checks.values())
print("result: one ThreadedSource counter lifetime can span multiple Service sessions")
PYRepository: protonspy/open-wiki
Length of output: 6934
Scope loss counters to one recording.
Service::status() reads cumulative source counters. Request::Stop retains the sources, and ThreadedSource::start does not reset them. A later recording therefore reports loss from earlier recordings.
Store per-source baselines at Request::Start, or reset both counters after both sources start successfully. Add a sequential-recording regression test.
🤖 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/recorder/src/capture.rs` around lines 52 - 63, Scope loss counters to
each recording by capturing per-source baselines when handling Request::Start,
or resetting both counters only after both sources start successfully, and have
Service::status() report deltas for the active recording. Ensure retained
sources and ThreadedSource::start do not carry prior recording losses into later
results, and add a regression test covering sequential recordings.
| /// `Err` when capture has stopped working. A source that has died must not | ||
| /// look like one that is merely silent. | ||
| fn health(&self) -> Result<(), String> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate start failures through health().
Service::status() relies on health() to distinguish a dead source from silence. In crates/recorder/src/pump.rs, the Command::Start branch ignores source.start() errors and does not set the shared fault. A failed resume can therefore leave capture stopped while capture_fault remains unset.
Record the error in the worker fault state and return it from health(). Consider making health() required so a failing source cannot inherit the healthy default. Add a test where the second start() call fails.
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'Command::Start|source\.start\(\)|thread_fault|fn health' \
crates/recorder/src/pump.rs \
crates/recorder/src/service.rs🤖 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/recorder/src/capture.rs` around lines 64 - 68, Make the source health
contract required and update the capture implementation’s health() to return the
shared worker fault instead of always returning Ok. In the Command::Start
handling in pump.rs, propagate source.start() failures into the shared fault
state so a failed resume is reported by Service::status(); add coverage for a
second start() failure and its health result.
1 participant
Closes plan tasks 4.1–4.5. 64 Rust tests; the three TDD tasks were built RED first.
Shape
The device sits behind a
CaptureSourcetrait. Everything that can be wrong without a symptom — track alignment, the silence that stands in for frames the API never sends, the pause arithmetic, the time map, the manifest, the JSON-RPC — lives above that line and is tested on every platform. WASAPI is a thin edge.Two bugs the TDD caught before they shipped:
No test here has captured a real frame — CI has no audio device. I compiled and clippy'd the WASAPI path for
x86_64-pc-windows-msvclocally, so the FFI type-checks against the real crate API, but green CI means the session logic is right and the binary builds. It does not mean audio works.That is not a hypothetical caveat. Reading the source, the reviews found three defects that meant the binary as first staged could not record on Windows at all:
(Render, Render). The crate derivesAUDCLNT_STREAMFLAGS_LOOPBACKfrom the pair — device Render, stream Capture. Passing (Render, Render) matched nothing, so the flag was never set, the client initialised as a plain playback stream, and asking it for anIAudioCaptureClientfailed — taking the sidecar down at launch. The device direction and the stream direction are two decisions that I had written as one function.open_streamstarted the stream, so the firststartrequest hitAUDCLNT_E_NOT_STOPPEDand dropped the session that had just been built, leaving two empty WAVs.min_period. Continuous capture would have required ~300 requests/second and could not have exceeded ~50. Everything missed was backfilled with manufactured silence — so an hour of nothing would report the right length, the right frame count, and a healthy time map.All compiled. All were invisible to every test that does not touch a device. The three manual checks the plan's group 4 notes require are the only thing that can close this, and they are outstanding — I've written that onto the plan's 4.1 line so a later reader cannot mistake a green build for working audio.
The rest of the review findings, all fixed
Send, which only surfaced by compiling for Windows.resumeanchored on the inflated figure, making it permanent and cumulative across pauses.first_framescounts real frames rather than manufactured silence — it wasnullfor both tracks in every realistic recording, and 4.4 names it as one of the three things the manifest is for.u64fields all named_nswith nothing to tell them apart — a comparison 4.7 and 4.11 would have got wrong by 1.7e18.Not needed after all
The workspace's
unsafe_code = "deny"did not have to be lifted. The plan anticipated group 4 relaxing it for WASAPI; thewasapicrate holds the unsafe, so this crate contains none. I updated the lint's comment rather than leave it promising a change that never happened.Verified
cargo test64 passing ·cargo clippy --all-targets -- -D warningsclean ·cargo fmt --checkclean — on both the Linux host andx86_64-pc-windows-msvcscc validate0 findings; all three new dependencies recorded indocs/stack.md🤖 Generated with Claude Code
https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
Summary by CodeRabbit
New Features
Documentation
Tests