fix(hl2): bound and log the DSP-setup phase (refs #5413) - #5415
Conversation
The window between beginDspSetup() returning and finishDspSetup() being posted back had no timeout of any kind. The only connect watchdog lives in MetisClient::start(), which is reached AFTER that phase, so a stall there sat outside every guard in the path: the caller received ok/deferred -- the same reply a successful connect gives -- and then silence, with no way to tell "still working" from "never going to finish". A single-shot QTimer now watches it, armed at the end of beginDspSetup() and stopped at the top of finishDspSetup(), which covers all four of its exits, and in disconnectRadio() beside the existing generation bump. ON FIRE IT INVALIDATES AND DOES NOT TEAR DOWN. WDSP's OpenChannel cannot return early, so the I/O thread may be inside configure() on those very chains and freeing them from the GUI thread would be a use-after-free. Bumping m_connectGeneration is the same one-liner disconnectRadio() uses: the build runs to completion and finishDspSetup() takes its stale branch, which releases what it built. TWO STAGES, AND THE WARN STAGE IS EXPECTED TO FIRE. WDSP builds every FFT with FFTW_PATIENT, 35 plans per channel, and imports a wisdom cache from $HOME/.cache/aethersdr to avoid re-measuring them. Measured on one machine and binary: 4.1 s with that cache present, still unfinished at 150 s without it. So a first open on a fresh profile IS legitimately slow and a single tight timeout would fail a connect that is working. The 10 s stage says so and keeps waiting; only the 90 s stage fails. Two qCInfo lines close the reporting hole. dspSetupProgress already carries this information and has exactly one consumer, MainWindow, so the phase was visible to an operator watching a dialog and invisible to everyone else -- the aethersdr#5052 asymmetry, on the automation side. Phase begin logs the chain total; phase end logs elapsed ms from a QElapsedTimer on PendingConnect. The staging is a pure function so it is testable without a radio, a socket or an event loop, per aethersdr#5358. Mutation-checked rather than asserted: with the policy returning None always -- today's behaviour, where nothing ever fires -- 7 assertions fail; swapping the warn and fail order fails 3; strict boundaries 1; a fixed poll instead of the remainder 2. The bridge-visible connectState from the triage's item 3 is NOT here. It touches AutomationServer and RadioModel and is independently useful, so it belongs in its own PR. Checkers: check_engine_boundary.py --strict 0 blocking, 102 pre-existing tracked findings; check_test_registration.py --strict OK; git diff --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
jensenpat
left a comment
There was a problem hiding this comment.
Not merging. The fail path contradicts its own invariant and will leak (and can UAF) on a timeout that this PR's own measurements say is a working first open.
Issue-fit: #5413 asked for a DSP-setup timeout, a log of the phase, and (optionally, separately) a bridge-visible connect state. Items 1 and 2 are in this PR; item 3 is correctly deferred. The policy extraction and tests are in scope for #5358.
Scope
| File | Claimed? | Verdict |
|---|---|---|
Hl2DspSetupPolicy.h |
yes, two-stage bound | ok |
Hl2Backend.{h,cpp} watchdog + logs |
yes | blocker in fail path |
hl2_dsp_setup_policy_test.cpp + tests.cmake |
yes | ok |
Blockers
-
Fail path resets
m_pendingConnect, so the stale branch never runs.disconnectRadio()only bumpsm_connectGenerationand leavesm_pendingConnectin place sofinishDspSetup()cantearDownReceivers(). This watchdog bumps the generation and resets pending, thenfinishDspSetup()hitsif (!m_pendingConnect) return;and never tears down. The comment on the fail path is therefore false: it is not "the same one-linerdisconnectRadio()uses". -
A retry after the 90 s error is a use-after-free.
connectRadio()queues behind an in-flight build only whenm_pendingConnectis set. After the reset, a retry (automationconnect waitafterconnectionError, or the reconnect timer) callsbuildReceivers(), which the existing comment says "would destroy the very chains the I/O thread is opening". The issue measured an uncached open still unfinished at 150 s, so the 90 s fail fires whileOpenChannelis still on those objects.
Fix shape: on fail, bump generation, emit connectionError / dspSetupFinished if the caller must unblock now, but leave m_pendingConnect set until finishDspSetup()'s stale branch releases the chains. Gate a second dspSetupFinished in that branch if it would otherwise double-fire.
Nits (non-blocking)
- 90 s fail vs the PR's own 150 s still-running measurement: a fresh-profile first open will be failed while it is still working. That may be an intentional bound; it makes blocker 2 reachable in ordinary first-run use, not only a true hang.
- CI never ran (
action_requiredon the fork). Separate from the code defect.
Verified by reading the PR-head finishDspSetup / connectRadio / disconnectRadio paths against the watchdog fail arm. Policy test is socket-free and was not executed here.
…hersdr#5415 review). Principle VIII. The fail path did two things where the triage asked for one. It bumped m_connectGeneration -- correct, and the same one-liner disconnectRadio() uses -- and then ALSO reset m_pendingConnect, which disabled the very mechanism the bump exists to trigger. finishDspSetup() opens with `if (!m_pendingConnect) return;`. With pending cleared, the build that the watchdog deliberately let run to completion comes back, hits that guard, and returns before its stale branch. So tearDownReceivers() never runs and every timed-out connect leaks its WDSP channels out of the 32-slot pool. The comment above the reset claimed the opposite ("finishDspSetup() takes its stale branch, which releases what it built"), which was false as written. The retry is worse than the leak. connectRadio() queues behind an in-flight build ONLY while m_pendingConnect is set. Clearing it made the 90 s error the moment that opened the door: the operator sees "did not finish", clicks connect again, and buildReceivers() runs on the chains the I/O thread is still inside configure() on. A use-after-free reached by the ordinary connect-failed-try-again gesture. So: bump the generation, LEAVE pending set, and let the stale branch do the teardown and re-drive whatever queued behind it -- which is what disconnectRadio() has always done. The caller still needs releasing now rather than in two minutes, so connectionError and dspSetupFinished are still emitted here; a finishSignalled flag on PendingConnect keeps the stale branch from emitting a second end-of-phase edge for one connect. The flag is set BEFORE the emits, because a connectionError handler can re-enter this object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
… repeats (aethersdr#5415 review). Principle VIII. The reviewer's nit was not a nit. 90 s was chosen against one observation -- an uncached open "still running at 150 s" -- and hl2-telemetry has now measured the thing itself. A cold first WDSP open on an IDLE machine, 21 one-minute load samples all between 3.3 and 4.1, is 98269 ms against HERMES 22.3's documented 18865 ms (streams/hl2-telemetry/runs/d57_bench_quiet_result.txt). So 90 s did not fail a hang. It failed a working connect, on a quiet machine, every time. And 150 s -- the number the bound was reasoned against -- is inside the working range, not outside it. Under load the same series measured the open at 188128 ms and four whole cold connects at 193 / 195 / 214 / 219 s (streams/hl2-telemetry/runs/d57_quiet_connect.py). Load roughly doubles it, so 98.3 s is a floor, not a typical case; and the cost being measured is FFTW timing candidate plans with FFTW_PATIENT, which varies several-fold across hardware, so a laptop or a CI runner is slower again. 600 s: ~6x the quiet floor, ~2.7x the slowest measured working connect, and still finite. The asymmetry is the argument -- failing a connect that would have succeeded loses the session, while a late error on a true hang only delays a message the warn line has been announcing for minutes. AND THE WARN NOW REPEATS, because it has to. One line at 10 s followed by ten minutes of silence is barely better than the unbounded phase this PR exists to bound. dspSetupNextCheckMs() keeps the exact remainder before the warn point -- a connect that finishes in four seconds still costs zero wake-ups -- and after it returns a 30 s cadence, clamped to the remainder so the fail point is hit exactly rather than overshot by up to one interval. A zero or negative cadence degrades to a single long wait rather than arming a zero-delay timer and spinning the event loop. The test's measured-reality block asserted the old belief ("an uncached open still running at 150 s has failed"). It now asserts every measured working number must warn rather than fail, and cites the files. AND THE LARGEST FIGURE IS NOT OURS. hl2-telemetry then found aethersdr#4877 -- closed, titled "every run re-measures 190 s of PATIENT plans" -- which reports 178.7 s on an i9-13980HX and 188-190 s across four consecutive CI runs. So ~190 s is an EXPECTED cold cost in at least one shipped configuration, independently of this bench; the 188.1 s above reproduces it rather than discovering it. 600 s clears the largest DOCUMENTED figure with ~3x headroom, which is a better argument than anything about one machine here. HERMES.md's caveat that the app's plan set and the tests' plan set are different FFTW problems is carried into the header, and it cuts toward a wider bound rather than a narrower one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
…actually costs you (aethersdr#5417 review). Principle X. The reviewer caught a claim that the page's own recipe disproves. I wrote "each run writes its own cache into a temporary tree that is then deleted -- so every run is a first open, for ever". The recipe directly above exports a stable $T=/tmp/aether-hl2-test and mkdir -p's it. Nothing removes it. Its cache therefore survives, and only the first launch is slow. The headline diagnosed a lifecycle the shown commands do not have. What is true, and is the part worth knowing: a redirected HOME puts the cache somewhere the operator's own cache is not, so the FIRST isolated run is a cold open even on a machine that has connected a hundred times. After that it depends on the profile's lifetime, which the harness author chooses -- stable directory, one slow launch; mktemp -d or a cleanup trap or a fresh container, no warm run ever. Both shapes present as the same symptom, so the text now says decide it rather than discover it. REPLACED "still running at 150 s" AS THE HEADLINE NUMBER. It was the moment an observer gave up, not a completion time, and quoting it as though it bounded the phase is what produced the 90 s timeout in aethersdr#5415 that failed working connects. hl2-telemetry has since measured the open itself: 98.3 s cold on an idle machine, 188.1 s at load 38-40, 86 ms warm. The table now carries both rows and says which one to quote and why, and names the repo the raw result lives in -- it is in the bench notebook, not in this tree, and a reader following the path from here would not have found it. Two nits from the same review: - The "three variables move three different things, and only one of them is about settings" sentence contradicted the table under it: two of the three move config locations and HOME moves those too. Says what the table says now. - "aethersdr#5413" was written as though it were a landed diagnostic. It is the report; aethersdr#5415 is the fix and is not merged. On current main the silence is still what a caller gets, and the text now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
The fail path did two things where the triage asked for one. It bumped
m_connectGeneration -- correct, and the same one-liner disconnectRadio()
uses -- and then ALSO reset m_pendingConnect, which disabled the very
mechanism the bump exists to trigger.
finishDspSetup() opens with `if (!m_pendingConnect) return;`. With
pending cleared, the build that the watchdog deliberately let run to
completion comes back, hits that guard, and returns before its stale
branch. So tearDownReceivers() never runs and every timed-out connect
leaks its WDSP channels out of the 32-slot pool. The comment above the
reset claimed the opposite ("finishDspSetup() takes its stale branch,
which releases what it built"), which was false as written.
The retry is worse than the leak. connectRadio() queues behind an
in-flight build ONLY while m_pendingConnect is set. Clearing it made the
90 s error the moment that opened the door: the operator sees "did not
finish", clicks connect again, and buildReceivers() runs on the chains
the I/O thread is still inside configure() on. A use-after-free reached
by the ordinary connect-failed-try-again gesture.
So: bump the generation, LEAVE pending set, and let the stale branch do
the teardown and re-drive whatever queued behind it -- which is what
disconnectRadio() has always done. The caller still needs releasing now
rather than in two minutes, so connectionError and dspSetupFinished are
still emitted here; a finishSignalled flag on PendingConnect keeps the
stale branch from emitting a second end-of-phase edge for one connect.
The flag is set BEFORE the emits, because a connectionError handler can
re-enter this object.
Refs: PR aethersdr#5415, review round 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
|
Both blockers fixed, at The fail path no longer resets The bound is 90 s → 600 s, and the nit was right to insist. The 90 s came from an observation quoted as if it were a bound — "still running at 150 s" was the moment an observer gave up, not a measurement of anything. Five measurements since, on this machine: a cold One thing stated rather than implied: the pending-reset fix has no regression test. A socket-free one needs an CI still has not run on this head ( |
The reviewer's nit was not a nit. 90 s was chosen against one observation
-- an uncached open "still running at 150 s" -- and hl2-telemetry has now
measured the thing itself. A cold first WDSP open on an IDLE machine, 21
one-minute load samples all between 3.3 and 4.1, is 98269 ms against
HERMES 22.3's documented 18865 ms
(streams/hl2-telemetry/runs/d57_bench_quiet_result.txt).
So 90 s did not fail a hang. It failed a working connect, on a quiet
machine, every time. And 150 s -- the number the bound was reasoned
against -- is inside the working range, not outside it.
Under load the same series measured the open at 188128 ms and four whole
cold connects at 193 / 195 / 214 / 219 s
(streams/hl2-telemetry/runs/d57_quiet_connect.py). Load roughly doubles
it, so 98.3 s is a floor, not a typical case; and the cost being measured
is FFTW timing candidate plans with FFTW_PATIENT, which varies
several-fold across hardware, so a laptop or a CI runner is slower again.
600 s: ~6x the quiet floor, ~2.7x the slowest measured working connect,
and still finite. The asymmetry is the argument -- failing a connect that
would have succeeded loses the session, while a late error on a true hang
only delays a message the warn line has been announcing for minutes.
AND THE WARN NOW REPEATS, because it has to. One line at 10 s followed by
ten minutes of silence is barely better than the unbounded phase this PR
exists to bound. dspSetupNextCheckMs() keeps the exact remainder before
the warn point -- a connect that finishes in four seconds still costs
zero wake-ups -- and after it returns a 30 s cadence, clamped to the
remainder so the fail point is hit exactly rather than overshot by up to
one interval. A zero or negative cadence degrades to a single long wait
rather than arming a zero-delay timer and spinning the event loop.
The test's measured-reality block asserted the old belief ("an uncached
open still running at 150 s has failed"). It now asserts every measured
working number must warn rather than fail, and cites the files.
AND THE LARGEST FIGURE IS NOT OURS. hl2-telemetry then found aethersdr#4877 --
closed, titled "every run re-measures 190 s of PATIENT plans" -- which
reports 178.7 s on an i9-13980HX and 188-190 s across four consecutive
CI runs. So ~190 s is an EXPECTED cold cost in at least one shipped
configuration, independently of this bench; the 188.1 s above reproduces
it rather than discovering it. 600 s clears the largest DOCUMENTED figure
with ~3x headroom, which is a better argument than anything about one
machine here. HERMES.md's caveat that the app's plan set and the tests'
plan set are different FFTW problems is carried into the header, and it
cuts toward a wider bound rather than a narrower one.
AND THE BOUND IS REASONED FROM ONE-RECEIVER DATA ONLY. beginDspSetup()
opens one chain per receiver and the transmit path opens none, so the
cost scales with actualNumRx -- and every figure above is a one-receiver
connect, while the board reports four. Plan sets overlap heavily enough
that receivers 2..N at the same rate are probably nearly free, but that
is an expectation and it is written into the header as one. Named rather
than assumed, so a four-receiver first connect that fails this bound
reads as the missing measurement it is.
Refs: PR aethersdr#5415, review round 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
1cc044f to
d0d6bb9
Compare
The reviewer caught a claim that the page's own recipe disproves. I wrote "each run writes its own cache into a temporary tree that is then deleted -- so every run is a first open, for ever". The recipe directly above exports a stable $T=/tmp/aether-hl2-test and mkdir -p's it. Nothing removes it. Its cache therefore survives, and only the first launch is slow. The headline diagnosed a lifecycle the shown commands do not have. What is true, and is the part worth knowing: a redirected HOME puts the cache somewhere the operator's own cache is not, so the FIRST isolated run is a cold open even on a machine that has connected a hundred times. After that it depends on the profile's lifetime, which the harness author chooses -- stable directory, one slow launch; mktemp -d or a cleanup trap or a fresh container, no warm run ever. Both shapes present as the same symptom, so the text now says decide it rather than discover it. REPLACED "still running at 150 s" AS THE HEADLINE NUMBER. It was the moment an observer gave up, not a completion time, and quoting it as though it bounded the phase is what produced the 90 s timeout in aethersdr#5415 that failed working connects. hl2-telemetry has since measured the open itself: 98.3 s cold on an idle machine, 188.1 s at load 38-40, 86 ms warm. The table now carries both rows and says which one to quote and why, and names the repo the raw result lives in -- it is in the bench notebook, not in this tree, and a reader following the path from here would not have found it. Two nits from the same review: - The "three variables move three different things, and only one of them is about settings" sentence contradicted the table under it: two of the three move config locations and HOME moves those too. Says what the table says now. - "aethersdr#5413" was written as though it were a landed diagnostic. It is the report; aethersdr#5415 is the fix and is not merged. On current main the silence is still what a caller gets, and the text now says so. Refs: PR aethersdr#5417, review round 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
|
Head is now One substantive addition beyond that, and it is a gap rather than a fix: every measurement the 600 s bound rests on is a ONE-receiver connect. |
…nciple X. The paragraph I added an hour ago said the bound is reasoned from one-receiver data "while the board reports four". True, and misleading: it implies a four-receiver first connect is an ordinary thing this timer might meet. Three things in the code say otherwise, all read rather than assumed. connectRadio() sets m_requestedNumRx = 1 unconditionally and raises it only for an explicit `numRx` CONNECT PARAM -- a saved count is deliberately not re-imposed, per the comment there -- and nothing in RadioModel passes that param. So an ordinary app connect opens exactly one chain. createPanadapter() refuses before m_connected, so a receiver the operator adds later opens its chain AFTER the phase this watchdog measures. It is outside the window entirely. receiverCeiling() is min(board, maxReceiversAtRate(rate, board)), and at 384 kHz -- the rate every measurement in the table used -- a 4-receiver board is honestly 3. A four-receiver run would have to change the rate, which changes the plan set, and the number would not belong in the same table as the ones it was meant to extend. What is actually reachable is an automation or embedder caller passing numRx>1 at connect: the same class of entry point as aethersdr#5402's lnaGainDb defect. That is the sentence, and it is a smaller worry than the one it replaces. Kept as a commit on top rather than an amend, because d0d6bb9 is pushed and a reviewer may already have read it. The correction is worth more visible than tidy. Credit: hl2-telemetry raised the ceiling and the post-connect guard against their own earlier framing; the m_requestedNumRx path is from reading connectRadio() here. Refs: PR aethersdr#5415, review round 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
|
Head It narrows the multi-receiver caveat I added two pushes ago, which was true but misleading: it implied a four-receiver first connect is an ordinary thing this timer might meet. Three things in the code say otherwise. What is left is an automation or embedder caller passing Separately, and not worth a rewrite: |
Audit prompted by console: for each assertion, what would have to be true of the world for it to be worth asserting? An assertion whose answer names a DEFECT is load-bearing only while that defect exists, and must say so, or fixing the defect leaves a green test defending the workaround. The measurement block here is the one case in this file with a conditional answer. It is load-bearing because a cold FFTW_PATIENT plan measurement really does take minutes -- aethersdr#4877, still the largest documented figure. If that issue's fix lands, or WDSP stops planning with FFTW_PATIENT, those durations stop describing anything real. They would still PASS, because they only assert such durations must not FAIL. So this one goes green-and-pointless rather than green-and-wrong, which is the better failure but still a reason to re-read the block rather than trust it. Said in the file, where someone fixing aethersdr#4877 will be. Refs: PR aethersdr#5415, review round 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg
There was a problem hiding this comment.
1. Issue fit
Partially — by design, and correctly scoped. #5413 lists three fixes; this PR implements items 1 (a bound on the DSP-setup phase) and 2 (a qCInfo at phase begin and end), and the author's coverage-map comment on the issue routes item 3 (connectState on the bridge) to #5416. Both halves land where the triage said they must: the watchdog is armed at the end of beginDspSetup() (Hl2Backend.cpp:2008), which is the exact window the triage identified as uncovered, and it invalidates rather than tears down — ++m_connectGeneration with m_pendingConnect left set (:2133), which is the constraint the triage comment spelled out and which @jensenpat's review comment flagged against an earlier revision. That comment is addressed at head: the reset() is gone, and the stale branch at :2163-2183 is still the only thing that calls tearDownReceivers().
One process nit below: commit 3a3c17e's message still says (fixes #5413) while the title says refs.
2. Scope
| File | What it changes | Claimed by the issue/body? | Verdict |
|---|---|---|---|
Hl2Backend.cpp |
Phase clock + finishSignalled on PendingConnect; two qCInfo lines; armDspSetupWatchdog()/onDspSetupWatchdog(); timer stop in finishDspSetup() and disconnectRadio() |
Yes — items 1 and 2 | In scope |
Hl2Backend.h |
Two private methods + QTimer* m_dspSetupWatchdog |
Yes | In scope |
Hl2DspSetupPolicy.h |
New pure policy header (warn/fail stages, next-check schedule) | Yes — the Hl2TxLevelPolicy.h idiom |
In scope |
tests/hl2_dsp_setup_policy_test.cpp |
20 assertions on the policy | Yes | In scope |
tests/tests.cmake |
Registers the target | Yes | In scope |
Nothing unexplained. No CHANGELOG.md entry — correct. No new public or protocol surface: dspSetupProgress/dspSetupFinished already existed, connectionError is the proven path the triage asked to reuse, and the bridge-visible state (which would be new surface) is correctly deferred to #5416. The only - lines are a brace reflow and moving one emit dspSetupFinished() under if (!alreadyFinished); no guard or symptom-naming comment is deleted. Registration in tests.cmake:3971-3975 matches the sibling hl2_tx_level_policy_test block byte for byte.
Socket tests: none added, modified, or removed. The new test includes only <cstdio> and the policy header — no QTcpServer/QUdpSocket/bind/listen/connectToHost, no Fake* peer, no Qt link at all.
CodeGuard: all 12 findings dropped. Every one is in a file this PR does not touch (appimage.yml, macos-dmg.yml, MainWindow.cpp, MemoryDialog.cpp, hl2_live_band_filter_probe.cpp, local_control_server_test.cpp). None is attributable to this diff.
3. Blockers
None. Nothing here regresses main — every path this changes was previously unbounded and silent.
4. Findings and nits (non-blocking)
- The retry after the timeout is still unbounded and silent — #5413's own symptom, one level up. (Inline at
Hl2Backend.cpp:2143.) The timer is single-shot and the Fail branch does not re-arm it, andconnectRadio()'s queue branch (:1626-1632) neither arms nor restarts it.RadioModel::onConnectionError(RadioModel.cpp:7251-7255) starts a 5 s reconnect on exactly this error, so the next thing that happens after the new message is a connect that logs"queued behind it", is stored inm_queuedConnect, and returns — with no bound and no reply. For a merely-slow build this resolves itself (the build completes, the stale branch re-drives the queue). For a genuinely hung build — the only case the 600 s bound exists to catch — the process is then permanently unable to connect, silently, which is the state the issue describes. This is the strongest thing I found; it is still a net improvement overmain, which is why it is not a blocker. - The setup dialog can reappear after the failure, for
numRx > 1. (Inline at:2136.)finishSignalledsuppresses the seconddspSetupFinished, but nothing suppressesdspSetupProgress: the still-running build keeps emitting it per receiver (:2046-2049), andMainWindow.cpp:6880re-armsarmWdspSetupDialog()on every one — an application-modal, Cancel-less window after the operator was told the connect failed. Unreachable on an ordinary connect (m_requestedNumRx = 1at:1752, and the single progress emit precedes receiver 0'sconfigure()), so this is narrow — but it is the samenumRxconnect-param entry point the policy header already reasons about at length. - Nothing asserts that the backend asks the policy. (Inline on the test.) All 20 assertions would stay green with
armDspSetupWatchdog()'s body emptied. This is the author's own precedent from #5414 — "twice the rule was right and nothing called it, and every unit test stayed green" — which added a wire test through a realHl2Backendfor exactly this. Not requesting one here: the wiring is timer/lifecycle behaviour with no obvious deterministic socket-free seam, and per AGENTS.md's test-layer boundary that is sanitizer-lane or bridge territory, not CTest. Worth naming rather than gating. - The warn repeats every 30 s up to the 600 s bound, so a legitimately slow-but-working first open now writes up to ~19
WRNlines into a support bundle for a connect that succeeded. Deliberate per the comment at:2101, and I think the trade is right — flagging only so it is a choice on the record. 3a3c17e's message saysfixes #5413; the title and the issue's coverage map sayrefs. As written, merging this auto-closes the issue while #5416 still owns item 3. Worth amending.
5. What I tried to break
- The use-after-free @jensenpat named. Walked it at head: the Fail branch leaves
m_pendingConnectset, soconnectRadio()takes its queue branch instead ofbuildReceivers();disconnectRadio()(:2294-2317) bumps the generation, stops the timer and clears the queue but calls no teardown. Nothing frees a chain from the GUI thread while the I/O thread may be insideconfigure(). Held. - Reentrancy through
connectionError. Both handlers —RadioModel::onConnectionError(:7234) andMainWindow::onConnectionError(:6535) — touch neitherm_pendingConnectnor the receivers, andfinishSignalledis set before the emit (:2136), so a handler that re-enters cannot produce a second end-of-phase edge. Held. - Double
dspSetupFinished. The other four emit sites (:2174,:2208,:2279,:2288) are all on the non-stale path, which the generation bump at:2133makes unreachable after a Fail. Held. - Timer clobber ordering.
finishDspSetup()stops the watchdog at its top (:2151), before the stale branch re-drivesm_queuedConnectthroughconnectRadio()→beginDspSetup()→armDspSetupWatchdog(), so the re-drive's fresh arm is not cancelled by the outer stop. Held. - An unstarted
QElapsedTimer.clock.start()always precedes theinvokeMethodthat is the only thing which postsfinishDspSetup(), so no path reads an invalid timer. Held. - Arithmetic and the
intcasts. EverydspSetupNextCheckMs()return is ≤ 600000 and fitsint; the final-wait clamp and therepeatMs <= 0zero-delay-spin guard are both asserted in the test. Held. - The policy header's factual claims about
numRx, which is what justifies 600 s:m_requestedNumRx = 1unconditionally, raised only by the explicit param (:1752-1756), andreceiverCeiling()at:816. Accurate as written. - Build risk on the Linux Qt floor.
QTimerandQElapsedTimerare already included inHl2Backend.h:6,9; the new header pulls only<cstdint>. Nothing Qt-6.5-only.
Could not check: anything at runtime — I am headless, so every finding above is reasoned from the code at /tmp/aetherclaude/pr-5415, not reproduced. get_check_runs returned zero runs for the head commit, so I could not confirm CI state either way; note that no ctest -R filter in ci.yml names hl2_dsp_setup_policy_test, so it is built but not merge-gating. I also cannot verify the bench figures cited in the policy header (streams/hl2-telemetry/runs/d57_*); the #4877 cross-check of ~190 s is at least an independent second source for the upper end.
6. Recommendation
Approve with nits. The change is well-targeted at the window the triage identified, it respects the one hard constraint (the build cannot be cancelled, so invalidate and let the stale branch clean up), and it survived every lifecycle and reentrancy attack I could construct. The one thing I would like the author to answer before merge is the first finding: after the watchdog gives up, the automatic 5 s reconnect is swallowed with no bound, so on a true hang the process ends up back in exactly the state #5413 reports. A gaveUp flag on PendingConnect that makes connectRadio()'s queue branch emit connectionError immediately rather than returning silently would close it in a few lines. Nice work on the measurement trail behind the 600 s bound — replacing the 90 s figure once it was shown to be an observer's stopwatch rather than a measurement is the right instinct.
🤖 aethersdr-agent · cost: $5.9253 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (12 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: $6.0916 · model: claude-opus-5 |
|
@jensenpat — both blockers are addressed and your non-blocking nit turned out to be the more serious of the two findings. Head is Blockers 1 and 2 — Now: bump the generation, leave The nit was the real defect — Completion times since:
#4877 is the citation that matters, because it is not ours: ~190 s is already documented as the expected cold cost in a shipped configuration. 600 s clears that with ~3× headroom and the worst measured connect with ~2.7×, and is still finite. The asymmetry is the argument — failing a connect that would have succeeded loses the session; a late error on a true hang only delays a message the warn line has been repeating for minutes. The warn therefore repeats — Two limitations written into the header rather than left for you to find:
One thing I am not claiming. Your other nit — CI never ran, |
Ozy311
left a comment
There was a problem hiding this comment.
Takeover review at 18b49e65ffbc016b9000145b51a78fa400bdf5f4, under Ozy311's explicit direction to take ownership, resolve blockers, and squash merge. Trusted governance: bc8017740de7cce1ddbc90bbb69bbfafa00ec44b; manifest PASS and constitution copies identical.
Issue fit: implements #5413 items 1 and 2: warn/fail during the asynchronous DSP-open window and log phase begin/end. #5416 remains open for item 3, so the squash message will use Refs #5413.
Scope: everything in the diff is explained by the issue.
| File/group | Purpose | Verdict |
|---|---|---|
| Hl2Backend.cpp/.h | Phase timer, elapsed logging, safe invalidation and completion signal | In scope |
| Hl2DspSetupPolicy.h | Warning/failure decisions and scheduling | In scope |
| Policy test + tests.cmake | Socket-free assertions and registration | In scope |
Blockers: none remain from the original review. At the current head the timeout leaves m_pendingConnect intact and increments the generation. connectRadio() therefore queues retries instead of rebuilding DSP objects while the worker is using them. The stale completion branch can now reach tearDownReceivers() and re-drive a queued connect. finishSignalled is set before notification and suppresses duplicate completion. The incorrect 90-second bound is now 600 seconds. These lifecycle conclusions are code-reviewed, not runtime reproductions.
Empirical verification: compiled the exact policy test with Clang/C++20 and -Wall -Wextra -Werror; all 23 assertions passed. Five separate scratch mutations failed as expected: no watchdog decisions (12 assertions), old 90-second limit (5), strict boundaries (3), fixed polling (1), no final clamp (1). Strict engine-boundary check found zero blockers (102 tracked findings), registration and whitespace checks passed. Linux/macOS/Windows configure and build steps plus both static jobs passed on this exact head. All five commit signatures are valid. A clean three-way merge preserves newer HL2 IO-board, capability, and telemetry changes on main.
Non-blocking limitations accepted for this merge: the retry after a permanently stalled build remains queued without a new watchdog; an explicit multi-receiver connect can deliver later progress after timeout; policy tests do not prove production timer arming or asynchronous cleanup. The PR body now states these explicitly. No hardware, live-radio/TX, or GUI session was run; SimBackend cannot exercise this HL2-only path. No new/modified socket test exists. Cold timing measurements and multi-receiver timing were not independently reproduced. The existing CodeGuard report was inspected; its listed paths are outside this five-file diff, and no new CodeGuard run was performed locally.
I also checked disconnect timer cancellation, stale-completion cleanup ordering, queuing before receiver destruction, and completion-before-rearm ordering against the surrounding callers. No additional blocker found. The original inline pending-reset finding is addressed; the three other threads remain documented limitations rather than claimed fixes.
Recommendation: approve with the limitations above. No additional source change was needed during takeover. PR description corrected to the current 600-second behavior and current validation. This approval is submitted for the authorized Tier 3 human account Ozy311 (live reviewer membership confirmed).
Superseded by verified takeover review at 18b49e6: pending build is preserved on timeout, retries queue safely, stale completion cleans up, and fail bound is 600s. Ozy311 explicitly authorized taking ownership and resolving blockers. Both original blockers are addressed; see the new review for tested evidence and coverage limits.
…5417) ### What and why **This is the harness-side half of #5413's cause.** #5415 bounds and logs the DSP-setup phase; this explains why that phase takes minutes in a harness and seconds otherwise. §10 tells harness writers to redirect `HOME`, `CFFIXED_USER_HOME` and `XDG_CONFIG_HOME` for an isolated profile. Two bullets later it promises that after the first ~19 s open, **"every later open — any receiver, any sample rate — is 40–175 ms."** That second claim assumes the FFTW wisdom cache survives between runs, and the recipe above it is what destroys it. **The page contradicts itself, and the reassuring half is the wrong one** — which is why the paragraph goes directly under that bullet rather than at the end of the section. ### The mechanism The three variables move three different things, and only two are about settings: | variable | what it moves | |---|---| | `CFFIXED_USER_HOME` | Qt's config and log locations | | `XDG_CONFIG_HOME` | the same, where consulted | | **`HOME`** | everything from `QDir::homePath()` — **and the WDSP wisdom cache** | `WdspChannel::wisdomPath()` resolves to `$XDG_CACHE_HOME`, else `$HOME/.cache`, then `/aethersdr/wdsp-fftw-wisdom`. A redirected `HOME` points it at an empty directory, and each run writes its own cache into a temporary tree that is then deleted — so **every run is a first open, for ever.** Far worse than ~19 s: WDSP builds every FFT with `FFTW_PATIENT`, 35 plans per channel. Measured on one machine and one binary, changing **only** whether that cache was reachable: | wisdom cache | connect | |---|---| | present | **4.1 s** | | absent | **still running at 150 s** | ### The fix, and the way it silently undoes itself One variable the code already provides for its own tests — `AETHER_WDSP_WISDOM_DIR` — pointed at a directory that outlives the run. **The example is absolute deliberately, and the text says why.** `HOME` is the variable this recipe redirects, so a `$HOME`-relative path lands *inside* the temporary tree and is deleted with it: the fix deletes itself, and the symptom is indistinguishable from never having applied it. On a single command line with `HOME=$T` in front it appears to work, by luck of shell expansion order; in a script that sets `HOME` first it does not. ### Two traps named because both cost a day - **There are two FFTW wisdom files.** `AudioEngine::wisdomFilePath()` under `~/.config/AetherSDR/` for NR2, and `WdspChannel::wisdomPath()` under `~/.cache/aethersdr/` for the channels. The startup line `Audio NR2 wisdom summary: status=missing` refers to the **first**, is routinely absent, and says nothing about the second. - **Until #5413 the stall produced no diagnostic at all** — no timeout, no log line — so it presented as a hang rather than as slowness. ### Citations **By symbol, not line** — `WdspChannel::wisdomPath()`, `AudioEngine::wisdomFilePath()`, `QDir::homePath()` — so the paragraph does not rot the way line citations in this file's neighbours have. Verified: three symbols, zero line citations. ### Validation One commit, one file, +59, no deletions. `git diff --check` clean. No build: this changes no code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FtHEsQsghFwhUQEZdwVKUz --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Ozy311 <Ozy311@users.noreply.github.com>
…5416) This completes #5413 item 3: radio snapshots now expose `connectState` (`idle`, `connecting`, `connected`) beside the existing `connected` boolean. The value uses RadioModel's existing whole-attempt lifecycle, from `connectToRadio()` until success, failure or cancellation. Connected state takes precedence. It does not create an HL2-only DSP progress flag. Address probing before the model request and retry backoff can still read `idle`; this matches the existing `connect wait` boundary. Scope: ConnectStatePolicy and RadioModel mapping, AutomationServer publication, bridge documentation, and two registered tests. #5415's setup diagnostics are already on main; this adds the polling state independently. Validation on the repaired head: both socket-free policy and actual model/bridge tests pass on macOS arm64. The model fixture uses existing private-state access to inject the attempt input, then calls the production getter, bridge dispatcher, cancellation and failure methods. It never starts transport or a bridge listener. Making the production getter ignore the attempt flag causes four model/bridge assertions to fail while the pure policy test stays green; restoring it passes 2/2. Strict registration, engine boundary and frozen CI gate checks pass. Coverage limit: the injected fixture does not exercise the request-edge assignment inside connectToRadio or a real radio session. No radio, RF/TX or full GUI validation was performed in this repair. After #5401 landed, the final signed main integration retained both automation test registrations. The combined rebuild and all four connection-state/DSP-readback tests passed (4/4). Published-head CI is reported by the checks below. Original contribution by @on8st with Claude Code; release-day integration and test repair by @Ozy311 with Codex. Signed main integration preserves upstream changes. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Ozy311 <Ozy311@users.noreply.github.com>
Refs #5413 — implements the DSP-setup watchdog and phase logging (items 1 and 2). #5416 remains open for bridge-visible connect state, so this PR must not close #5413.
beginDspSetup()opens the receive DSP chains asynchronously beforeMetisClient::start()can arm its connection watchdog. This PR covers that previously unwatched phase with a single-shot timer: warn after 10 seconds, repeat the warning every 30 seconds, and report failure after 600 seconds. Beginning and completing the phase also write categorized log entries, including the chain count and elapsed time.Cold FFTW planning can legitimately take minutes. The author reported 98–219-second cold opens/connects, with independent historical figures around 179–190 seconds in #4877; the original 90-second limit was therefore raised to 600 seconds. These timing measurements were not independently repeated during the takeover review.
On timeout the backend invalidates the connect generation and leaves the pending build alive. WDSP cannot cancel an in-flight OpenChannel safely; cleanup runs only when the existing completion path receives the stale build. Retrying while it is opening queues behind it, preserving the DSP objects. A flag suppresses a duplicate dspSetupFinished signal. The watchdog stops on completion and disconnect.
Validation at 18b49e6
Coverage and remaining limitations
The timing policy is tested; production timer arming, asynchronous cleanup, and signal ordering were inspected in code rather than reproduced in a stalled backend. No full local application build, GUI session, live-radio session, or TX test was performed; the demo backend does not exercise HL2 DSP setup. No socket-owning test is added or modified.
A permanently stuck WDSP build remains stuck: after the reported failure, later retries can queue without another watchdog, and shutdown/family switching can still wait for the worker. For an explicit multi-receiver connect, later progress signals can reopen the setup dialog after timeout. Those existing review observations remain non-blocking limitations, not claims of fixes delivered here. Ordinary application connects start with one receiver; multi-receiver cold planning was not measured independently.
The diff is limited to Hl2Backend.cpp/.h, Hl2DspSetupPolicy.h, its policy test, and registration in tests/tests.cmake. No CHANGELOG entry or unrelated code changes.
Principle VIII: Evidence Over Assertion.