fix(clientcore): stop leaking goroutines and connections on engine teardown

[garmr-ulfr]

Problem

On engine stop, several long-lived goroutines and a live peer connection were leaked on every connect/disconnect cycle:

• The bus observer, table routers, and downstream UI ticker had no shutdown path and ran until process exit.
• The passive user-stream WorkerFSM state blocked on a bare select{}, so its Start goroutine never returned to release its WaitGroup token — hanging engine teardown.
• A WorkerFSM carrying a live *webrtc.PeerConnection (or coder/websocket Conn) discarded it unclosed when cancelled between states, stranding the connection's pion interceptor goroutines (they start at PeerConnection construction).

Under repeated connect/disconnect this accumulates goroutines and heap without bound.

Fix

• Thread an engine-owned context into the bus observer, routers, and UI ticker so stop() cancels them; guard router sends on that context so a send can't block teardown.
• Have the passive user-stream state block on <-ctx.Done() and return, so its goroutine releases the WaitGroup token.
• On the WorkerFSM cancellation exit, close whatever connection the interrupted state was carrying (io.Closer for the pion PeerConnection, CloseNow() for coder/websocket), plus the producer state-1 cancel path.

Verification

• Profiled under a repeated connect/disconnect loop: the engine's long-lived goroutines (routers, bus observer, UI ticker) and per-worker FSM goroutines no longer survive teardown, and pion PeerConnection interceptor goroutines drop to zero at the disconnected state (previously accumulated every cycle). The per-cycle newIpcChan bus residue no longer ratchets.

Summary by CodeRabbit

• Bug Fixes

  • Improved shutdown handling across engine, routing, IPC, and worker components.
  • Active WebRTC connections and worker resources now close cleanly when operations are canceled.
  • Prevented messages from being sent to unavailable workers.
  • Improved termination of user stream producers and background processing.

• Performance

  • Updated throughput monitoring to use scheduled refresh intervals for more consistent updates.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2f9d8c86-e6aa-4943-9ffb-ec183c454eea

📥 Commits

Reviewing files that changed from the base of the PR and between 2c9b6f0 and 935bb82.

📒 Files selected for processing (4)

• clientcore/broflake.go
• clientcore/producer.go
• clientcore/ui.go
• clientcore/user.go

🚧 Files skipped from review as they are similar to previous changes (3)

• clientcore/user.go
• clientcore/ui.go
• clientcore/broflake.go

---

📝 Walkthrough

Walkthrough

BroflakeEngine now owns a cancelable context passed to IPC, routing, and UI components. IPC and router loops honor cancellation and closed channels, while worker FSMs, WebRTC producers, and UI throughput handling perform explicit shutdown cleanup.

Changes

Lifecycle cancellation and cleanup

Layer / File(s)	Summary
Engine context wiring clientcore/broflake.go, clientcore/ui.go	BroflakeEngine creates and cancels a lifecycle context, which is passed to IPC, routers, and downstream UI handling; UI updates use a stoppable ticker.
Context-aware IPC and routing clientcore/ipc.go, clientcore/routing.go	IPC forwarding and router receive/send paths stop on cancellation or channel closure, and downstream worker targets are validated.
Worker resource cleanup clientcore/protocol.go, clientcore/producer.go, clientcore/user.go	FSM cancellation closes pending resources and WebRTC peer connections, while producer user state waits for context cancellation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant BroflakeEngine
  participant IPCObserver
  participant TableRouter
  participant WorkerFSM
  BroflakeEngine->>IPCObserver: Start(ctx)
  BroflakeEngine->>TableRouter: Init(ctx)
  BroflakeEngine->>WorkerFSM: propagate cancellation
  BroflakeEngine->>BroflakeEngine: cancel()
  IPCObserver-->>IPCObserver: stop forwarding
  TableRouter-->>TableRouter: stop routing
  WorkerFSM-->>WorkerFSM: close pending resource

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title clearly reflects the main change: fixing clientcore teardown leaks by stopping goroutines and connections.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch garmr/fix-engine-teardown-leak

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR addresses resource leaks during BroflakeEngine teardown by introducing context-driven shutdown paths for long-lived goroutines (bus observer, routers, UI ticker) and ensuring WorkerFSM states release their WaitGroup token and close carried connection resources when cancelled.

Changes:

• Replace an infinite select {} in the passive user-stream producer state with <-ctx.Done() so the WorkerFSM goroutine can exit cleanly on Stop.
• Thread an engine-owned context.Context into the bus observer, table routers, and downstream UI ticker to support cancellation-driven shutdown.
• Close connection-like resources on WorkerFSM cancellation (generic closure in WorkerFSM.Start() plus an explicit PeerConnection.Close() on a cancellation path in producer state 1).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
clientcore/broflake.go	Adds engine-owned context/cancel and wires it into bus/router/UI startup; triggers cancellation on stop.
clientcore/ipc.go	Makes the IPC observer forwarders context-cancellable to avoid goroutine leaks.
clientcore/protocol.go	Adds generic “close carried resource on cancellation” logic to WorkerFSM shutdown.
clientcore/producer.go	Ensures a carried *webrtc.PeerConnection is closed when a state is cancelled mid-loop.
clientcore/routing.go	Adds context-aware recv/send helpers so router goroutines can stop and router sends don’t block teardown.
clientcore/ui.go	Replaces time.After loop with a time.Ticker and adds context cancellation for the UI throughput ticker goroutine.
clientcore/user.go	Fixes passive producer state to block on context cancellation rather than an infinite select.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

