fix(audio): reset quit_ in the pulse ADM's Init() — and fix two build bugs found proving it #113
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "capture/audio-rearm"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Root-causes the audio re-arm defect to an upstream libwebrtc bug, fixes it
with a patch, and fixes two build-system bugs found on the way.
The C++/patch change is UNVERIFIED as a behaviour fix. The lane confirms
it applies and the tree compiles; whether audio survives a re-arm is settled
only by running
tests/interactivetwice against one worker process,which is the run that has never passed. That happens on the new image before
this merges.
The defect
AudioDeviceLinuxPulse::Terminate()setsquit_ = trueand joins the recordand playout threads.
Init()spawns a fresh pair — and never resetsquit_.Both new threads evaluate
if (quit_) return false;on their first wakeup andexit.
Init()reports success with two threads that are already gone.Everything downstream then fails somewhere else:
StartRecording()signals_timeEventRecand waits on_recStartEventfor a thread that does notexist, so it returns -1 in microseconds with the same "failed to activate
recording" text its 10-second timeout path logs. That is why the symptom
read as a timing problem for weeks.
patches/0006adds the three-line reset under the existing mutex.Corrections to the finding's own record
The finding claimed the pre-arm probe reported
StartRecording=0. It reports-1. Two theories are refuted in the doc, one of them mine — the recordthread has EXITED, it is not blocked, and the failure is not a race.
Two build bugs, both found by testing rather than by a green run
src/is not one git repository. DEPS clones sub-repos into it, each withits own
.git;third_party/webrtcis one (DEPS:3012).git am --3wayfromsrc/has no blob for a path the src index does not track and fails withcould not build fake ancestor— which reads as a corrupt patch and is notone.
apply-patchesnow applies each patch from the checkout that owns it,stripping the prefix into a temp copy so the patch stays readable against the
chromium tree it documents. Confirmed in the live lane log:
The sub-repo needs its own reset, on every fire.
apply-patchesresetssrc/to the LKGM base each run, but nothing resetthird_party/webrtc, andthe chromium tree is a hostPath that outlives the Job. So the fire after
the one that first lands 0006 re-applies it onto itself and dies with the
identical two lines from an unrelated cause, leaving
.git/rebase-applybehind to poison the run after that. Measured in a fixture, not predicted.
Now reset by authorship — walk to the first commit not authored by us, which
is exactly the DEPS-pinned revision whatever gclient chose, so there is no
second sha to drift from DEPS.
Wrong turns, recorded in
patches/README.mdindexline and removed it. Wrong —--3wayneeds it. Therefutation was cheap and is why the real cause surfaced next.
git apply --check, which passes on patchesgit amrejects.The check ran; it was not the check that mattered.
git amhad already committedthe change, so the comparison was structurally incapable of failing.
build.shmid-if, sogit amneverran and three green "rounds" proved nothing about applying.
reset v8whileresetting
third_party/webrtc—${sub}outside the loop holds the lastiterated value. Three sites, all now keyed on
strip_prefix.Verification
make verify/make lintUpdate: lane green, image built and deployed
Lane succeeded at
9253939.apply-patchesrouted 0006 to the webrtccheckout and the tree compiled — 671 targets, 8 test targets built and linked:
Image
cr7727-92539396945c, digestsha256:c6603ff6dd65eb5d12f1eae20880a6a70d7c61b389da28dcb1c8a619fd6e76a9— read back from the registry with
crane digest, compared against whatkaniko reported, and checked for well-formedness before pinning (an empty
digest is accepted by
kubectl set imageand lands the worker inInvalidImageName; that cost a deploy earlier in this work). Both pin filesin one commit;
lint-deploy-pinandlint-guest-releaseclean.Tagged at
9253939rather than branch head: the commits since are docs plusthe sub-repo reset, which changes how patches are applied, not what they
apply to — the fixture shows both converge on an identical tree.
A second build bug, found by testing the first fix rather than watching it pass
Four consecutive
apply-patchesrounds against a fixture carrying the realpinned M147 source: round 1 applied, round 2 died with the same two lines
this PR was written to fix, from an unrelated cause, and left
.git/rebase-applybehind to poison round 3.Not a retry-only concern —
apply-patchesruns on every fire and the chromiumtree is a hostPath that outlives the Job, so the fire after the one that
first lands 0006 hits it. Had I only watched this build go green, the next
person to fire the lane would have inherited an error message pointing at a
bug that was already fixed. Now resets the sub-repo by authorship: walk to the
first commit not authored by us, which is the DEPS-pinned revision whatever
gclient chose, so there is no second sha to drift from DEPS.
The harness also caught two bugs in my own fix, neither visible from a green
run: a log line that said
reset v8while resettingthird_party/webrtc(
${sub}outside the loop holds the last iterated value), and a first harnessthat sliced
build.shmid-ifsogit amnever ran — three green "rounds"that proved nothing.
The defect reproduces in ten seconds, with no session
Init()spawns two audio-module threads. The live unpatched worker hadone (pid 23, 9 h uptime) — the
quit_latch, observable directly:Sharper than the suite's audio check, which goes red for a silent ADM and
for any transport fault: 2 threads with no audio is a different bug. The
patched worker reads 2 at boot.
Match the name exactly —
grep -ci audiocountsAudioEncoderQueandAudioDeviceBufftoo and would keep reading 3 after the fix.Corrections to the repo's own record
docs/roadmap-ga.mdprescribed "tear the pulse ADM down on re-arm" as thefix. A teardown-and-reinit is precisely what triggers the bug; following
that instruction would have reproduced the symptom more reliably and sent
the next person hunting in
capture/. Corrected with its reasoning ratherthan deleted — the wrong fix is plausible enough to be proposed again. The
same paragraph was also duplicated verbatim on main.
Terminate(). The pinned source settles it without that answer, and I wrotethe patch without going back to fix the claim.
capture/audio/cb_audio_lifecycle.cc:206had already named this defect— "it sets
quit_and NOTHING ever clears it" — before the symptom wasdiagnosed. The author routed around it rather than patching libwebrtc, which
is why the pre-arm has its unusual stop/re-init/start shape. Now cited in the
finding.
CLAUDE.md: a cancelled CI job (DB status 3) is reported asfailureby thecommit-status API. Two "CI failed" alerts in this work were my own pushes
cancelling in-flight runs; 811 jobs in this repo are cancelled vs 223 truly
failed.
Still outstanding
Nothing — see the verification below.
VERIFIED: audio survives the re-arm
Three consecutive viewer sessions against one browser process (pid 23
before and after each), driving the real client bundle in real Chrome:
Every previous second session received zero audio bytes.
Guest-side ledger for that same process: 6 re-arms, 6 pre-arms reporting
StartRecording=0 now_recording=1, zeroStartRecording=-1, zero "failed toactivate recording" — the string this defect produced on every re-arm since
it was first seen. Both
webrtc_audio_mothreads alive throughout.The first two verification attempts failed for a reason I caused
kubectl apply -f stack.yaml— which I ran to set the image pin — silentlystripped TURN from both halves of the stack:
WEBRTC_ICE_SERVERS(guest fell back tostun:stun.l.google.com:19302), andTURN_URLS/TURN_USER/TURN_PASS, which is what theclient's ICE config comes from.
stack.yamldefines neither;deploy.shsets both after applying it. Soapplying the manifest alone — the natural thing to do to change an image pin
— leaves a stack that connects, negotiates, and shows no video. The client
gathered exactly one
.localmDNS candidate and gave up; the harness reported"the worker already served a session", which it had not.
Restored both from the previous ReplicaSets. Three fixes came out of it:
deploy.shhad a dead hardcodedTURN_IP.95.217.200.179refuses3478 — coturn had moved to triform-7. It sat directly beneath a comment
block explaining how to diagnose exactly that. Now discovers the node and
dials 3478 before using it. Two selector attempts returned a
plausible-but-wrong node first: there are two coturn deployments, and
coturn-2matches every name-prefix pattern forcoturnbecause2is ahex digit. Selecting on the
app=label is exact."NEITHER is a bug in what you are testing". Missing TURN is a third, it is
a deployment fault, and all three are indistinguishable from the client
side. It now leads with the guest's own log, which separates them in one
grep.
tests/interactivenow asserts the audio module has both its threads,so this defect is catchable by the suite instead of only by hand. Verified
the exact command returns
2live, with a negative control (1 forAudioDeviceBuff, 0 for a nonexistent name) proving it is not a constant.Two bugs in my own probe along the way, both of which read as product
failures: it bound to the pre-navigation execution context (destroyed by the
login submit), and it never passed
?e2e=1, without whichwindow.__cbwrtc_pcdoes not exist. Both produced{"pc": null}— "theclient never built a peer connection" — while the console showed a healthy
negotiation.
Found by testing the fix rather than trusting it. Four consecutive apply-patches rounds against a fixture with the real pinned M147 pulse source: round 1 applied, round 2 died with error: sha1 information is lacking or useless (...pulse_linux.cc) error: could not build fake ancestor — the SAME two lines as the wrong-repo bug the previous commit fixed, from a completely unrelated cause, and it left .git/rebase-apply behind to poison round 3. This is not a retry-only concern. apply-patches runs on EVERY lane fire and the chromium tree is a hostPath that outlives the Job, so the fire after the one that first lands 0006 hits it. Had I only watched the current build go green, the next person to fire the lane would have inherited an error message pointing at a bug that was already fixed. The upfront recovery resets ${CHROMIUM_SRC} to the LKGM base and does nothing to third_party/webrtc, whose HEAD keeps whatever the last run committed. Reset it by authorship: walk down to the first commit not authored by us, which is exactly the DEPS-pinned revision whatever gclient chose — no second sha to drift from DEPS. A tree with none of our commits resets to HEAD, a no-op. Failing loudly beats failing confusingly: if every commit looks like ours, apply-patches now dies with a message naming the cause instead of applying a patch onto itself and reaching "fake ancestor" ~30 min later. Two bugs the harness caught in my own code, neither visible from a green run: - The log said "reset v8" while resetting third_party/webrtc. ${sub} outside the for-loop holds the LAST iterated value, not the matched one. Three sites; all now use strip_prefix, set at the match. - My first harness sliced build.sh mid-`if`, so `git am` never ran and three "rounds" proved nothing about applying. The rounds now run the real block, sliced to its closing line and asserted to contain the am. Verified: 4/4 rounds rc=0, converging on one commit and one applied hunk; the loud-failure path fires when the base is authored by us. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>failureby the commit-status API 616890f20egrep -c audiocounts three