v1.40.34: player resilience, boundary double-clutch fix, loopback health checks - #307
Merged
Merged
Conversation
Three changes to the floating global player. The panel goes from 400px to 480px wide. A 46-character title like "Blue Monday (1988 Remaster) - Extended Club Mix" previously cut off at "Extende" and had to scroll; it now fits on one line. The marquee measures clientWidth at runtime, so it simply scrolls less rather than needing any change. max-width: calc(100vw - 40px) still protects narrow screens. The transport buttons in the dashboard player were centred, because the row was justify-content-between with three children (elapsed time, transport, volume). The row is now a plain flex with ms-auto on the transport group, so play/pause sits to the right next to the volume slider & the time stays left. The live transport now defaults to http instead of webrtc. WebRTC needs a signaling WebSocket, and the websocket path to the edge is where every observed listener drop happens: 8 days of link monitoring shows the prod<->edge tunnel losing 0.10% of packets with p50 34.6ms and no relay flaps, while the app still logs "websocket write failed ... broken pipe" against 192.168.195.1. HTTP sidesteps that path. WebRTC stays fully available from the transport menu. Anyone who has already picked a transport keeps it, because the stored grimnir-live-transport value still wins over the default; only clients with no stored preference change behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #280 fixed one double-clutch and a second one survived it. This is that second one, traced on prod 1.40.30 (which does contain f66dee7). resolveEntryForNow keeps an entry resolvable for 2s past its own EndsAt so a boundary tick can still find it. At a hard boundary that means the entry ending exactly now and its successor both resolve in the same pass. #280's per-tick guard deliberately does not let the ended entry claim the mount's single launch, so its successor can still preempt. Nothing stopped the ended entry reaching handleEntry FIRST and building a complete pipeline that the successor then tore down and replaced. On prod mount d4f41798 that fired at the top of every hour: a recurring "custom" template ending at exactly 00:00:00 launched Behind The Woodshed, and the 00:00 instance rebuilt the same upstream 0.81s later. Two builds, one boundary, audible stutter. Measured over 6h of prod logs by pairing "feed started" with the following EOF per mount: 422 builds, 14 under 10s, 3 under 1s, and the sub-second ones landed at 22:00:00.19, 23:00:00.36 & 00:00:00.33. An entry that has already ended now only launches when nothing still current covers the same mount. The grace still works as dead-air cover; it just stops producing a throwaway build whenever a successor is ready. Two tests. The first seeds an ended entry plus its successor on one mount and asserts a single build, and that the mount is left on the successor rather than the entry that ended; it reports 2 builds without the guard. The second seeds an ended entry alone and asserts it still launches, so the fix cannot turn a boundary with no successor into silence. Note the counting trap for anyone re-checking this on air: "4 feed starts per mount per minute" is NOT the signature. It false-positives on short content, e.g. rlmradioxyz runs a 13-item promo playlist with items like "1.25 sec Hal AI - Promo". Pair feed-start with the following EOF and look at pipeline lifetime instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nect Two halves of the same problem: a brief network wobble ended a listener's stream and nothing brought it back, so an eight-second dip looked exactly like dead air. Server side. A single failed write disconnected a client. That is right for a broken pipe or a connection reset, where the peer is already gone, and wrong for a deadline exceeded, which only means the client stopped draining and may well come back. Failed writes now run against a stall budget of 3x writeTimeout (90s at the default 30s) and every other error still drops on the spot. The budget is elapsed time, not a count of failed writes: a client returning from a dip has a backlog to work through and blows several deadlines in a row while catching up, so counting attempts would disconnect precisely the listener this is meant to keep. Same rule on the keepalive flush path. A timed-out chunk is dropped and counted in skipped, so the listener hears the same discontinuity Broadcast already produces for a slow client rather than losing the stream. Client side. reconnectLiveStream() was written, complete, with backoff and a 20-attempt ceiling, and never called from anywhere. onError returned after a toast, onEnded returned early for live, onStalled and onWaiting only printed "Buffering...", and stalledTimer was cleared in onPlaying but never set. Any drop therefore became a manual page refresh. All four paths now feed it: error and ended reconnect immediately, stalled and waiting arm an 8s timer that reconnects only if playback has not resumed by itself, and close() cancels anything pending so a deliberately closed player cannot resurrect. The hold-open decision is extracted as classifyWriteErr and tested directly rather than through a socket, because a socket-level version of this fights kernel send-buffer autotuning and cannot reliably force the timeout it means to test — two earlier attempts passed with the budget removed, which makes them worse than no test. The table covers a first timeout, a timeout deep inside the budget, a timeout past it, and a broken pipe both cold and during a stall run; all four fail with the budget disabled. The existing half-open-client reap test still passes, so a genuinely dead socket is still collected rather than held forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
52 of the 66 webstreams on prod point at https://rlmradio.xyz/live/<mount>, which is a mount this same box already serves. Playout already handled that: rewriteSelfRelayURL pulls those over 127.0.0.1, and prod logged 14 loopback rewrites against 11 webstream playouts in 6h, so the audio path never leaves the machine. The health checker did not. It probed whatever URL the row carried, so those same 52 relays were checked by their public URL: 28,249 checks in 6 hours, about 78 a minute, every one leaving the box for the CDN edge and coming back over the tunnel to ask whether a local mount is alive. That makes an internal relay's health depend on the least reliable link in the system, and a wobble there marks a healthy mount unhealthy and can trip a failover the source never needed. The 02:00 logs show exactly that shape: "failover threshold reached, grace period started" alongside health-check clients arriving from an external address. Health checks now run through the same loopback resolver. The rewrite logic moves into webstream.LoopbackURL so playout and the checker share one implementation, and the director keeps its extra guard that a mount is never fed from itself. The resolver is injected from server.go, where the broadcast server is already in scope, and a nil resolver leaves every URL untouched. Configured URLs in the database are not modified. The rewrite happens at request time, so a webstream that later stops being local, or an operator reading the row, still sees exactly what was configured. Tests: LoopbackURL over our own mount, a non-default port, the 0-port fallback, and four external-relay shapes that must never be redirected at our own loopback, since that would silently serve one of our streams in place of somebody else's. Two checker-level tests assert the probe hits loopback and not the public URL, and that a checker with no resolver still probes the configured URL; the first fails with the redirect removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
make ci runs the suite with -race and TestService_PlaybackLifecycle failed on a data race that predates this branch (reproduced on main at bd63b09). CI itself runs `go test -count=1 ./...` with no -race, which is why it has been going green. Two goroutines called Wait on the same exec.Cmd. monitorProcess, started by Start, calls it to record the exit code and fire onExit. Stop then called it again from its own goroutine purely to get a 5s graceful-shutdown timeout. os/exec forbids calling Wait more than once, and the two calls raced on the Cmd's internal state; either could also have observed the wrong exit status, so an emergency insert during a crossfade could mis-report why a pipeline died. monitorProcess is now the only caller. It closes a waitDone channel once the process is reaped and its exit state recorded, and Stop selects on that instead of waiting itself, reading exitError under the lock for its log line. The force-kill path is unchanged: on timeout the kill still lands and monitorProcess reaps the process as before. Verified with -count=5 -race on the failing test, the whole mediaengine tree under -race, and a full make ci. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rolls 1.40.34. Five changes, all traced from prod logs on 1.40.30.
Boundary double-clutch (the one that survived #280)
#280 is deployed —
f66dee7is an ancestor ofv1.40.30, and was not inv1.40.24. It fixed its case; a second one remained.resolveEntryForNowkeeps an entry resolvable for 2s past its ownEndsAtso a boundary tick can still find it. At a hard boundary the entry ending now and its successor both resolve in one pass. #280 correctly stopped the ended entry from claiming the mount's launch slot, but nothing stopped it runninghandleEntryfirst and building a whole pipeline the successor then replaced.Prod mount
d4f41798, top of every hour:e343ce12custom983d7b83Builds at
00:00:00.329and00:00:01.138. The 0.81s gap isstartWebstreamEntry's synchronous ICYFetchOncerunning between them.An ended entry now only launches when nothing still current covers the mount. The grace still covers dead air when there is no successor.
Detector note: "4 feed starts per mount per minute" is not the signature — it false-positives on short content (rlmradioxyz runs a 13-item promo playlist with items like "1.25 sec Hal AI - Promo"). Pair
feed startedwith the following EOF and look at pipeline lifetime. 6h baseline: 422 builds, 14 under 10s, 3 under 1s.Listener connections survive stalls
A single failed write disconnected a listener. Correct for a broken pipe or reset; wrong for a deadline exceeded, which only means the client stopped draining. Timeouts now run against a stall budget of
3 × writeTimeout(90s default); every other error still drops immediately.The budget is elapsed time, not a count of failed writes — a client returning from a dip has a backlog to chew through and blows several deadlines in a row while catching up, so counting attempts would drop exactly the listener this protects.
Player auto-reconnect
reconnectLiveStream()was fully written — backoff, 20-attempt ceiling, LQ fallback — and called from nowhere.onErrorreturned after a toast,onEndedreturned early for live,onStalled/onWaitingonly printed "Buffering...", andstalledTimerwas cleared but never set. Every drop became a manual refresh. All four paths now feed it; stalled/waiting arm an 8s timer that fires only if playback hasn't recovered.close()cancels pending timers.Also: transport defaults to HTTP instead of WebRTC, so a new listener starts on LQ over plain HTTP with no signaling websocket. Anyone with a stored
grimnir-live-transportkeeps their choice.Loopback health checks
52 of 66 webstreams point at
https://rlmradio.xyz/live/<mount>— mounts this same box serves. Playout already pulled those over loopback (14 rewrites / 11 playouts in 6h). The health checker did not: 28,249 checks in 6 hours, ~78/min, each leaving the box for the CDN edge and returning over the tunnel to ask whether a local mount is alive. A wobble there marks a healthy mount unhealthy and can trip a failover the source never needed.Checks now use the same resolver. Rewrite logic is shared via
webstream.LoopbackURL; the director keeps its guard that a mount is never fed from itself. Configured URLs in the DB are untouched — the rewrite is request-time only.Pre-existing race fixed
make ciruns with-raceandTestService_PlaybackLifecyclefailed on a race that predates this branch (reproduced onmainatbd63b09). CI runsgo test -count=1 ./...with no-race, which is why it stayed green.monitorProcessandStopboth calledWaiton the sameexec.Cmd.os/execforbids that, and either could observe the wrong exit status.monitorProcessis now the sole caller and closes awaitDonechannel;Stopselects on it. Force-kill path unchanged.Player layout
Panel 400px → 480px, so a 46-char title fits instead of scrolling. Transport buttons move right via
ms-autoinstead of sitting centered between the elapsed time and the volume slider. Verified by rendering the template against the real CSS.Verification
make cipasses with-race(it did not before this branch). Coverage 66.90%.Two tests I threw away rather than keep: socket-level attempts at the hold-open path passed with the budget removed, because kernel send-buffer autotuning meant no write ever timed out. That logic is now
classifyWriteErr, tested directly — all four cases fail with the budget disabled. Same discrimination check run on the double-clutch guard (2 builds without it) and the health-check redirect (public hits = 1, want 0).🤖 Generated with Claude Code