clientcore/broflake.go (1)

24-94: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Recreate the engine context before restart

stop() cancels the only ctx, but start() never reinitializes ctx/cancel. The wasm UI also expects stop → ready → start to be a valid cycle, so a same-instance restart leaves bus.Start, both routers, and DownstreamUIHandler attached to a dead context. Recreate the engine wiring in start(), or make reuse after stop() unsupported.

🤖 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 `@clientcore/broflake.go` around lines 24 - 94, The start/stop lifecycle leaves
BroflakeEngine.ctx canceled across same-instance restarts. Update start() to
recreate the context and cancel function before starting the worker tables and
dependent components, preserving the stop() cancellation flow so stop → ready →
start attaches bus.Start, both routers, and DownstreamUIHandler to a live
context.

🧹 Nitpick comments (2)

clientcore/producer.go (1)

148-155: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Good fix — but sibling State 4 has the same shape of gap.

Closing peerConnection here is correct and necessary; without it, closeWorkerResource(fsm.nextInput) in protocol.go has nothing to close since this state returns an empty slice.

Worth confirming whether the unchanged State 4 (further down, its select only covers connectionEstablished / time.After(options.NATFailTimeout)) is intentionally out of scope. If it isn't, an FSM parked in State 4 during teardown will only unblock after NATFailTimeout elapses rather than reacting to cancellation immediately, delaying the WaitGroup release covered by this PR's stated goal.

🤖 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 `@clientcore/producer.go` around lines 148 - 155, Update State 4’s select loop
to handle ctx.Done() immediately, close the carried peerConnection, and return
the empty input state so teardown does not wait for NATFailTimeout. Preserve the
existing connectionEstablished and timeout behavior.

clientcore/ui.go (1)

78-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Ticker cancellation logic is correct; fix the duration typing flagged by static analysis.

tickMs is declared as time.Duration but actually holds a plain millisecond count, then gets multiplied by time.Millisecond again — this is exactly what durationcheck (duration×duration multiplication) and staticcheck ST1011 (unit-suffixed Duration variable) are warning about. It happens to compute the right value here, but it's a latent bug pattern.

♻️ Suggested fix

-	var bytesPerSec int64
-	var tick uint
-	tickMs := time.Duration(1000 / uiRefreshHz)
+	var bytesPerSec int64
+	var tick uint
+	tickInterval := time.Second / uiRefreshHz
 
 	go func() {
-		ticker := time.NewTicker(tickMs * time.Millisecond)
+		ticker := time.NewTicker(tickInterval)

🤖 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 `@clientcore/ui.go` around lines 78 - 97, Update tickMs in DownstreamUIHandler
to represent the ticker interval as a proper time.Duration once, avoiding
multiplication of a duration by time.Millisecond; rename it to remove the
misleading unit suffix and pass it directly to time.NewTicker.

Source: Linters/SAST tools

🤖 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 `@clientcore/broflake.go`:
- Around line 24-94: The start/stop lifecycle leaves BroflakeEngine.ctx canceled
across same-instance restarts. Update start() to recreate the context and cancel
function before starting the worker tables and dependent components, preserving
the stop() cancellation flow so stop → ready → start attaches bus.Start, both
routers, and DownstreamUIHandler to a live context.

---

Nitpick comments:
In `@clientcore/producer.go`:
- Around line 148-155: Update State 4’s select loop to handle ctx.Done()
immediately, close the carried peerConnection, and return the empty input state
so teardown does not wait for NATFailTimeout. Preserve the existing
connectionEstablished and timeout behavior.

In `@clientcore/ui.go`:
- Around line 78-97: Update tickMs in DownstreamUIHandler to represent the
ticker interval as a proper time.Duration once, avoiding multiplication of a
duration by time.Millisecond; rename it to remove the misleading unit suffix and
pass it directly to time.NewTicker.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 77eefbdf-40be-4e2b-b2e7-822b158277c1

📥 Commits

Reviewing files that changed from the base of the PR and between a36cb2c and 2c9b6f0.

📒 Files selected for processing (7)

• clientcore/broflake.go
• clientcore/ipc.go
• clientcore/producer.go
• clientcore/protocol.go
• clientcore/routing.go
• clientcore/ui.go
• clientcore/user.go

[garmr-ulfr]

Addressing coderabbitai's review:

• producer.go State 4 — Fixed. Added a <-ctx.Done() case that closes the peer connection and returns, so teardown no longer waits out NATFailTimeout.
• ui.go duration typing — Fixed. The ticker interval is now time.Second / uiRefreshHz (no Duration × Millisecond).
• broflake.go start/stop restart ctx (Major) — Confirmed accurate. It only affects a same-instance stop → start restart, which the current radiance consumer doesn't exercise (a fresh engine is built per connection). A fix means re-wiring the bus/routers in start() (they're constructed in NewBroflake, not start()), which I'd rather do as a separate lifecycle PR than fold into this teardown-leak fix.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.