Merge Brandon McGinty's latest Barnard hardening

Merge the rewritten contributor history through ecf0027, including bounded queues and buffers, safer packet handling, audio resource cleanup, regression coverage, and the memory watcher.

Co-authored-by: Brandon McGinty <git@bmcginty.us>

barnard.go

@@ -70,6 +70,10 @@ type Barnard struct {
 	// Added for file playback
 	FileStream      *fileplayback.Player
 	FileStreamMutex sync.Mutex
+	// stereoEncoder is reused across connections. Each one holds a little
+	// under a megabyte of encoder state, so building a fresh one per
+	// reconnect is pure churn; it is reset when file playback ends.
+	stereoEncoder gumble.AudioEncoder
 
 	// Added for tone test mode (bypasses all soundcard/OpenAL)
 	ToneTest            bool
@@ -91,6 +95,8 @@ type Barnard struct {
 
 	reconnectStop     chan struct{}
 	reconnectStopOnce sync.Once
+	reconnectMutex    sync.Mutex
+	reconnecting      bool
 }
 
 // cleanupConnectionAudio releases connection-owned audio resources before a
@@ -114,11 +120,20 @@ func (b *Barnard) cleanupConnectionAudio() {
 	b.connectionMutex.Unlock()
 }
 
-func (b *Barnard) cleanupToneTestAudio() {
+// detachToneTestAudio unsubscribes the saver without closing its output, so a
+// reconnect can re-attach the same file. The saver's output is opened
+// exclusively and cannot be reopened.
+func (b *Barnard) detachToneTestAudio() {
 	if b.toneTestSaverDetach != nil {
 		b.toneTestSaverDetach.Detach()
 		b.toneTestSaverDetach = nil
 	}
+}
+
+// cleanupToneTestAudio detaches the saver and closes its output. Use it when
+// the client is shutting down, not between connections.
+func (b *Barnard) cleanupToneTestAudio() {
+	b.detachToneTestAudio()
 	if b.toneTestSaver != nil {
 		b.toneTestSaver.Stop()
 		b.toneTestSaver = nil

barnard.toml

@@ -1,36 +0,0 @@
-AudioDriver = 'pipewire'
-MicVolume = 1.0
-InputDevice = ''
-OutputDevice = ''
-DefaultServer = 'mumble.the-brannons.com:64738'
-Username = 'bmc-beta'
-NotifyCommand = '/usr/share/barnard/barnard-sound.sh "%event" "%who" "%what"'
-NoiseSuppressionEnabled = false
-Certificate = ''
-RecordingFormat = 'flac'
-RecordingDirectory = '~/Audio'
-
-[Hotkeys]
-Talk = 'f8'
-VolumeDown = 'f5'
-VolumeUp = 'f6'
-VolumeReset = 'f7'
-MuteToggle = 'f4'
-RecordToggle = 'ctrl_r'
-Exit = 'f10'
-ToggleTimestamps = 'f3'
-SwitchViews = 'tab'
-ScrollUp = 'pgup'
-ScrollDown = 'pgdn'
-AdminMenu = 'f11'
-NoiseSuppressionToggle = 'f9'
-
-[[Servers]]
-Host = 'mumble.the-brannons.com'
-Port = 64738
-
-[[Servers.Users]]
-Username = 'bmc-beta'
-Boost = 1
-Volume = 1.0
-LocallyMuted = false

client.go

@@ -61,15 +61,24 @@ func (b *Barnard) connect(reconnect bool) bool {
 		// --- Tone test mode: skip all OpenAL; generate 440 Hz tone
 		// --- and save incoming audio to a file.
 
-		// Open the output first. Starting transmission before this succeeds
-		// leaves an orphaned tone goroutine when the path is unusable.
-		saver, err := NewAudioFileSaver(b.ToneTestOutput)
-		if err != nil {
-			b.exitWithError(err)
-			return false
+		// The output is reserved exclusively, so it can only be opened once.
+		// A reconnect keeps writing to the saver opened for the first
+		// connection instead of failing on the file that already exists.
+		if b.toneTestSaver == nil {
+			// Open the output first. Starting transmission before this
+			// succeeds leaves an orphaned tone goroutine when the path is
+			// unusable.
+			saver, err := NewAudioFileSaver(b.ToneTestOutput)
+			if err != nil {
+				b.exitWithError(err)
+				return false
+			}
+			b.toneTestSaver = saver
 		}
-		b.toneTestSaver = saver
-		b.toneTestSaverDetach = b.Client.Config.AttachAudio(saver)
+		// Detach any registration left over from the previous connection so
+		// the shared audio listener list does not grow once per reconnect.
+		b.detachToneTestAudio()
+		b.toneTestSaverDetach = b.Client.Config.AttachAudio(b.toneTestSaver)
 
 		b.setConnected(true)
 		if b.toneTestAutoTransmit() {
@@ -99,11 +108,16 @@ func (b *Barnard) connect(reconnect bool) bool {
 		}
 	})
 
-	// Initialize stereo encoder for file playback
-	b.Client.SetStereoEncoder(opus.NewStereoEncoder())
+	// Initialize stereo encoder for file playback, reusing the one built for
+	// the previous connection rather than allocating another.
+	if b.stereoEncoder == nil {
+		b.stereoEncoder = opus.NewStereoEncoder()
+	}
+	b.Client.SetStereoEncoder(b.stereoEncoder)
 
 	// Initialize file player
 	b.FileStreamMutex.Lock()
+	previousFile := b.FileStream
 	b.FileStream = fileplayback.New(b.Client)
 	b.FileStream.SetErrorFunc(func(err error) {
 		// Disable stereo when file finishes or errors
@@ -113,9 +127,25 @@ func (b *Barnard) connect(reconnect bool) bool {
 	stream.SetFilePlayer(b.FileStream)
 	b.FileStreamMutex.Unlock()
 	b.connectionMutex.Lock()
+	previousStream := b.Stream
 	b.Stream = stream
 	b.connectionMutex.Unlock()
 
+	// A disconnect that lands while the OpenAL devices are opening starts a
+	// second reconnect, so two connects can race to install a stream. The one
+	// that loses must be released here: an orphaned stream keeps its OpenAL
+	// device, its render thread and — because only Destroy detaches it — its
+	// entry in the shared audio listener list, so every later audio packet
+	// from every user is dispatched to it as well, for the life of the
+	// process. Release outside the locks, since Destroy waits on the
+	// per-user audio goroutines.
+	if previousFile != nil {
+		_ = previousFile.Stop()
+	}
+	if previousStream != nil {
+		previousStream.Destroy()
+	}
+
 	b.setConnected(true)
 	// Dial delivers OnConnect before connect creates the OpenAL stream, so
 	// start auto-transmit here as well for initial connections and reconnects.
@@ -208,7 +238,9 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) {
 			close(b.toneTestStop)
 			b.toneTestStop = nil
 		}
-		b.cleanupToneTestAudio()
+		// Keep the saver's output open: it was reserved exclusively and a
+		// reconnect re-attaches the same file. It is closed on exit.
+		b.detachToneTestAudio()
 	}
 
 	b.Notify("disconnect", "me", reason)
@@ -223,7 +255,27 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) {
 		b.UiTree.Rebuild()
 		b.Ui.Refresh()
 	})
-	go b.reconnectGoroutine()
+	b.startReconnect()
+}
+
+// startReconnect launches the reconnect loop unless one is already running.
+// Disconnect notifications can arrive more than once for a connection, and
+// every extra loop is another connect racing to install its own audio stream.
+func (b *Barnard) startReconnect() {
+	b.reconnectMutex.Lock()
+	defer b.reconnectMutex.Unlock()
+	if b.reconnecting {
+		return
+	}
+	b.reconnecting = true
+	go func() {
+		defer func() {
+			b.reconnectMutex.Lock()
+			b.reconnecting = false
+			b.reconnectMutex.Unlock()
+		}()
+		b.reconnectGoroutine()
+	}()
 }
 
 func (b *Barnard) reconnectGoroutine() {

extras/barnard-memwatch.sh

@@ -0,0 +1,80 @@
+#!/bin/bash
+# barnard-memwatch.sh
+# Description: Records barnard's memory use alongside its Go heap size, and
+# captures profiles before the kernel's OOM killer can take the evidence away.
+#
+# Start barnard with -profile, then run this alongside it. The ratio between
+# the two numbers is the diagnosis:
+#
+#   RSS large, go_heap small -> the growth is in cgo memory (OpenAL, opus,
+#                               rnnoise). The Go garbage collector cannot see
+#                               it and so applies no back pressure at all.
+#   RSS large, go_heap large -> the growth is Go-side, and the heap profile
+#                               this script dumps names what is holding it.
+#
+# Usage: barnard-memwatch.sh [output-log]
+#
+# Copyright 2026, Storm Dragon, <storm_dragon@linux-a11y.org>
+#
+# This is free software; you can redistribute it and/or modify it under the
+# terms of the GNU General Public License as published by the Free
+# Software Foundation; either version 3, or (at your option) any later
+# version.
+#
+# This software is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+
+set -u
+
+# barnard serves its profiles here when started with -profile.
+profile_host="localhost:6060"
+# Dump profiles once resident memory passes this many kilobytes.
+dump_threshold_kb=2000000
+sample_interval=30
+
+output="${1:-$HOME/barnard-memwatch.log}"
+
+pid="$(pgrep -n -x barnard)" || {
+    echo "barnard is not running" >&2
+    exit 1
+}
+
+if ! curl -s -m 2 "http://${profile_host}/debug/pprof/" > /dev/null; then
+    echo "no profile server on ${profile_host}; start barnard with -profile" >&2
+    exit 1
+fi
+
+echo "watching barnard (pid ${pid}), writing to ${output}"
+
+dumped=0
+while kill -0 "$pid" 2> /dev/null; do
+    rss="$(awk '/^VmRSS/{print $2}' "/proc/${pid}/status" 2> /dev/null)"
+    swap="$(awk '/^VmSwap/{print $2}' "/proc/${pid}/status" 2> /dev/null)"
+    heap="$(curl -s -m 2 "http://${profile_host}/debug/pprof/heap?debug=1" \
+        | awk '/^# HeapInuse/{print $4}')"
+    goroutines="$(curl -s -m 2 "http://${profile_host}/debug/pprof/goroutine?debug=1" \
+        | head -1 | grep -o '[0-9]*')"
+    threads="$(ls "/proc/${pid}/task" 2> /dev/null | wc -l)"
+
+    printf '%s rss=%skB swap=%skB go_heap=%sB goroutines=%s threads=%s\n' \
+        "$(date +%T)" "${rss:-?}" "${swap:-0}" "${heap:-?}" \
+        "${goroutines:-?}" "${threads}" >> "$output"
+
+    # Capture the evidence once, while the process is still alive to ask.
+    if [[ ${dumped} -eq 0 && ${rss:-0} -gt ${dump_threshold_kb} ]]; then
+        dumped=1
+        curl -s -m 10 -o "${output}.heap" \
+            "http://${profile_host}/debug/pprof/heap"
+        curl -s -m 10 -o "${output}.goroutine" \
+            "http://${profile_host}/debug/pprof/goroutine?debug=2"
+        cp "/proc/${pid}/smaps_rollup" "${output}.smaps" 2> /dev/null
+        printf '%s *** dumped profiles at rss=%skB ***\n' \
+            "$(date +%T)" "${rss}" >> "$output"
+    fi
+
+    sleep "${sample_interval}"
+done
+
+echo "barnard exited; log is in ${output}"

fix.txt

@@ -1,308 +0,0 @@
-Barnard audit findings
-======================
-
-This list was assembled from static review of the maintained Go sources,
-including Barnard, gumble, gumbleopenal, the local OpenAL binding, recording,
-file playback, UI, and protocol code. Generated protobuf output and the
-vendored Mumble C++ source were not treated as files to modify.
-
-Priority 0: security and crashers
----------------------------------
-
-[x] 1. UDP crypto secrets are logged
-   Files: gumble/gumble/udp15.go, gumble/gumble/crypt.go
-   Both crypto setup paths log the AES key and IV/nonce material at debug
-   level. Anyone who obtains debug logs and a packet capture can decrypt
-   voice traffic. Remove all secret material from logs. At most log lengths,
-   setup success, and a non-secret connection identifier.
-
-[x] 2. Native UDP races TCP state mutation
-   Files: gumble/gumble/udp15.go, gumble/gumble/handlers.go,
-          gumble/gumble/audiolisteners.go
-   The UDP reader runs independently of the TCP read routine. It reads
-   Client.Users, User.decoder/audio sequence state, and audio-listener stream
-   maps while TCP handlers add/remove users and close stream channels. This
-   can cause data races, concurrent map read/write panics, or sends to a
-   closed stream channel. Establish one synchronization regime: protect
-   client user/channel state and audio stream registration with locks, and
-   ensure channel close/send are serialized. Do not rely on the TCP read
-   routine being serialized with UDP.
-
-[x] 3. Context actions panic on receipt and on trigger
-   Files: gumble/gumble/client.go, gumble/gumble/handlers.go,
-          gumble/gumble/contextaction.go
-   Client.ContextActions is never initialized, so the first
-   ContextActionModify_Add writes to a nil map. Further, newly created
-   ContextAction values do not receive client = c, so Trigger methods dereference
-   nil. Initialize the map in DialWithDialer and assign its owning client when
-   actions are created. Add handler tests for add/remove/trigger.
-
-[x] 4. Unknown ChannelId deadlocks the protocol reader
-   File: gumble/gumble/handlers.go
-   In handleUserState, the unknown ChannelId branch takes c.volatile.Lock()
-   again instead of unlocking before returning. This leaves the mutex locked
-   forever. Replace with one unlock (prefer defer after acquisition) and add
-   a malformed/out-of-order channel test.
-
-[x] 5. OpenAL Buffer.Delete deletes a source, not a buffer
-   File: gumble/go-openal/openal/buffer.go
-   Buffer.Delete calls C.walDeleteSource. It must call C.walDeleteBuffer.
-   The current code reports invalid source errors and leaks OpenAL buffers.
-   Add a binding test that creates and deletes a single buffer and checks
-   openal.Err().
-
-[x] 6. UI writes are concurrent and termbox is not protected
-   Files: ui.go, client.go, gumble/gumbleopenal/stream.go, uiterm/*.go
-   Network callbacks, audio capture error callbacks, and reconnect goroutines
-   directly update Ui, Textview, Tree, Label, and termbox while Ui.Run updates
-   the same state. These types have no locks and termbox calls are not safe
-   from arbitrary goroutines. Route UI work through a UI-owned event queue, or
-   protect all state and ensure only the UI goroutine calls termbox.
-
-[x] 7. Terminal control sequences from server data are rendered
-   Files: client.go, ui.go, ui_tree.go, admin.go
-   HTML escaping does not remove terminal escape/control sequences. Server
-   supplied messages, names, comments, and channel names are displayed in the
-   terminal and can contain ANSI/OSC controls. Sanitize for terminal display:
-   remove/control-escape C0, DEL, ESC, and dangerous Unicode controls before
-   rendering or notifying.
-
-Priority 1: transport, lifecycle, and correctness
----------------------------------------------------
-
-[x] 8. TCP audio is discarded before UDP is proven usable
-   Files: gumble/gumble/crypt.go, gumble/gumble/client.go, gumble/gumble/udp.go
-   udpActive is set immediately after CryptSetup. The TCP read routine then
-   discards UDPTunnel packets even if inbound UDP is blocked or NAT setup has
-   failed. Mark UDP active only after an authenticated UDP response/audio
-   packet (or retain TCP until confirmed), and define fallback/recovery rules.
-
-[x] 9. Stream capture shutdown/startup races OpenAL
-   File: gumble/gumbleopenal/stream.go
-   StopSource closes a channel but does not wait for sourceRoutine. Destroy
-   immediately closes the capture device, so the routine can access a closed
-   device. A quick stop/start can also run two capture routines at once. Use
-   a cancellation context plus WaitGroup/done channel; serialize Start, Stop,
-   reopen, and Destroy; wait before CaptureCloseDevice.
-
-[x] 10. Renderer can be used after it is closed
-    Files: gumble/gumbleopenal/stream.go, gumble/gumble/audiolisteners.go
-    Existing OnAudioStream goroutines can run cleanup after Destroy closes
-    renderCh. Their final render call then panics sending on a closed channel.
-    Stop and join all audio stream goroutines before renderer shutdown; make
-    render reject work after shutdown without panicking.
-
-[x] 11. Reconnect leaks the old audio stream
-    File: client.go
-    OnDisconnect starts reconnecting but never destroys the existing Stream or
-    stops its file player/capture routine. connect creates a new Stream and
-    overwrites b.Stream. Destroy/stop the old resources before reconnecting;
-    make disconnect cleanup idempotent.
-
-[x] 12. File player sessions race each other
-    File: fileplayback/player.go
-    readFileAudio repeatedly reads mutable Player stopChan/ctx/audioChan.
-    Stop can return and a new PlayFile can replace them while the old ffmpeg
-    goroutine is still running. Old audio can enter the new session and old
-    workers can survive. Put per-playback state in a session object with local
-    context, stop channel, output channel, and WaitGroup. Stop must cancel and
-    join that session before another begins.
-
-[x] 13. Recorder Stop races the encoder writer
-    File: recording/recorder.go
-    Stop closes stdin while run may be writing. A normal stop can therefore
-    record a closed-pipe error and be reported as failed. Have run own stdin
-    closure: signal stop, wait for run to finish/close stdin and Wait ffmpeg,
-    then return its result. Do not close stdin concurrently from Stop.
-
-[x] 14. Tone-test startup leaks transmission on output-file error
-    File: client.go
-    connect starts StartToneGenerator and sets Tx before NewAudioFileSaver. If
-    output file creation fails, the tone goroutine continues. Create the saver
-    first, or close/wait for the tone generator and reset Tx on every failure.
-
-[x] 15. Gumble ffmpeg Pause can block forever
-    File: gumble/gumbleffmpeg/stream.go
-    Pause checks StatePlaying, releases the lock, then sends on an unbuffered
-    pause channel. If process exits in between, no receiver remains. Redesign
-    around context cancellation/state guarded by a mutex and a per-run done
-    channel. Also synchronize Volume, which is currently read and written
-    without protection.
-
-[x] 16. Audio listener/event listener detach is not concurrency-safe
-    Files: gumble/gumble/listeners.go, gumble/gumble/audiolisteners.go
-    Event listener detach has no lock; audio detach removes streams without
-    closing/joining them. Concurrent attach/detach/delivery can corrupt linked
-    lists or strand goroutines. Use mutex-protected listener snapshots and an
-    idempotent detach operation.
-
-[x] 17. Notification commands block callers and substitution is unsafe
-    File: main.go
-    Notify sends to an unbuffered channel. The one consumer waits for each
-    shell command, so slow notification programs block UI/network callbacks.
-    Use a bounded queue and define dropping/backpressure behavior. Placeholder
-    replacement is sequential: a user-controlled value containing a later
-    placeholder can be re-expanded inside prior substituted text. Build argv
-    without a shell where possible, or perform non-recursive token expansion
-    in one pass.
-
-Priority 2: protocol and data correctness
-------------------------------------------
-
-[x] 18. UserStats FromServer fields are copied from FromClient
-    File: gumble/gumble/handlers.go
-    In handleUserStats, FromServer.Good is correct but Late/Lost/Resync read
-    packet.FromClient. Use packet.FromServer for all four fields and add a
-    regression test with differing values.
-
-[x] 19. Full channel link updates leave stale reverse links
-    File: gumble/gumble/handlers.go
-    A ChannelState Links replacement assigns a new channel.Links map but does
-    not remove channel from the Links maps of old peers. Remove reciprocal old
-    links before replacement and add link add/remove/full-replacement tests.
-
-[x] 20. Malformed protobuf fields can panic handlers
-    File: gumble/gumble/handlers.go
-    Several optional proto fields are dereferenced without validation, notably
-    ACL group.Name and UserList_User.UserId. Validate required fields before
-    dereferencing and return errInvalidProtobuf for malformed server packets.
-    Audit all packet pointer dereferences similarly.
-
-[x] 21. UDP protocol state has no complete interoperability test coverage
-    Files: gumble/gumble/udp15.go, gumble/gumble/udp.go
-    Tests are mostly local encrypt/decrypt round trips. Add captured/reference
-    vectors from current Mumble for CryptSetup, encrypted audio, ping, packet
-    loss, IV wrap, late/replayed packets, protobuf and legacy envelopes, frame
-    terminators, positional data, and volume adjustment. Test real UDP
-    fallback behavior too.
-
-[x] 22. Opus bitrate calculation assumes a 10 ms interval
-    File: gumble/opus/opus.go
-    bitrate is maxDataBytes * 8 * 100. For permitted 20/40/60 ms intervals it
-    is 2x/4x/6x too high. Calculate bits per frame divided by the actual
-    Config.AudioInterval, or set the bitrate once when configuration changes.
-
-[x] 23. AudioInterval accepts invalid values
-    File: gumble/gumble/config.go
-    AudioFrameSize truncates arbitrary intervals to a count of 10 ms frames,
-    while the ticker still uses the original interval. Validate and reject
-    values other than 10/20/40/60 ms (and validate AudioDataBytes/Buffers).
-
-[x] 24. Legacy/custom varint has a MinInt64 recursion failure
-    File: gumble/gumble/varint/write.go
-    Encoding math.MinInt64 evaluates -value to the same negative number and
-    recursively encodes forever until panic. Handle MinInt64 explicitly or
-    encode negatives using an unsigned magnitude without overflow. Validate
-    output buffer capacity in the exported encoder too.
-
-[x] 25. Mumble version layout documentation is wrong
-    File: gumble/gumble/version.go
-    The comment says major uses bits 0-15, but SemanticVersion and ClientVersion
-    use bits 16-31. Correct the documentation and add known version tests.
-
-Priority 3: configuration, UI, and binding hardening
-------------------------------------------------------
-
-[x] 26. Persisted microphone volume is unused and zero is impossible
-    Files: config/user_config.go, ui.go, gumble/gumbleopenal/stream.go
-    MicVolume is stored but never applied when a Stream is created; UI changes
-    do not call SaveConfig. GetMicVolume treats stored zero bits as
-    uninitialized and returns 1.0, so mute cannot persist. Initialize the
-    atomic value to 1.0 in New, apply configured volume during connect, allow
-    zero, and save volume changes.
-
-[x] 27. Config address and file error handling can panic
-    File: config/user_config.go
-    makeHostPort splits on ':' and panics for malformed addresses or IPv6.
-    fileExists dereferences info after non-ENOENT Stat failures. Replace with
-    net.SplitHostPort (with explicit default-port policy) and return/report
-    errors from Stat rather than dereferencing nil.
-
-[x] 28. Config SaveConfig panics and is not robust
-    File: config/user_config.go
-    Configuration write/rename errors panic the client. Return errors to the
-    caller, preserve the prior config on failure, and consider fsyncing the
-    temporary file/directory before rename. Avoid broad unrelated formatting
-    changes while fixing this.
-
-[x] 29. FIFO reader spins after an error
-    File: main.go
-    setup_fifo ignores all ReadBytes errors and retries immediately. It also
-    never closes the FIFO descriptor. Exit the reader on terminal errors,
-    close the descriptor, and make shutdown cancellable.
-
-[x] 30. Empty UI tree can panic; UI startup errors are swallowed
-    Files: uiterm/tree.go, uiterm/ui.go
-    Tree.uiKeyEvent indexes lines[activeLine] for a non-arrow key even when
-    no lines exist. Guard empty trees. Ui.Run returns nil when termbox.Init
-    fails, hiding startup failure. Return that error. Also stop/join the
-    PollEvent goroutine on UI shutdown and make Close nonblocking/idempotent.
-
-[x] 31. Text UI is not Unicode-safe and timestamp parsing is fragile
-    Files: uiterm/textbox.go, uiterm/textview.go
-    Textbox cursor positions are byte offsets but editing/display iterates
-    runes, so non-ASCII input can be split into invalid UTF-8. Textview assumes
-    every line contains ']' when timestamps are hidden and can panic otherwise.
-    Track rune boundaries and use safe timestamp parsing/fallbacks.
-
-[x] 32. OpenAL binding needs API and unsafe hardening
-    Files: gumble/go-openal/openal/*.go
-    Many public slice APIs unconditionally use &slice[0] and panic for empty
-    input (NewBuffers(0), Delete empty lists, SetData empty data, GetIntegerv
-    size zero, etc.). Add guards or documented errors. go vet reports unsafe
-    pointer misuse in alcCore.go; replace stored uintptr C handles with a
-    vetted representation/pattern and re-run vet. Listener orientation uses a
-    global tempSlice without synchronization; use a local fixed array.
-
-[x] 33. OpenAL errors are mostly ignored
-    Files: gumble/gumbleopenal/stream.go, gumble/go-openal/openal/*.go
-    Source/buffer/context/capture calls generally do not check AL/ALC errors,
-    so invalid device/context/buffer operations become silent audio failure.
-    Add checked wrapper operations for lifecycle-critical calls and surface
-    actionable errors to Barnard.
-
-[x] 34. Beep helpers panic when the external command is absent
-    Files: ui.go, gumble/gumbleopenal/stream.go
-    Both helpers panic on exec failure. Return/log an error or remove unused
-    helpers; a missing optional beep binary must not terminate Barnard.
-
-[x] 35. Admin manual-ban duration accepts negative values
-    Files: admin.go, gumble/gumble/bans.go
-    Negative minutes become a negative duration, then are cast to uint32
-    seconds for the protocol, creating a huge ban duration. Reject negative
-    durations and validate mask/address before sending.
-
-[x] 36. Admin, tree, and client code read mutable maps outside Client.Do
-    Files: admin.go, ui_tree.go, client.go
-    UI code ranges Client.Users, Channels, Channel.Users, and Children while
-    network handlers mutate them. This overlaps issue 2 but must be fixed on
-    the UI side as well: take a safe snapshot under the client lock and render
-    the snapshot outside the lock.
-
-Suggested repair order
-----------------------
-
-1. Remove crypto secret logging; fix ContextActions initialization/client;
-   fix the handler deadlock and OpenAL Buffer.Delete.
-2. Define client/UDP/audio-stream locking and lifecycle ownership, then add
-   race/integration tests for user removal, disconnect, reconnect, and UDP
-   fallback.
-3. Make UI updates single-threaded and sanitize terminal output.
-4. Repair recording/file/capture session ownership and joining.
-5. Fix protocol data errors, validation, Opus interval math, and config input.
-6. Harden the OpenAL binding and remaining UI/config edge cases.
-
-Required verification after fixes
---------------------------------
-
-- gofmt only touched files.
-- go test ./...
-- go test -race ./...
-- go vet ./... with no remaining unsafe-pointer warnings.
-- Add focused unit tests for every deterministic bug above.
-- Add a local Mumble integration test or reproducible harness for TCP-only,
-  UDP success, blocked inbound UDP fallback, reconnect, user removal during
-  UDP audio, and native 1.5 UDP reference packets.
-- Manually test capture stop/start, disconnect/reconnect, file start/stop,
-  recording stop, zero mic volume, Unicode/UI input, and no-beep/no-ffmpeg
-  failure paths.

gumble/gumble/audiolisteners.go

@@ -54,10 +54,12 @@ func (e *AudioListeners) Attach(listener AudioListener) Detacher {
 	if e.head == nil {
 		e.head = item
 	}
-	if e.tail == nil {
-		e.tail = item
-	} else {
+	if e.tail != nil {
 		e.tail.next = item
 	}
+	// tail was previously left pointing at the first item ever attached. Once
+	// anything detached, the next attach linked itself onto a node that was no
+	// longer in the list, so that listener never received audio again.
+	e.tail = item
 	return item
 }

gumble/gumble/audiolisteners_regression_test.go

@@ -0,0 +1,71 @@
+package gumble
+
+import "testing"
+
+type countingAudioListener struct{ streams int }
+
+func (l *countingAudioListener) OnAudioStream(e *AudioStreamEvent) {
+	l.streams++
+	go func() {
+		for range e.C {
+		}
+	}()
+}
+
+// Regression: an audio listener is removed from the shared list only by
+// Detach. A stream that was created but never destroyed therefore stayed
+// subscribed for the life of the process, and every audio packet from every
+// user was dispatched to it as well — one goroutine, one packet queue and one
+// set of playback buffers per orphan, per user. This test pins the fan-out
+// behaviour that makes failing to detach so expensive.
+func TestDispatchAudioFansOutToEveryAttachedListener(t *testing.T) {
+	c := &Client{Config: NewConfig(), Users: make(Users)}
+	user := c.Users.create(1)
+
+	first := &countingAudioListener{}
+	second := &countingAudioListener{}
+	firstLink := c.Config.AttachAudio(first)
+	c.Config.AttachAudio(second)
+
+	c.dispatchAudio(user, &AudioPacket{Client: c, Sender: user})
+	if first.streams != 1 || second.streams != 1 {
+		t.Fatalf("expected both listeners to receive the stream, got %d and %d",
+			first.streams, second.streams)
+	}
+
+	// Detaching must actually stop the fan-out; this is the only thing that
+	// keeps a replaced stream from accumulating.
+	firstLink.Detach()
+	third := &countingAudioListener{}
+	c.Config.AttachAudio(third)
+	other := c.Users.create(2)
+	c.dispatchAudio(other, &AudioPacket{Client: c, Sender: other})
+
+	if first.streams != 1 {
+		t.Fatalf("detached listener still received audio: %d streams", first.streams)
+	}
+	if second.streams != 2 || third.streams != 1 {
+		t.Fatalf("attached listeners missed the second user: %d and %d",
+			second.streams, third.streams)
+	}
+}
+
+// Detach must also release the per-user stream channels it owns, so a
+// destroyed stream does not pin its queued audio.
+func TestDetachClosesPerUserStreams(t *testing.T) {
+	c := &Client{Config: NewConfig(), Users: make(Users)}
+	user := c.Users.create(1)
+
+	listener := &countingAudioListener{}
+	link := c.Config.AttachAudio(listener)
+	c.dispatchAudio(user, &AudioPacket{Client: c, Sender: user})
+
+	item := c.Config.AudioListeners.head
+	if item == nil || len(item.streams) != 1 {
+		t.Fatal("expected one per-user stream before detaching")
+	}
+	link.Detach()
+	if len(item.streams) != 0 {
+		t.Fatalf("Detach left %d per-user streams behind", len(item.streams))
+	}
+}

gumble/gumble/channel_cycle_regression_test.go

@@ -0,0 +1,84 @@
+package gumble
+
+import (
+	"testing"
+
+	"git.stormux.org/storm/barnard/gumble/gumble/MumbleProto"
+	"google.golang.org/protobuf/proto"
+)
+
+// Regression: handleChannelState accepted any parent the server named, so a
+// channel could be made its own ancestor. Everything that walks the resulting
+// Parent/Children graph then recurses until it exhausts memory.
+func TestChannelStateRejectsSelfParent(t *testing.T) {
+	c := &Client{Config: NewConfig(), Channels: make(Channels)}
+	root := c.Channels.create(0)
+	child := c.Channels.create(1)
+	child.Parent = root
+	root.Children[child.ID] = child
+
+	id, parent := child.ID, child.ID
+	data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Parent: &parent})
+	if err := c.handleChannelState(data); err != nil {
+		t.Fatal(err)
+	}
+
+	if child.Parent == child {
+		t.Fatal("channel was made its own parent")
+	}
+	if _, ok := child.Children[child.ID]; ok {
+		t.Fatal("channel was made its own child")
+	}
+	if child.Parent != root {
+		t.Fatal("the rejected move should have left the original parent intact")
+	}
+}
+
+// A channel must not be reparented under one of its own descendants either.
+func TestChannelStateRejectsDescendantParent(t *testing.T) {
+	c := &Client{Config: NewConfig(), Channels: make(Channels)}
+	root := c.Channels.create(0)
+	middle := c.Channels.create(1)
+	leaf := c.Channels.create(2)
+	middle.Parent, root.Children[middle.ID] = root, middle
+	leaf.Parent, middle.Children[leaf.ID] = middle, leaf
+
+	id, parent := middle.ID, leaf.ID
+	data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Parent: &parent})
+	if err := c.handleChannelState(data); err != nil {
+		t.Fatal(err)
+	}
+
+	if middle.Parent != root {
+		t.Fatal("a cyclic reparent was applied instead of ignored")
+	}
+	if isChannelDescendant(middle.Parent, middle) {
+		t.Fatal("channel graph is cyclic")
+	}
+}
+
+// A legitimate move must still be applied.
+func TestChannelStateAllowsNonCyclicMove(t *testing.T) {
+	c := &Client{Config: NewConfig(), Channels: make(Channels)}
+	root := c.Channels.create(0)
+	a := c.Channels.create(1)
+	b := c.Channels.create(2)
+	a.Parent, root.Children[a.ID] = root, a
+	b.Parent, root.Children[b.ID] = root, b
+
+	id, parent := b.ID, a.ID
+	data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Parent: &parent})
+	if err := c.handleChannelState(data); err != nil {
+		t.Fatal(err)
+	}
+
+	if b.Parent != a {
+		t.Fatal("a valid reparent was rejected")
+	}
+	if a.Children[b.ID] != b {
+		t.Fatal("child link missing after a valid reparent")
+	}
+	if _, ok := root.Children[b.ID]; ok {
+		t.Fatal("stale child link left on the old parent")
+	}
+}

gumble/gumble/client.go

@@ -181,7 +181,12 @@ func DialWithDialer(dialer *net.Dialer, config *Config, tlsConfig *tls.Config) (
 
 		state: uint32(StateConnected),
 
-		connect: make(chan *RejectError),
+		// Buffered: once DialWithDialer returns on its synchronization
+		// timeout nothing reads this channel again, and an unbuffered send
+		// from handleReject would block readRoutine forever, leaking the
+		// goroutine along with the client, its user and channel maps and its
+		// multi-megabyte read buffer.
+		connect: make(chan *RejectError, 1),
 		end:     make(chan struct{}),
 	}
 

gumble/gumble/config.go

@@ -38,6 +38,12 @@ type Config struct {
 	Buffers        int
 }
 
+// MaximumBuffers caps Config.Buffers. Each buffer holds up to one maximum
+// sized audio frame and is allocated per speaking user, so a large value
+// multiplied by a populated channel is a substantial amount of memory. A few
+// seconds of buffering is already far more than playback needs.
+const MaximumBuffers = 1024
+
 // NewConfig returns a new Config struct with default values set.
 func NewConfig() *Config {
 	return &Config{
@@ -64,6 +70,12 @@ func (c *Config) Validate() error {
 	if c.Buffers <= 0 {
 		return fmt.Errorf("gumble: Buffers must be positive")
 	}
+	// Buffers is allocated per speaking user, both as a queue of decoded
+	// frames and as OpenAL playback buffers, so an unbounded value multiplies
+	// straight into memory use as a channel fills up.
+	if c.Buffers > MaximumBuffers {
+		return fmt.Errorf("gumble: Buffers must be at most %d", MaximumBuffers)
+	}
 	return nil
 }
 

gumble/gumble/config_buffers_regression_test.go

@@ -0,0 +1,17 @@
+package gumble
+
+import "testing"
+
+// Regression: Buffers had no upper bound, but it is allocated per speaking
+// user both as a decoded-frame queue and as OpenAL playback buffers.
+func TestConfigRejectsOversizedBuffers(t *testing.T) {
+	config := NewConfig()
+	config.Buffers = MaximumBuffers + 1
+	if err := config.Validate(); err == nil {
+		t.Fatal("expected Buffers above the maximum to be rejected")
+	}
+	config.Buffers = MaximumBuffers
+	if err := config.Validate(); err != nil {
+		t.Fatalf("Buffers at the maximum should be accepted: %v", err)
+	}
+}

gumble/gumble/conn.go

@@ -16,6 +16,10 @@ import (
 // DefaultPort is the default port on which Mumble servers listen.
 const DefaultPort = 64738
 
+// retainedPacketBytes is the largest read buffer kept between packets. Bigger
+// buffers are allocated as needed and released again afterwards.
+const retainedPacketBytes = 64 * 1024
+
 // Conn represents a control protocol connection to a Mumble client/server.
 type Conn struct {
 	sync.Mutex
@@ -54,6 +58,11 @@ func (c *Conn) ReadPacket() (uint16, []byte, error) {
 	}
 	if pLengthInt > len(c.buffer) {
 		c.buffer = make([]byte, pLengthInt)
+	} else if len(c.buffer) > retainedPacketBytes && pLengthInt <= retainedPacketBytes {
+		// One oversized packet — a large ACL, user list or channel comment —
+		// used to pin its full size for the life of the connection. Give the
+		// memory back once ordinary traffic resumes.
+		c.buffer = make([]byte, retainedPacketBytes)
 	}
 	if _, err := io.ReadFull(c.Conn, c.buffer[:pLengthInt]); err != nil {
 		return 0, nil, err

gumble/gumble/conn_buffer_regression_test.go

@@ -0,0 +1,43 @@
+package gumble
+
+import (
+	"encoding/binary"
+	"net"
+	"testing"
+)
+
+// Regression: the read buffer grew to the largest packet ever seen and kept
+// that memory for the life of the connection.
+func TestConnBufferShrinksAfterAnOversizedPacket(t *testing.T) {
+	server, client := net.Pipe()
+	defer server.Close()
+	conn := NewConn(client)
+
+	big := 4 * 1024 * 1024
+	go func() {
+		defer server.Close()
+		writePacket := func(length int) {
+			var header [6]byte
+			binary.BigEndian.PutUint16(header[:], 3)
+			binary.BigEndian.PutUint32(header[2:], uint32(length))
+			server.Write(header[:])
+			server.Write(make([]byte, length))
+		}
+		writePacket(big)
+		writePacket(128)
+	}()
+
+	if _, _, err := conn.ReadPacket(); err != nil {
+		t.Fatalf("reading the oversized packet: %v", err)
+	}
+	if len(conn.buffer) < big {
+		t.Fatalf("oversized packet should have grown the buffer, got %d", len(conn.buffer))
+	}
+	if _, _, err := conn.ReadPacket(); err != nil {
+		t.Fatalf("reading the small packet: %v", err)
+	}
+	if len(conn.buffer) > retainedPacketBytes {
+		t.Fatalf("buffer stayed at %d bytes after a small packet, above the %d retained size",
+			len(conn.buffer), retainedPacketBytes)
+	}
+}

gumble/gumble/handlers.go

@@ -184,8 +184,10 @@ func (c *Client) handleUDPTunnel(buffer []byte) error {
 	log.Info("handleUDPTunnel: %s session=%d seq=%d audio_len=%d final=%v buf_remain=%d",
 		user.Name, session, seq, audioLength, isFinal, len(buffer))
 
-	if audioLength > len(buffer) {
-		log.Warn("handleUDPTunnel: audio length %d > remaining buffer %d",
+	// A negative length would pass the upper bound check below and then panic
+	// on the slice expression.
+	if audioLength < 0 || audioLength > len(buffer) {
+		log.Warn("handleUDPTunnel: audio length %d out of range for buffer %d",
 			audioLength, len(buffer))
 		return errInvalidProtobuf
 	}
@@ -447,6 +449,23 @@ func (c *Client) handleChannelRemove(buffer []byte) error {
 	return nil
 }
 
+// maxChannelDepth bounds ancestry walks over a channel tree that may already
+// be cyclic. Real Mumble trees are far shallower than this.
+const maxChannelDepth = 1024
+
+// isChannelDescendant reports whether candidate is channel itself or sits
+// below it in the channel tree. The walk is bounded so an already-cyclic
+// graph cannot hang the caller.
+func isChannelDescendant(candidate, channel *Channel) bool {
+	for i := 0; candidate != nil && i <= maxChannelDepth; i++ {
+		if candidate == channel {
+			return true
+		}
+		candidate = candidate.Parent
+	}
+	return false
+}
+
 func (c *Client) handleChannelState(buffer []byte) error {
 	var packet MumbleProto.ChannelState
 	if err := proto.Unmarshal(buffer, &packet); err != nil {
@@ -473,16 +492,25 @@ func (c *Client) handleChannelState(buffer []byte) error {
 		}
 		event.Channel = channel
 		if packet.Parent != nil {
-			if channel.Parent != nil {
-				delete(channel.Parent.Children, channelID)
-			}
 			newParent := c.Channels[*packet.Parent]
-			if newParent != channel.Parent {
-				event.Type |= ChannelChangeMoved
-			}
-			channel.Parent = newParent
-			if channel.Parent != nil {
-				channel.Parent.Children[channel.ID] = channel
+			// Reparenting a channel under itself or one of its own
+			// descendants makes Parent/Children cyclic, and anything that
+			// walks the tree then recurses until it exhausts memory. Ignore
+			// the move rather than corrupt the channel graph.
+			if isChannelDescendant(newParent, channel) {
+				log.Warn("handleChannelState: ignoring cyclic parent %d for channel %d",
+					*packet.Parent, channelID)
+			} else {
+				if channel.Parent != nil {
+					delete(channel.Parent.Children, channelID)
+				}
+				if newParent != channel.Parent {
+					event.Type |= ChannelChangeMoved
+				}
+				channel.Parent = newParent
+				if channel.Parent != nil {
+					channel.Parent.Children[channel.ID] = channel
+				}
 			}
 		}
 		if packet.Name != nil {

gumble/gumble/reject_regression_test.go

@@ -0,0 +1,56 @@
+package gumble
+
+import (
+	"io"
+	"net"
+	"testing"
+	"time"
+
+	"git.stormux.org/storm/barnard/gumble/gumble/MumbleProto"
+	"google.golang.org/protobuf/proto"
+)
+
+// Regression: the connect channel was unbuffered, so a Reject arriving after
+// DialWithDialer had already returned on its synchronization timeout blocked
+// readRoutine forever, leaking that goroutine and the whole client with it.
+func TestHandleRejectDoesNotBlockWithoutAReceiver(t *testing.T) {
+	c := &Client{
+		Config:  NewConfig(),
+		Users:   make(Users),
+		connect: make(chan *RejectError, 1),
+		state:   uint32(StateConnected),
+	}
+	c.Conn = NewConn(nopConn{})
+
+	reason := "server is full"
+	data, _ := proto.Marshal(&MumbleProto.Reject{Reason: &reason})
+
+	done := make(chan struct{})
+	go func() {
+		_ = c.handleReject(data)
+		close(done)
+	}()
+
+	select {
+	case <-done:
+	case <-timeoutChan():
+		t.Fatal("handleReject blocked with no reader on the connect channel")
+	}
+}
+
+// nopConn is a net.Conn that discards everything, so handleReject's Close call
+// has something to act on.
+type nopConn struct{}
+
+func (nopConn) Read(b []byte) (int, error)         { return 0, io.EOF }
+func (nopConn) Write(b []byte) (int, error)        { return len(b), nil }
+func (nopConn) Close() error                       { return nil }
+func (nopConn) LocalAddr() net.Addr                { return nil }
+func (nopConn) RemoteAddr() net.Addr               { return nil }
+func (nopConn) SetDeadline(t time.Time) error      { return nil }
+func (nopConn) SetReadDeadline(t time.Time) error  { return nil }
+func (nopConn) SetWriteDeadline(t time.Time) error { return nil }
+
+func timeoutChan() <-chan time.Time {
+	return time.After(10 * time.Second)
+}

gumble/gumbleopenal/stream.go

@@ -138,6 +138,10 @@ type Stream struct {
 	recorderMu          sync.RWMutex
 	errorFunc           func(error) // called on capture errors
 	recorder            Recorder
+	// streamWG tracks the per-user goroutines started by OnAudioStream. They
+	// release their OpenAL source and buffers through the renderer, so Destroy
+	// must let them finish before it tears the renderer down.
+	streamWG sync.WaitGroup
 }
 
 func New(client *gumble.Client, inputDevice *string, outputDevice *string, test bool) (*Stream, error) {
@@ -393,13 +397,37 @@ func (s *Stream) getRecorder() Recorder {
 	return s.recorder
 }
 
+// destroyDrainTimeout bounds how long Destroy waits for the per-user audio
+// goroutines to finish draining, so a wedged renderer cannot hang a reconnect.
+const destroyDrainTimeout = 2 * time.Second
+
 func (s *Stream) Destroy() {
 	if s.link != nil {
+		// Detach closes every per-user stream channel, which ends the
+		// goroutines started by OnAudioStream.
 		s.link.Detach()
 	}
+	// Those goroutines delete their OpenAL source and buffers through
+	// s.render, which stops working the moment the renderer is closed below.
+	// Waiting for them here is what keeps the device's sources and buffers
+	// from being orphaned on every reconnect.
+	drained := make(chan struct{})
+	go func() {
+		s.streamWG.Wait()
+		close(drained)
+	}()
+	select {
+	case <-drained:
+	case <-time.After(destroyDrainTimeout):
+		log.Warn("Destroy: timed out waiting for audio stream goroutines; " +
+			"OpenAL sources and buffers may be released only by CloseDevice")
+	}
 	if s.deviceSource != nil {
 		s.StopSource()
-		s.deviceSource.CaptureCloseDevice()
+		if !s.deviceSource.CaptureCloseDevice() {
+			log.Error("Destroy: closing capture device %q failed",
+				deviceName(s.inputDeviceName))
+		}
 		s.deviceSource = nil
 	}
 	if s.deviceSink != nil {
@@ -417,7 +445,14 @@ func (s *Stream) Destroy() {
 			<-s.renderDone
 			s.contextSink = nil
 		}
-		s.deviceSink.CloseDevice()
+		// alcCloseDevice returns ALC_FALSE and frees nothing while the device
+		// still has contexts, buffers or sources outstanding. Dropping the
+		// handle after that silently leaks the device and every buffer it
+		// owns, which is invisible to the Go GC, so at least report it.
+		if !s.deviceSink.CloseDevice() {
+			log.Error("Destroy: closing playback device %q failed; its OpenAL "+
+				"buffers cannot be reclaimed", deviceName(s.outputDeviceName))
+		}
 		s.deviceSink = nil
 	}
 }
@@ -485,7 +520,9 @@ func (s *Stream) SetMicVolume(change float32, relative bool) {
 }
 
 func (s *Stream) OnAudioStream(e *gumble.AudioStreamEvent) {
+	s.streamWG.Add(1)
 	go func(e *gumble.AudioStreamEvent) {
+		defer s.streamWG.Done()
 		log.Info("audio stream started for user %s", e.User.Name)
 		var source openal.Source
 		var emptyBufs openal.Buffers
@@ -576,6 +613,9 @@ func (s *Stream) OnAudioStream(e *gumble.AudioStreamEvent) {
 				return nil
 			}
 			p := jitterBuf[0]
+			// Clear the slot before resliceing: the popped entries stay in
+			// the backing array otherwise, pinning a decoded frame each.
+			jitterBuf[0] = nil
 			jitterBuf = jitterBuf[1:]
 			jitterDuration -= audioPacketDuration(p)
 			// Frame numbers are Mumble timestamps in 10 ms units.
@@ -657,6 +697,7 @@ func (s *Stream) OnAudioStream(e *gumble.AudioStreamEvent) {
 									jitterBuf[0].Sequence, e.User.Name, jitterNextSeq, len(jitterBuf))
 							}
 							jitterDuration -= audioPacketDuration(jitterBuf[0])
+							jitterBuf[0] = nil
 							jitterBuf = jitterBuf[1:]
 							continue
 						}

main.go

@@ -105,6 +105,10 @@ func main() {
 	if err != nil {
 		handle_raw_error(err)
 	}
+	if *buffers <= 0 || *buffers > gumble.MaximumBuffers {
+		handle_raw_error(fmt.Errorf("buffers must be between 1 and %d, got %d",
+			gumble.MaximumBuffers, *buffers))
+	}
 
 	// Set up logging
 	var level barnlog.Level

recording/queue_regression_test.go

@@ -0,0 +1,46 @@
+package recording
+
+import "testing"
+
+// Regression: the per-source mix queues grew without bound. The tick drains a
+// fixed chunk per source, so a stalled encoder leaves a deficit the loop never
+// makes up, and the backlog only ever grew from there.
+func TestRecorderQueueIsCapped(t *testing.T) {
+	t.Parallel()
+
+	var queue []int16
+	frame := make([]int16, 960)
+	// Far more audio than the encoder could have consumed.
+	for i := 0; i < 2000; i++ {
+		queue = appendCapped(queue, frame)
+	}
+
+	if len(queue) > maxQueuedSamples {
+		t.Fatalf("queue grew to %d samples, above the %d cap",
+			len(queue), maxQueuedSamples)
+	}
+}
+
+// Capping must keep the newest audio: dropping the newest would make the
+// recording lag further behind with every overflow.
+func TestRecorderQueueKeepsNewestAudio(t *testing.T) {
+	t.Parallel()
+
+	var queue []int16
+	// Fill past the cap with a marker in the final frame.
+	filler := make([]int16, maxQueuedSamples)
+	queue = appendCapped(queue, filler)
+	newest := []int16{1, 2, 3, 4}
+	queue = appendCapped(queue, newest)
+
+	if len(queue) != maxQueuedSamples {
+		t.Fatalf("expected the queue to sit at the %d cap, got %d",
+			maxQueuedSamples, len(queue))
+	}
+	tail := queue[len(queue)-len(newest):]
+	for i, want := range newest {
+		if tail[i] != want {
+			t.Fatalf("newest audio was dropped: tail %v, want %v", tail, newest)
+		}
+	}
+}

recording/recorder.go

@@ -20,6 +20,12 @@ const (
 	FormatOpus = "opus"
 )
 
+// maxQueuedSamples bounds the per-source mix backlog at roughly five seconds
+// of 48 kHz stereo audio. A source that runs further ahead than this is ahead
+// because the encoder stalled, and no amount of retained audio recovers the
+// timeline; keeping the newest is better than growing without bound.
+const maxQueuedSamples = 5 * gumble.AudioSampleRate * gumble.AudioChannels
+
 type Recorder struct {
 	path      string
 	format    string
@@ -188,6 +194,19 @@ func (r *Recorder) Stop() error {
 	return r.err
 }
 
+// appendCapped adds a source's incoming samples to its mix queue, bounded at
+// maxQueuedSamples. Each tick drains one fixed chunk per source, so a stalled
+// encoder leaves a deficit the loop never makes up and the backlog would
+// otherwise grow for as long as the recording ran. The newest audio is kept:
+// discarding it instead would only push the recording further behind.
+func appendCapped(queue []int16, incoming []int16) []int16 {
+	queue = append(queue, incoming...)
+	if len(queue) > maxQueuedSamples {
+		queue = append(queue[:0], queue[len(queue)-maxQueuedSamples:]...)
+	}
+	return queue
+}
+
 func (r *Recorder) run() {
 	defer close(r.done)
 	ticker := time.NewTicker(r.interval)
@@ -203,7 +222,7 @@ func (r *Recorder) run() {
 			r.closeEncoder()
 			return
 		case item := <-r.input:
-			queues[item.source] = append(queues[item.source], item.samples...)
+			queues[item.source] = appendCapped(queues[item.source], item.samples)
 		case <-ticker.C:
 			clear(chunk)
 			for source, buffer := range queues {

ui.go

@@ -445,15 +445,19 @@ func (b *Barnard) OnMicVolumeUp(ui *uiterm.Ui, key uiterm.Key) {
 }
 
 func (b *Barnard) OnQuitPress(ui *uiterm.Ui, key uiterm.Key) {
-	b.stopReconnects()
-	b.StopRecordingIfActive(true)
-	b.Client.Disconnect()
-	b.Ui.Close()
+	b.shutdown()
 }
 
 func (b *Barnard) CommandExit(ui *uiterm.Ui, cmd string) {
+	b.shutdown()
+}
+
+// shutdown releases everything that outlives a single connection, including
+// the tone test saver's output file, which reconnects deliberately keep open.
+func (b *Barnard) shutdown() {
 	b.stopReconnects()
 	b.StopRecordingIfActive(true)
+	b.cleanupToneTestAudio()
 	b.Client.Disconnect()
 	b.Ui.Close()
 }

uiterm/textview.go

@@ -72,6 +72,48 @@ func (t *Textview) ScrollBottom() {
 	t.uiDraw()
 }
 
+const (
+	// maxScrollbackLines bounds the retained chat history. It used to grow for
+	// the life of the process, and every line added re-wrapped the whole
+	// buffer, so the cost of a session grew as the square of its length. This
+	// is far more history than a reader ever scrolls back through.
+	maxScrollbackLines = 10000
+	// scrollbackTrimChunk is how much history is discarded once the cap is
+	// reached. Trimming a block at a time means the rebuild it forces happens
+	// once every scrollbackTrimChunk lines rather than on every line, which
+	// keeps the amortised cost of an append constant.
+	scrollbackTrimChunk = 1000
+)
+
+// wrapLine renders one stored line as the display lines it occupies.
+func (t *Textview) wrapLine(line string, width int) []string {
+	l := line
+	if !t.showTimestamps {
+		// Server and local messages need not have a timestamp prefix.
+		if _, text, ok := strings.Cut(line, "]"); ok {
+			l = strings.TrimSpace(text)
+		}
+	}
+	var wrapped []string
+	// A Builder keeps this linear; appending a rune at a time to a string
+	// reallocates once per character.
+	var current strings.Builder
+	chars := 0
+	for _, ch := range l {
+		if chars >= width {
+			wrapped = append(wrapped, current.String())
+			current.Reset()
+			chars = 0
+		}
+		current.WriteRune(ch)
+		chars++
+	}
+	if chars > 0 {
+		wrapped = append(wrapped, current.String())
+	}
+	return wrapped
+}
+
 func (t *Textview) updateParsedLines() {
 	width := t.x1 - t.x0
 
@@ -83,33 +125,7 @@ func (t *Textview) updateParsedLines() {
 
 	parsed := make([]string, 0, len(t.Lines))
 	for _, line := range t.Lines {
-		var l = line
-		if !t.showTimestamps {
-			// Server and local messages need not have a timestamp prefix.
-			if _, text, ok := strings.Cut(line, "]"); ok {
-				l = strings.TrimSpace(text)
-			}
-		}
-		current := ""
-		chars := 0
-		reader := strings.NewReader(l)
-		for {
-			if chars >= width {
-				parsed = append(parsed, current)
-				chars = 0
-				current = ""
-			}
-			if reader.Len() <= 0 {
-				if chars > 0 {
-					parsed = append(parsed, current)
-				}
-				break
-			}
-			if ch, _, err := reader.ReadRune(); err == nil {
-				current = current + string(ch)
-				chars++
-			}
-		}
+		parsed = append(parsed, t.wrapLine(line, width)...)
 	}
 	t.parsedLines = parsed
 	t.clampCurrentLine()
@@ -117,7 +133,19 @@ func (t *Textview) updateParsedLines() {
 
 func (t *Textview) AddLine(line string) {
 	t.Lines = append(t.Lines, line)
-	t.updateParsedLines()
+	if len(t.Lines) > maxScrollbackLines {
+		// Trimming invalidates the wrapped buffer and forces a rebuild, so
+		// discard a block rather than a single line; otherwise every append
+		// past the cap would re-wrap the whole buffer.
+		keep := maxScrollbackLines - scrollbackTrimChunk
+		t.Lines = append(t.Lines[:0], t.Lines[len(t.Lines)-keep:]...)
+		t.updateParsedLines()
+	} else if width := t.x1 - t.x0; width > 0 {
+		// Wrap just the new line. Rebuilding every stored line on each append
+		// is what made a long-lived session stall the terminal.
+		t.parsedLines = append(t.parsedLines, t.wrapLine(line, width)...)
+		t.clampCurrentLine()
+	}
 	t.uiDraw()
 }
 

uiterm/textview_regression_test.go

@@ -0,0 +1,116 @@
+package uiterm
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+)
+
+// addLine appends without drawing, so these tests need no terminal.
+func addLineNoDraw(t *Textview, line string) {
+	t.Lines = append(t.Lines, line)
+	if len(t.Lines) > maxScrollbackLines {
+		keep := maxScrollbackLines - scrollbackTrimChunk
+		t.Lines = append(t.Lines[:0], t.Lines[len(t.Lines)-keep:]...)
+		t.updateParsedLines()
+		return
+	}
+	if width := t.x1 - t.x0; width > 0 {
+		t.parsedLines = append(t.parsedLines, t.wrapLine(line, width)...)
+		t.clampCurrentLine()
+	}
+}
+
+// Regression: AddLine used to re-wrap every stored line on each append, which
+// made the cost of a session grow as the square of its length. It now wraps
+// only the new line, so that incremental result must match a full rebuild.
+func TestTextviewIncrementalWrapMatchesFullRebuild(t *testing.T) {
+	t.Parallel()
+
+	lines := []string{
+		"short [12:00:01]",
+		strings.Repeat("a", 200) + " [12:00:02]",
+		"",
+		"exactly-twenty-chars",
+		"unicode ünïcödé line with wide content [12:00:03]",
+	}
+
+	incremental := &Textview{x0: 0, x1: 20, showTimestamps: true}
+	for _, line := range lines {
+		addLineNoDraw(incremental, line)
+	}
+
+	full := &Textview{x0: 0, x1: 20, showTimestamps: true}
+	full.Lines = append([]string(nil), lines...)
+	full.updateParsedLines()
+
+	if len(incremental.parsedLines) != len(full.parsedLines) {
+		t.Fatalf("incremental produced %d wrapped lines, full rebuild %d",
+			len(incremental.parsedLines), len(full.parsedLines))
+	}
+	for i := range full.parsedLines {
+		if incremental.parsedLines[i] != full.parsedLines[i] {
+			t.Fatalf("wrapped line %d differs: incremental %q, full %q",
+				i, incremental.parsedLines[i], full.parsedLines[i])
+		}
+	}
+}
+
+// Regression: the scrollback had no cap, so a long-lived client retained every
+// line it had ever displayed.
+func TestTextviewScrollbackIsCapped(t *testing.T) {
+	t.Parallel()
+
+	view := &Textview{x0: 0, x1: 40, showTimestamps: true}
+	for i := 0; i < maxScrollbackLines+500; i++ {
+		addLineNoDraw(view, fmt.Sprintf("line %d", i))
+	}
+
+	if len(view.Lines) > maxScrollbackLines {
+		t.Fatalf("expected scrollback capped at %d lines, got %d",
+			maxScrollbackLines, len(view.Lines))
+	}
+	if len(view.Lines) < maxScrollbackLines-scrollbackTrimChunk {
+		t.Fatalf("trim discarded more than one chunk: %d lines remain", len(view.Lines))
+	}
+	// The newest line must survive; the oldest must not.
+	if got := view.Lines[len(view.Lines)-1]; got != fmt.Sprintf("line %d", maxScrollbackLines+499) {
+		t.Fatalf("newest line was dropped, got %q", got)
+	}
+	if view.Lines[0] == "line 0" {
+		t.Fatal("oldest line should have been trimmed")
+	}
+	if len(view.parsedLines) != len(view.Lines) {
+		t.Fatalf("wrapped buffer out of sync after trim: %d wrapped, %d stored",
+			len(view.parsedLines), len(view.Lines))
+	}
+}
+
+// wrapLine replaced a loop that concatenated one rune at a time; confirm the
+// wrapping itself is unchanged for the boundary cases.
+func TestTextviewWrapLineBoundaries(t *testing.T) {
+	t.Parallel()
+
+	view := &Textview{showTimestamps: true}
+	for _, tc := range []struct {
+		line  string
+		width int
+		want  []string
+	}{
+		{"", 5, nil},
+		{"abc", 5, []string{"abc"}},
+		{"abcde", 5, []string{"abcde"}},
+		{"abcdef", 5, []string{"abcde", "f"}},
+		{"abcdeabcde", 5, []string{"abcde", "abcde"}},
+	} {
+		got := view.wrapLine(tc.line, tc.width)
+		if len(got) != len(tc.want) {
+			t.Fatalf("wrapLine(%q, %d) = %q, want %q", tc.line, tc.width, got, tc.want)
+		}
+		for i := range tc.want {
+			if got[i] != tc.want[i] {
+				t.Fatalf("wrapLine(%q, %d) = %q, want %q", tc.line, tc.width, got, tc.want)
+			}
+		}
+	}
+}

uiterm/tree.go

@@ -89,10 +89,10 @@ func (t *Tree) rebuild(preserveActive bool, sameItem func(previous, current Tree
 	previousLine := t.activeLine
 	lines := []renderedTreeItem{}
 	for _, item := range t.Generator(nil) {
-		children := t.rebuild_rec(item, 0)
-		if children != nil {
-			lines = append(lines, children...)
+		if len(lines) >= maxTreeLines {
+			break
 		}
+		lines = t.rebuild_rec(lines, item, 0)
 	}
 	t.lines = lines
 	if preserveActive {
@@ -113,21 +113,31 @@ func (t *Tree) rebuild(preserveActive bool, sameItem func(previous, current Tree
 	}
 }
 
-func (t *Tree) rebuild_rec(parent TreeItem, level int) []renderedTreeItem {
-	if parent == nil {
-		return nil
-	}
-	lines := []renderedTreeItem{
-		renderedTreeItem{
-			Level: level,
-			Item:  parent,
-		},
+// A server is free to describe a channel graph in which a channel is its own
+// ancestor. The generator follows parent/child links literally, so without
+// these limits such a graph recurses until the process is out of memory.
+// Real trees are orders of magnitude smaller than either bound.
+const (
+	maxTreeDepth = 64
+	maxTreeLines = 100000
+)
+
+// rebuild_rec appends parent and its descendants to lines. Accumulating into
+// one slice keeps maxTreeLines a budget for the whole tree rather than for
+// each level, and avoids building a slice per node.
+func (t *Tree) rebuild_rec(lines []renderedTreeItem, parent TreeItem, level int) []renderedTreeItem {
+	if parent == nil || level >= maxTreeDepth || len(lines) >= maxTreeLines {
+		return lines
 	}
+	lines = append(lines, renderedTreeItem{
+		Level: level,
+		Item:  parent,
+	})
 	for _, item := range t.Generator(parent) {
-		children := t.rebuild_rec(item, level+1)
-		if children != nil {
-			lines = append(lines, children...)
+		if len(lines) >= maxTreeLines {
+			break
 		}
+		lines = t.rebuild_rec(lines, item, level+1)
 	}
 	return lines
 }

uiterm/tree_regression_test.go

@@ -0,0 +1,65 @@
+package uiterm
+
+import (
+	"testing"
+	"time"
+)
+
+// cyclicItem reports itself as its own child, standing in for a channel graph
+// in which a channel is its own ancestor.
+type cyclicItem struct{ name string }
+
+func (i *cyclicItem) String() string { return i.name }
+
+func (i *cyclicItem) TreeItemStyle(fg, bg Attribute, active bool) (Attribute, Attribute) {
+	return fg, bg
+}
+
+// Regression: rebuild_rec followed parent/child links with no depth limit, so
+// a cyclic channel graph recursed until the process ran out of memory. A
+// rebuild must now terminate and stay bounded.
+func TestTreeRebuildTerminatesOnCyclicGraph(t *testing.T) {
+	t.Parallel()
+
+	self := &cyclicItem{name: "loop"}
+	tree := Tree{
+		Generator: func(item TreeItem) []TreeItem {
+			return []TreeItem{self}
+		},
+	}
+
+	done := make(chan struct{})
+	go func() {
+		tree.rebuild(false, nil)
+		close(done)
+	}()
+
+	select {
+	case <-done:
+	case <-timeoutAfterSeconds(10):
+		t.Fatal("rebuild did not terminate on a cyclic tree")
+	}
+
+	if len(tree.lines) == 0 {
+		t.Fatal("expected the bounded rebuild to still produce lines")
+	}
+	if len(tree.lines) > maxTreeLines {
+		t.Fatalf("rebuild produced %d lines, above the %d cap",
+			len(tree.lines), maxTreeLines)
+	}
+	for _, line := range tree.lines {
+		if line.Level >= maxTreeDepth {
+			t.Fatalf("rebuild recursed to level %d, at or past the %d cap",
+				line.Level, maxTreeDepth)
+		}
+	}
+}
+
+func timeoutAfterSeconds(n int) <-chan struct{} {
+	ch := make(chan struct{})
+	go func() {
+		time.Sleep(time.Duration(n) * time.Second)
+		close(ch)
+	}()
+	return ch
+}