Repository navigation
fix(hl2): a sweep no probe left is not silence. Principle VIII. - #6376
Conversation
Answers the non-blocking note on the approving review of aethersdr#6293. Hl2Discovery marked a sweep as sent from the socket count, before the writes. A sweep whose every writeDatagram failed therefore closed as silence, and the third one logged "no Hermes-Lite 2 answered 3 sweeps" although the send-failure warning had already named the cause. DiscoveryNotices now holds the rule: sendSucceeded marks the open sweep, sweepClosed counts it as unanswered only when marked, and clears the mark. One socket sending is enough. A sweep no probe left neither adds to the count nor clears it. Hl2Discovery reports every closed sweep and keeps no flag of its own. hl2_discovery_logging_test covers it socket-free: three all-failed sweeps log each send warning once and no silence line; sent and unanswered sweeps still log it; one socket sent and one failed counts as sent; the mark does not outlive its sweep. Refs aethersdr#6293 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Yes, and the framing is better than the body claims. No fixes keyword, so this closes nothing — it is a follow-up to #6293 (merged as 8a6415621), which covered #6285 item 3.
What makes it more than a polish pass: #6293's own merged body already documented the design as feeding DiscoveryNotices "from sweepNow() (whether a probe left each socket)". The implementation did not do that — m_probeSent = !m_sockets.isEmpty() was set before the writes, so it recorded "a socket existed", not "a probe left". The sendFailed path that exposed the gap arrived late on that PR (5f11af00, the last comment on #6293). This PR makes the code match the intent #6293 shipped with. git show main:src/.../Hl2Discovery.h:137 still carries m_probeSent, confirming the delta.
The chosen seam is right: the rule lives in DiscoveryNotices, which is the socket-free policy object, so it is testable without a network. Per AGENTS.md's test-layer boundary this is "non-events / dropped messages" → socket-free CTest, which is exactly what was added. I could not read the review note on #6293 that the body says it answers (only issue comments are reachable to me), but the commit chain makes the gap self-evident.
2. Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/hl2/Hl2Discovery.cpp |
sendSucceeded sets m_probeLeft; sweepClosed consumes it via std::exchange and gates the count; m_probeSent dropped from stop()/sweepNow(); sweepClosed now called unconditionally |
Yes | In scope |
src/core/backends/hl2/Hl2Discovery.h |
bool m_probeLeft; m_probeSent removed; three comments |
Yes | In scope |
tests/hl2_discovery_logging_test.cpp |
sentSweep() helper, new sweepWithNoProbeSentIsNotSilence(), existing silence/reset checks routed through the helper |
Yes | In scope |
Everything in the diff is explained by the stated fix. No build config, no docs, no CHANGELOG.md, no unrelated files, no formatting churn. No new public surface: m_probeLeft is private, the DiscoveryNotices signature set is unchanged, and no protocol verb, settings key or capability field is touched. No preference change — nothing user-facing moves except one log line being suppressed in a case where a warning already named the cause.
Socket tests: none added, none removed, none modified into network behaviour. tests/hl2_discovery_logging_test.cpp drives DiscoveryNotices only — grep for QUdpSocket / QTcpSocket / bind( / listen( / connectToHost in the test returns nothing. It links Qt6::Network solely because Hl2Discovery.cpp is in its source list. Recorded per policy; not a finding.
3. Blockers
None.
4. Nits (all non-blocking)
-
src/core/backends/hl2/Hl2Discovery.h:27— the comment is now a floor, not a figure:// 3 sweeps at Hl2Discovery's default 5000 ms interval: 15 s of silence. static constexpr int kSilentSweepsBeforeWarning = 3;
Unsent sweeps neither count nor reset (deliberate, and pinned by your
stalecase), so the line can now fire after a count accumulated across an arbitrarily long send outage — 2 silent sweeps, an hour ofNetwork unreachable, then 1 more silent sweep logs "3 sweeps". Reads cleaner as "at least 15 s of silence", since the constant is no longer a wall-clock claim. -
src/core/backends/hl2/Hl2Discovery.cpp:106-108— the interface count in the silence line is bound interfaces, not sending ones. In your ownmixedscenario (one socket sent, one failed) it reads "on 2 interfaces" while only one asked. Thecannot send a probe from …warning is adjacent in the log, so this is marginal — noting it because the new rule makes the two numbers mean different things for the first time. -
src/core/backends/hl2/Hl2Discovery.cpp:291-297(pre-existing, not from this PR) — when the directed broadcast succeeds and the global one fails,sentwins anderroris discarded, so the second failure is never logged. Correct for the new flag ("one reaching the wire is a sent probe"), just worth knowing the error is silently dropped.
5. What I tried to break
- Flag leaking across sweep boundaries. This is the one that would have been a real bug, and it is written correctly:
std::exchangeatHl2Discovery.cpp:100is unconditional and sits before theif (answered)early return. Had it been placed after, an answered sweep would have leftm_probeLeft == truefor the next window, so a following all-failed sweep would count as silence. Your M2 mutant ("read the mark without clearing it") is exactly this, and thestalecase catches it. - Set/consume pairing. Verified 1:1 by reading the call order:
onSweepTimercloses the previous sweep at:345, thensweepNow()at:359sets the mark for the new one.start()at:232probes immediately and the first timer tick consumes it. Grep forsweepNow()acrosssrc/andtests/finds only those two call sites plusAnanDiscovery's unrelated namesake — no external caller can desynchronise the window. stop()→start()with the removed reset. The diff deletesm_probeSent = falsefromstop();m_notices.reset()does*this = DiscoveryNotices{}, som_probeLeftgoes false with it. Grep form_probeSent|probeSentacrosssrc/andtests/returns nothing — no dangling reference, no stale state across a restart.- The deleted comment.
-lines remove "A sweep with no socket to send on is not counted: the no-interface warning covers it." That named symptom is still prevented:sweepNowreturns at:255on emptym_socketsbefore anysendSucceeded, soprobeLeftstays false and the no-socket sweep is still suppressed. Not a regression. - Partial-send ordering inside the socket loop.
sendFaileddoes not clear the mark, and success can land before or after a failure; either order leavesprobeLeft == true, so any sweep where one socket sent still counts as asked. Yourmixedcase pins it (M3). - Sibling implementations. Grep for
silentSweep|sweepClosed|m_probeLeftoutsideHl2Discoveryis empty.AnanDiscoveryhas asweepNowbut no notices object and no silence counter, so no parallel copy carries this defect. (ANAN's own silent-bind gap was explicitly deferred on #6293 — still open, correctly out of scope here.) - Does the test discriminate? Reasoned, not run: main's
DiscoveryNoticesexposes the identical public API, so the new test compiles against it unchanged. On main,failedSweep×3 reaches the threshold and logs the silence line, so bothfailed.filter(silence).isEmpty()andfailed.size() == 2fail; thestaleblock's second assertion gets 0 instead of 1 because the count is already past 3 by then. Three discriminating checks, consistent with your M1 row. The reworked legacy checks pass on main and with the fix — correct, they are regression pins, not the fix's evidence.
Could not check. No build, no test run, no provoked socket failure — every statement above is reasoned from the code in the head checkout, not reproduced at runtime. Your body concedes the matching limit ("that sweepNow makes them is read from the code, not tested"); I confirmed the sendSucceeded/sendFailed call sites at :293/:295 the same way. I could not reproduce the mutation-testing table or the 755/758 full-suite numbers.
CI. All five checks are green on 52c9309 (build, check-macos, check-windows, Static checks, Sanitizer configure). Note that no ctest -R line in ci.yml names hl2_discovery_logging_test — green proves the new test compiles on three platforms, not that it passes. Your local macOS run is the only execution evidence.
Security audit (security-audit / CodeGuard, required by the harness): no findings. m_probeLeft is a plain bool on a single-threaded QObject with no new input boundary, sink, or logged field. Triaged and dropped: the Hl2Discovery.h:34/36/38 CS-VLN-CMDI-BACKTICK CRITICALs are lexical backtick matches on Markdown-quoted identifiers inside // comments (no shell invocation exists anywhere in the file); the CS-PII-IP / CS-VLN-SSRF-PRIV-192 hits on tests/hl2_discovery_logging_test.cpp:60-61 are in this diff but are RFC 1918 / RFC 3927 fixture constants in a test that opens no socket. The other nine files on the audit list are not in this diff and their findings are main-tree noise.
6. Recommendation
Approve with nits. The fix is correctly scoped, the seam is in the right place, and the one subtle hazard — the mark surviving into the next sweep — is handled by putting the unconditional std::exchange ahead of the answered return. The mutation table is the kind of evidence that makes a logging-policy change reviewable, and the stated limits are honest about what the socket-free test does not reach. Next step: a maintainer may want to rule on the accumulate-across-outage choice the body flags (silence before and after a send outage adding up), since it is the only judgment call here; the two comment nits can be folded in or skipped. Worth noting the branch sits on b3e0302e, two GUI-only commits behind main — no conflict with these files.
Thanks for the follow-up, and for not bundling it into #6293 after that PR had been reviewed.
🤖 aethersdr-agent · cost: $12.2863 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (116 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $13.9684 · model: claude-opus-5 |
Answers the two comment nits on the review of aethersdr#6376. Nit 1 (Hl2Discovery.h, kSilentSweepsBeforeWarning): the comment gave 15 s as a figure. A sweep no probe left neither counts nor resets, so 15 s is a floor. The comment says "at least". Nit 2 (the interface count in the silence line): it was the number of BOUND interfaces, so one socket sending and one failing read "on 2 interfaces". DiscoveryNotices now learns each bound address's interface from bindSucceeded, sendSucceeded records the interface the probe left, and sweepClosed reports how many sent in the sweep it closes. That set replaces the m_probeLeft flag: empty is "no probe left". endRefresh loses its interfaceCount argument, which nothing reads any more (one caller, Hl2Discovery::refreshSockets). The message text is unchanged apart from the number and its plural. hl2_discovery_logging_test: two bound and one sent reads "on 1 interface"; two addresses of one interface read "on 1 interface". Refs aethersdr#6376 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Fixed in
Not changed: the second error that is dropped when the directed broadcast goes out and the global one fails. As you say, it predates this PR. Still yours: whether silence before and after a send outage should add up. Full ctest at 73, ON8ST |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
The follow-up to #6293 correctly suppresses the no-answer notice when no probe was sent, while preserving mixed-success sweeps and counting sending interfaces. No blockers or outstanding nits found.
Scope
| File | Change | Claimed? | Verdict |
|---|---|---|---|
| Hl2Discovery.cpp | Track successful sends and consume per-sweep interface state | Yes | In scope |
| Hl2Discovery.h | Replace socket-existence flag with sending-interface state | Yes | In scope |
| hl2_discovery_logging_test.cpp | Cover failed, mixed, and successive sweeps | Yes | In scope |
Everything in the diff is explained by the stated fix. No unrelated changes, new protocol surface, or preference changes.
Verification
Reviewed head c818031 and GitHub merge 708f745. Built the focused socket-free test against main f753dbf with the three PR files applied; no overlapping discovery changes were present. hl2_discovery_logging_test passed. Removing the no-send guard caused three checks to fail; restoring it and rebuilding returned the test to green. ARM64 processors/executable and absence of RNNoise x86 sources were verified; RADE remained enabled. The four architecture checks had identical findings on main and the combined sources. All five current GitHub checks are green; the filtered PR CI gate does not execute this particular test.
Attacked all-failed sends, partial success, multiple addresses on one interface, and state surviving sweep boundaries. The focused tests held; answer-reset and stop/start call ordering were also inspected.
No GUI or live-radio session was run: the demo cannot reproduce HL2 discovery socket failures. Production send/reply integration was inspected from code. The accumulate-across-send-outage policy is explicit in the body and tests. No unresolved review threads remain.
Approve.
2 participants
Refs #6293
Answers the non-blocking note left on the approving review of #6293.
Hl2Discovery::sweepNowmarked a sweep as sent from the socket count,before the writes. A sweep whose every
writeDatagramfailed thereforeclosed as silence, and the third one logged the Info line "HL2 discovery:
no Hermes-Lite 2 answered 3 sweeps…", although
sendFailedhad alreadylogged the real cause.
From this PR's review (
c818031e): the silence line now counts theinterfaces a probe actually left in that sweep, not the bound ones, and the
constant's comment says "at least 15 s".
hl2_discovery_logging_testhas29 checks.
Limits
provoked: the test drives
DiscoveryNoticeswith the callsHl2Discoverymakes. ThatsweepNowmakes them is read from the code,not tested.
That is a choice: silence before and after a send outage adds up.
Change
The rule lives in
DiscoveryNotices, so it is testable without a socket.sendSucceededmarks the open sweep.sweepClosedcounts the sweep asunanswered only when it is marked, and clears the mark. One socket sending
is enough.
Hl2Discoveryreports every closed sweep and no longer keepsm_probeSent. A sweep with no socket at all is still not counted, by thesame rule.
Tests
hl2_discovery_logging_test, new functionsweepWithNoProbeSentIsNotSilence; the existing silence checks now send aprobe before each closed sweep, as the product does.
if (!probeLeft) return;guardsendFailedclears the markEach mutant was built as a scratch copy of
Hl2Discovery.cpplinked intothe test; the unmutated control passed with 0 failures. M1 failed 3
checks, M2 2, M3 1, M4 1. "Three sweeps that sent and got no answer log
the line" is the existing third-silent-sweep check; no mutant was run
against it here.
Full ctest at
c818031e,-j8: 756 of 758 passed, 2 failed, both also onthe serial rerun.
peripheral_auth_dialog_interaction_test: fails on clean main on thisMac (feat(peripherals): authenticate direct TGXL, PGXL, and Antenna Genius connections #6008); this diff touches no dialog file.
unified_title_bar_test: fails on clean main on this Mac (Unified title bar (#6198): native-platform checks still open — Windows Snap Layouts, #4557 drift, macOS M1, xcb move #6224); onlyits two known checks failed; this diff touches no title-bar file.
tools/check_*.py --strictandgen_touchpoint_manifest.py --checkpass.73, ON8ST
🤖 Generated with Claude Code