Skip to content

Disconnect evicted capture streams before dropping them - #69

Open
HoneyHazard wants to merge 1 commit into
tsowell:mainfrom
HoneyHazard:fix-capture-stream-disconnect-leak
Open

Disconnect evicted capture streams before dropping them#69
HoneyHazard wants to merge 1 commit into
tsowell:mainfrom
HoneyHazard:fix-capture-stream-disconnect-leak

Conversation

@HoneyHazard

@HoneyHazard HoneyHazard commented Aug 6, 2026

Copy link
Copy Markdown

⚠️ Full Disclosure: Drafted with AI assistance (Claude); reviewed by me briefly. I am neither a RUST developer nor pipewire expert. If I should stop making these PRs into your wonderful project, please let me know. ⚠️

That being said, I hope these can be helpful and useful additions that users could appreciate.


Found this while working on a separate feature that counts "wiremix-capture" nodes in the shared PipeWire graph - it kept seeing more capture nodes than wiremix's own bookkeeping (capturing_objects) accounted for, and it turned out to be a real leak, not a counting bug.

StreamRegistry::add_stream() evicts any existing stream already registered under the same stream_id before inserting the new one:

if let Some(old) = self.streams.insert(stream_id, stream) {
    self.garbage_streams.push(old);
    ...

StreamRegistry::remove() does the same eviction, but calls .disconnect() on the outgoing stream first:

if let Some(stream) = self.streams.remove(&stream_id) {
    let _ = stream.disconnect();
    self.garbage_streams.push(stream);
    ...

add_stream() was missing that .disconnect() call. Dropping the evicted StreamRc alone only destroys the client-side stream object - StreamBox's Drop impl calls pw_stream_destroy directly, never pw_stream_disconnect. Without an explicit disconnect first, the corresponding node can be left registered in the graph, invisible to wiremix's own state but still visible to every other PipeWire client, until something else eventually tears it down.

This path is hit every time a capture is renewed after CaptureEligibility::NeedsRestart - start_capture() gets called again for an object_id that already has an active stream, so add_stream()'s eviction branch runs on every restart, not just on genuinely new captures.

Verified live (this module has no existing test coverage - StreamRc requires a real, connected PipeWire core to construct, nothing here is mocked):

  • Before the fix: launched a test instance, forced repeated link churn against a node (so it kept needing NeedsRestart), and watched pw-dump for "wiremix-capture"-named nodes climb and stay elevated well past what the instance's own capturing_objects said it was capturing.
  • After the fix: same repeated-restart stress, pw-dump's count tracked the instance's actual active capture count exactly, and clean shutdown (Ctrl+C) always returned the graph to baseline with nothing orphaned.
  • cargo test --release: 144/144 passing (no regressions; this fix doesn't touch anything under existing test coverage)
  • cargo fmt --check / cargo clippy -- -D warnings / cargo doc (matching this repo's CI): all clean

StreamRegistry::add_stream() evicted an existing stream from its map
without calling disconnect() on it first, unlike remove(), which
already did. Dropping the evicted StreamRc alone destroys the
client-side stream object (pw_stream_destroy) without ever calling
pw_stream_disconnect, so the corresponding PipeWire node can be left
registered in the graph until something else tears it down.

This path is hit every time a capture is renewed after
CaptureEligibility::NeedsRestart, since that calls start_capture()
again for an object_id that already has an active stream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant