[on8st]

Refs #6154 (finding 1)

Limits first.

1. ASan was run on macOS only (Apple clang, arm64, Qt 6.12.0, not instrumented), and only the one target offline_health_registry_test was built that way. The Linux lane image was not used.
2. One test file. No product code, no tests.cmake change; the touchpoint manifest regenerates unchanged.
3. [sanitizer] ASan + UBSan weekly run failure (fp 55e8f7b54c82) #6154 holds other findings. This PR touches none of them.

What was wrong

Our mistake, from #5642. Section 5 of tests/offline_health_registry_test.cpp re-declares the hl2 factory through OfflineHealthRegistry::declare() with a lambda that captures RecorderState rec by reference, and leaves it installed on purpose: section 6 needs hl2 to keep building the socket-free double. rec was declared inside section 5's block. Section 6's rebuildBackendForTest("hl2") then built a RecordingOfflineSource on the dead slot, and RadioModel::offlineHealthRows() wrote through it in noteOfflineDemand().

The change

rec is declared at main scope, before any RadioModel, so it outlives every source the factory can build. The factory stays installed. Restoring the real hl2 declaration instead would make section 6 aim the real telemetry service, which binds a socket. The only other declare() in the file (flex, section 6) captures nothing.

Proof

ASan, -DAETHERSDR_SANITIZER=address, this one target:

• at main (8df6db2f): aborts after 41 checks with stack-use-after-scope in RecordingOfflineSource::noteOfflineDemand() (offline_health_registry_test.cpp:95), called from RadioModel::offlineHealthRows() and main at :311; ASan names the variable, 'rec' (line 197).
• at this head: 53 checks OK, 0 failed, no ASan report.

The Linux lane's own report in #6154 names the same site and the same variable, so the diagnosis is confirmed on both platforms; the fix was run under ASan on macOS only.

Full suite at this head (macOS arm64, Qt 6.12.0, -j8): 641 of 643 pass. The two failures are known on main on this Mac and untouched here: peripheral_auth_dialog_test (#6008, host serial-port names) and vkamp_connection_test (#5254, its two known checks). Both also fail when rerun alone.

Static checks: every script step of the "Static checks" job passes locally; no src/gui file changed.

Not verified

• The full suite under ASan, and TSan.
• Linux and Windows locally; CI only.
• Whether the stack-use-after-scope ever changed a result in a normal build. The test passed before and passes now.

73, ON8ST

🤖 Generated with Claude Code

[aethersdr-agent]

Issue fit

Yes, and it is the right diagnosis. #6154's lead SUMMARY block is literally this defect — AddressSanitizer: stack-use-after-scope tests/offline_health_registry_test.cpp:95 in RecordingOfflineSource::noteOfflineDemand(), with the shadow map naming 'rec' (line 197) and the neighbouring frame 'm' (line 306), which is section 6's RadioModel. main's line 197 is RecorderState rec; inside section 5's block and line 95 is void noteOfflineDemand() override { ++m_st->demands; }. The diff moves exactly that declaration out, and the factory installed at line 203 — [&rec](QObject*) — now captures something that outlives every source it can build.

Scoped to finding 1 of six in that sticky, which the body states. Refs rather than Fixes is the correct verb: the two timeouts and the three QPointer<QLabel> UBSan downcasts in VfoWidget::eventFilter remain, so the sticky must stay open.

Scope

File / group	What it changes	Claimed by title/body	Verdict
tests/offline_health_registry_test.cpp (+5/−1, two hunks)	hoists RecorderState rec from section 5's block to main scope, with a comment stating why	yes	In scope

Everything in the diff is explained by the stated fix. No product code, no tests.cmake change, no CHANGELOG.md entry, no src/gui file, no new public surface, no removed guard. The one - line is the declaration being moved, not deleted.

Socket-test surfacing: this PR neither adds nor removes a socket test or fake peer. The test owns no socket by construction — section 5 injects a RecordingOfflineSource precisely so the real Hl2TelemetryPoller never binds — and the diff does not touch that arrangement.

Blockers

None.

Nits

• tests/offline_health_registry_test.cpp:136-139 — the comment explains the lifetime requirement, but nothing in the file enforces it: nothing asserts on rec after section 5, so a future edit that moves the declaration back into a block, or wraps sections 5–6 differently, reintroduces the UB silently and the test still passes in a normal build (which is how it got here). Non-blocking, and main scope is the simplest correct answer — but if you want a belt-and-braces version, static RecorderState rec; at file scope makes the lifetime unconditional and independent of where main's locals sit. Your call; the current fix is correct as written.
• The body's ASan evidence is macOS-only, one target, and says so. Worth noting that #6154 is the independent Linux-lane confirmation the body lists under "Not verified" — GCC 11.5.0 / Qt 6.12.0 / ubuntu 24.04, same file, same line, same variable. Citing it would strengthen the proof section at zero cost.
• Process, not code: #6154 would benefit from a comment saying finding 1 is addressed here, so the next triager reading that sticky does not re-diagnose line 95 from scratch. Per the fingerprint-reuse pattern, stickies named by test get re-closed against the wrong cause easily.

What I tried to break

• Is the fix complete, or does some source outlive rec anyway? RadioModel::ensureOfflineHealth() (src/models/RadioModel.cpp:4547) stores the source in a unique_ptr member parented to the model, and every RadioModel in this file is block-scoped. The last one dies at section 6's closing brace, so no RecordingOfflineSource survives into main's epilogue or into static destruction — even though OfflineHealthRegistry::table() is a process-global QHash<QString, Factory> that keeps the capturing lambda alive to the end. Closed.
• Does hoisting rec perturb section 5's own assertions? Sections 1–4 never touch it: section 3 calls OfflineHealthRegistry::create() against the real hl2 factory (before the re-declare), section 4 builds Flex. So rec is still default-constructed when section 5 starts, and !rec.destroyed, rec.target.isNull() and the demandsBefore delta all mean what they meant. Section 6 never reads rec. One rec symbol in the file, no shadowing.
• How bad was it on main, really? Reasoned from code, not run: section 6 makes roughly six writes through the dangling pointer — ++demands on the first offlineHealthRows(), m_st->destroyed = true on the rebuildBackendForTest("flex") release, then m_st->target = addr from setOfflineHealthTarget("hl2", 192.0.2.1) plus ++aims/++demands, then another destructor write. The target assignment is the nastiest: QHostAddress::operator= releases the old d first, so it refcount-decrements through whatever now occupies that stack slot (per #6154's shadow map, adjacent to section 6's RadioModel). The test passed on main only because the clobbered bytes happened not to matter.
• Other capturing factories left behind? Grepped the head file: two declare() calls. The flex one in section 6 captures nothing. No sibling to fix.
• The body's arithmetic. 48 check( call sites, section 2's loop runs its one site six times → 53 executed, matching the claimed "53 checks OK". Sections 1–4 are 19, section 5 is 17, 5b is 3, plus section 6's first two → 41, matching the claimed abort point, which lands on section 6's first offlineHealthRows() after the hl2 rebuild. Both numbers are exactly right.
• CI reality, not the green badge. This test is in no ci.yml -R filter, so the per-PR gate never ran it in either state; the five green checks on 15affff prove it compiles and that static checks pass, nothing about the UB. sanitizers.yml:263 runs the full ctest, which is why ASan caught it and is the only lane that would. I did not dispatch that workflow on this branch — a PR-branch sanitizer dispatch files a bogus "weekly" sticky — so the Linux verification here is #6154's own report rather than a fresh run.

Findings are reasoned from the code and the sanitizer report; I built and ran nothing.

Recommendation

Approve with nits. Minimal, correct, and correctly scoped: it fixes the named finding without touching the other five, keeps the injected socket-free double that section 6 depends on, and resists the tempting wrong fix (restoring the real hl2 declaration, which would put section 6 back on a UDP socket — the body says so explicitly, and the file's header documents why that matters). Next step is a maintainer merge; the nits are optional and none needs a round trip. Nice catch on the declare()-outlives-its-capture shape — that one is invisible in a normal build by construction.

---

_{🤖 aethersdr-agent · cost: $8.4491 · model: claude-opus-5}

[on8st]

Two of the three nits are taken, no code change:

• The body now cites [sanitizer] ASan + UBSan weekly run failure (fp 55e8f7b54c82) #6154's lane report as the Linux confirmation of the diagnosis.
• [sanitizer] ASan + UBSan weekly run failure (fp 55e8f7b54c82) #6154 has a pointer to this PR for finding 1.

Left as it is: rec at main scope rather than static at file scope. It is correct as written, and I would rather not add a second lifetime rule to the file; say if you prefer the static.

73, ON8ST

[on8st]

@ten9876, your branch fix/asan-standing-findings carries the same fix (1f9c4ed6, rec moved to main scope), so this PR duplicates it and the two would conflict on that hunk. Yours also takes the DSP lock in SetTXABandpassFreqs (patch 16), which answers the question I asked on #6156.

I will close this one when your PR is up, unless you would rather merge it first. #6164 (the seam test's wall-clock check) is not in your branch and stays as it is.

73, ON8ST

[ten9876]

Issue fit

Yes — head 15affff087c84d3709ac9145a2f92b8bc109a8d8 fixes finding 1 of #6154. The factory installed in section 5 captures rec by reference; section 6 still uses that factory. Moving the recorder into main scope keeps it alive until after all block-scoped models/sources are destroyed. The other sanitizer findings remain outside this PR and the issue correctly uses Refs.

Scope

File	Change	Claimed?	Verdict
tests/offline_health_registry_test.cpp	Move the recorder declaration outward and explain its required lifetime	Yes	Entire diff belongs to finding 1

The one commit's authored date and subject fit this work. No product changes, new public identifiers, defaults, removed guards, test deregistration, or CHANGELOG changes. The modified target remains socket-free: the captured recording source substitutes for the real telemetry transport before the later aim calls.

Blockers

None.

Nits (non-blocking)

None. The previous review's suggestion to make the recorder static is unnecessary: all source instances die inside their model blocks; destruction of a retained reference-capturing lambda does not dereference its referent.

What I verified

• Built a focused Linux AddressSanitizer reproduction using the exact RecorderState and RecordingOfflineSource definitions from this head and its production OfflineHealthRegistry.cpp. The fixed lifetime ran successfully. Moving the declaration back into the factory-installation block reproduced AddressSanitizer: stack-use-after-scope in RecordingOfflineSource::noteOfflineDemand(). This is a minimized registry/lifetime probe, not a run of the full offline_health_registry_test target.
• Read sections 1–6 and all source destruction paths. Earlier sections cannot touch the hoisted recorder before the replacement factory is installed; the other installed factory captures nothing. Keeping the recording factory avoids reintroducing socket activity.
• Compared #6154's original report: it identifies the same dead rec and demand callback. This supplies independent Linux evidence for the original defect; the PR body accurately limits its own fix run to macOS.
• CI runs on this head were created 2026-10-04T19:40:01Z, after current main's tip timestamp 2026-10-04T18:26:40Z; no timestamp staleness gap found. Those green per-PR checks do not run the full suite or this sanitizer test. No full-suite claim is made by this review.

No GUI session: this change only repairs a test-local lifetime. The optional code-review skill was unavailable; this was a manual review with a sanitizer probe.

Recommendation

Approve. The fix is minimal and the sanitizer mutation demonstrates the failing lifetime and its repair. Normal CODEOWNERS/maintainer review remains the merge gate; I am posting COMMENT rather than an approval event.

— Jeremy KK7GWY + Codex

[ten9876]

Issue fit: yes. At 15affff0, this fixes finding 1 of #6154: the stack-use-after-scope of rec, which the hl2 factory installed in section 5 captures by reference and section 6 still uses. Moving RecorderState rec to main() scope makes it outlive every RecordingOfflineSource the factory can build, because every RadioModel in the file is block-scoped. Refs rather than Fixes is the right verb, since the other #6154 findings are untouched.

Scope

File	Change	Claimed?	Verdict
tests/offline_health_registry_test.cpp:136-139, 202	rec moved from section 5's block to main() scope, with a comment on why	Yes	In scope

Everything in the diff is explained by the stated fix: one authored commit, test-only, no CHANGELOG.md, no new surface, no guard removed. Socket preflight: the target is unchanged in shape. Section 5 injects the recording double precisely so the real Hl2TelemetryPoller never binds, and this PR keeps that.

Blockers

None.

Needs maintainer decision

1. This duplicates 1f9c4ed6 in #6173 (fix/asan-standing-findings, @ten9876). That PR makes the same move of rec to main() scope in tests/offline_health_registry_test.cpp:196-202, with a different comment, so the two conflict on that hunk. The author has already offered (in the thread) to close this one when #6173 lands. Pick one:

   • land #6173 and close this;
   • or land this first and drop the hunk from #6173.

   The code is equivalent either way.

Nits (non-blocking)

None. The earlier suggestion of a file-scope static would add a second lifetime rule for no gain. The lambda retained in the process-global registry holds a dangling reference at exit, but destroying a reference-capturing lambda never dereferences it.

What I tried to break / verified

• Built and ran this head's offline_health_registry_test (Linux, Qt 6.12.0, RelWithDebInfo): passes. I did not rebuild it under ASan. The red/green under ASan rests on three independent sources: the author's macOS run, #6154's Linux lane report naming 'rec' (line 197), and #6173's own red/green. A plain build cannot show the defect; it passed on main too.
• Hidden reader of rec before section 5: checked for one that would now see a pre-used recorder. Sections 1–4 never reference it, and there is one rec symbol in the file with no shadowing. So !rec.destroyed, rec.target.isNull() and the demands delta in section 5 mean what they did.
• Other reference-capturing factories: there are two declare() calls. The flex one in section 6 captures nothing.
• CI is not evidence here: this target is in no ci.yml -R filter. Only the sanitizer lane runs it.
• Merge: clean against current main (85f2a124); it conflicts only with #6173.

Recommendation: no code blocker. The only open question is the merge order against #6173.

— Jeremy KK7GWY + Claude

[ten9876]

Maintainer ruling: #6173 carries this fix (commit 1f9c4ed6, same one-line move on the same lines). Closing this in favour of #6173. Thanks @on8st: you found and fixed this one first, and the lifetime analysis here is what #6173's fix rests on.

— Jeremy KK7GWY + Claude