fix(camera): report the nominations the stall report could not see - #225
Merged
Conversation
Sourcing the report's nominated set from _bridge_uc_info alone was wrong on exactly the shape that stalls. On a late answer - measured at +1.3 s, so it misses the pre-launch snapshot - it is _nominate_from_answer_sdp inside the first-media wait that nominates, and that function neither appends to _bridge_uc_info["cands"] nor flips ["sent"]. The line would have printed "nominated=none; use-candidate=not-sent" on an open that did nominate, and collapsed the "answer carried no ICE credentials" row into the "answer carried one unroutable candidate" row - two different diagnoses, and the whole reason the run exists. Record what that path nominated and union it with the setup / trickle / late-answer set and the bridge's peer-reflexive learning, deduped and order-preserving. Any one source alone under-reports on some shape. Also make the dropped-probe count distinct sources rather than packets: one unrecorded source probing fifty times is one source, and a per-packet counter would have put a wrong number in the deliverable line. The gating test now anchors on the dedented `if` rather than the bare substring - the `while` condition above contains the same text, so the old form would have passed with the guard deleted.
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
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.
Follow-up to #224, which merged while this was being checked. It is a
correctness fix to the report's data source, not a new feature: #224 as
merged under-reports nomination on precisely the shape the run is meant to
diagnose.
What is wrong in #224 as merged
The report sourced
nominated=anduse-candidate=from_bridge_uc_infoalone. That dict is fed by the setup parse, the trickle consumer, the
late-answer parses and the bridge's peer-reflexive learning - but NOT by
_nominate_from_answer_sdp, which is the function that actually nominates ona late answer. It parses the peeked answer, installs permissions, sends
USE-CANDIDATE to every candidate and returns a count; it never touches
_bridge_uc_info["cands"]and never flips["sent"].A late answer is exactly the measured failure shape - item 3 records the
answer landing at +1.3 s, missing the
_pre_launch_answer_sdpsnapshot. Soon that shape the line would have said:
on an open that nominated one candidate and probed it. Two consequences, both
fatal to the experiment:
indistinguishable from its "answer carried a single unroutable candidate"
row. Those are different diagnoses and telling them apart is half of what
the run is for.
Same class of error as the
_br_stun_resp_countone already called out in#224: a field that reads plausible and answers a different question.
The fix
_nominate_from_answer_sdprecords what it nominated into_nominated_seen,and the report takes the deduped, order-preserving union of that, the
_bridge_uc_infoset and the peer-reflexive set. Any one source aloneunder-reports on some shape.
use-candidateis now true if either path sentone.
Two smaller ones in the same commit
The dropped-probe count was packets, not sources.
_br_probe_overflowincremented per packet once the cap was hit, so one unrecorded source probing
fifty times rendered as
(+50 more source(s))- a wrong number in thedeliverable line that someone would reason from. It is a set of distinct
source labels now.
The gating test was weaker than its name. It searched for the bare
substring
_first_video_pt[0] is None, which thewhilecondition above theguard also contains, so it would have passed with the
ifdeleted. Itanchors on the dedented
ifnow. The guard itself was correct; only the testwas.
Still instrumentation only, still one WARNING on the stall path, still
nothing new above DEBUG on a healthy open. Two new source-level tests, 25 in
the file. Full unit tier green (1082 passed, 5 skipped);
ruff check aidot_cameras/ tests/clean.