Repository navigation
fix(tests): tests reach WDSP only through WdspChannel test hooks, and Static checks enforce it. Principle VIII. - #6309
Conversation
The weekly sanitizer lanes build with AETHER_SHARED_CORE=ON. There the core does not re-export WDSP's symbols: aether_wdsp is a PRIVATE dependency built with hidden visibility. wdsp_wbfm_test (added in #5973) calls OpenChannelWithExchangeDepth directly but linked only aethercore, so both the ASan + UBSan and TSan builds stopped at link with an undefined reference. No tests ran. Link aether_wdsp as well, as rtl_wfm_pipeline_test (also from #5973) and rtl_receive_pipeline_test already do. In the default static-core build the archive is already on the link line, so nothing changes there. Fixes #6283 Fixes #6284 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
464c2bb to
d046e57
Compare
ten9876
left a comment
There was a problem hiding this comment.
Issue fit: Yes, for the reported build failure. At d046e57fb, #6283 and #6284 both point to the same sanitizer run that failed before executing tests. I read the actual failed-run logs: both lanes report the two undefined references to OpenChannelWithExchangeDepth at wdsp_wbfm_test.cpp:610 and :615. Adding the direct aether_wdsp dependency fixes that hidden-symbol link failure without changing workflows, sanitizer flags, suppressions, or production code. This does not claim the subsequent whole sanitizer suite will be green.
Scope
| File | Change | Claimed? | Verdict |
|---|---|---|---|
tests/tests.cmake |
Add aether_wdsp to wdsp_wbfm_test and explain why shared core needs it |
Yes | Entirely in scope |
The single signed commit and all four changed lines match the stated fix. No unrelated changes or preference changes; no CHANGELOG, registration, timeout, or frozen-gate changes. The comment describes the current link constraint rather than retelling history.
Blockers
None.
Nits — non-blocking
- Keep the shared-build coverage limitation visible —
tests/wdsp_wbfm_test.cpp:610and:619. I verified that the executable and shared core contain separateOpenChannelWithExchangeDepthandchsymbols. The direct refusals target the executable's copy, whileWdspChannel's allocation/ring observations target the core's copy. The invalid-input return checks remain useful, but the assertion that a refusal preserved the existing live ring cannot prove that in these sanitizer builds. The PR accurately discloses this trade-off, and the same test still observes one state copy in the shipping/static configuration. A later wrapper-level test seam could restore that sanitizer assertion; it need not be bundled into this link repair. - Watch the existing timeout on the next sanitizer run —
tests/tests.cmake:968. The author reports 103.7 seconds under ASan against a 120-second timeout. My optimized, uninstrumented shared-core run took 19.59 seconds and passed, which does not settle hosted-runner ASan/TSan timing. I did not reproduce a timeout, so I am not requesting an arbitrary timeout increase as a blocker.
What I tried to break
- Completed test-boundary preflight against this head's sources.
wdsp_wbfm_testand the four selected sibling targets use numeric DSP input/state injection; no sockets, synthetic external-device peer, live radio or simulator process were required. The PR changes no test source or network behavior. - Built the exact head's
wdsp_wbfm_testin a lean Linux / Qt 6.12 shared-core build, reusing only an owned scratch build cache checked out atd046e57fb. Removing the new direct archive from its generated linker command fails with the same two undefined references at lines 610/615; the unchanged PR link command succeeds. This independently checks the link fix's before/after effect without weakening visibility or adding exports. - Ran the registered socket-free tests serially with
--no-tests=error:wdsp_wbfm_test,wdsp_channel_test,wdsp_allocation_scope_test,rtl_wfm_pipeline_test, andrtl_receive_pipeline_testpassed 5/5, in 96.18 seconds total. This local run was uninstrumented, shared core,-O2 -g1, with ASR/RADE/D-STAR/DFNR disabled; it is not a reproduction of the author's full sanitizer configure. - Configured the shipping/static-core link graph before and after the added dependency: both already contain
libaether_wdsp.aonce. Inspectedchannel.c:127: all direct calls in this fixture supply an invalid depth or channel, and the guard returns beforeopen_channel()touches state. Test registration and the frozen CI test-gate checker pass; the two RTL targets already declare the same explicit dependency. - Did not independently rebuild the entire ASan/TSan tree, rerun the author's 680-test suite or GPU-leak classification, or run native Windows/macOS tests. Those claims remain author-reported. Did not build or drive the GUI: the change is strictly a headless test-link dependency, and the demo bridge cannot verify a linker edge. No app session was launched. A separately model-invocable automated-review skill was unavailable; no prior PR bot review existed when checked.
CI and governance
At posting preparation, Windows and Static checks pass; Linux build and macOS are still running. The shared-core configure check passes, but it only configures and is not evidence that this test linked. Main remains 888ac58bf (2026-10-09T00:35:52Z), earlier than the head's pull-request CI run creation (02:47:46Z), so these runs are not stale relative to current main. GitHub verifies the commit signature; the subject cites Principle VIII. Direct-call tests can explicitly link WDSP as the review build contract specifies. No binding governance rule requires expanding this one-edge repair into a new exported test API.
Recommendation: Approve with nits, pending the remaining required CI checks. The reported link failure was independently reproduced by removing the archive and resolved by the proposed link edge; all five focused tests pass. Merge after required checks pass, then rerun both sanitizer lanes and watch for the disclosed timeout or unrelated failures; the full-suite claims remain separate from this verified link fix. Formal approval remains the maintainer's decision, so this is a COMMENT review.
There was a problem hiding this comment.
Issue fit
Yes. #6283 (ASan+UBSan) and #6284 (TSan) are both Build-failure stickies from run 37796760576 on 14ef3e7 with the same fingerprint b83f0666e782; I read both and confirmed each body carries the "The lane did NOT run its tests: the Build step failed" text, so neither died at Configure — the stale-:tsan-image pattern is not in play and Fixes #6284 holds. The root cause in the body checks out: CMakeLists.txt:1669 is target_link_libraries(aethercore PRIVATE aether::wdsp), third_party/wdsp/CMakeLists.txt:22-30 sets C_VISIBILITY_PRESET hidden, so with a SHARED core (CMakeLists.txt:151-166) the test's direct reference cannot resolve through libaethercore.so, while the static core propagates the archive via $<LINK_ONLY:> and links fine. One-line link fix, correct seam.
No test is needed and none should be asked for: the defect is a link edge, and the lane that exercises it is the sanitizer lane itself. No socket test is added, modified or removed by this PR.
Scope
| File / group | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
tests/tests.cmake:964-966 |
adds aether_wdsp to wdsp_wbfm_test's link line + 2-line comment |
Yes | In scope |
Everything in the diff is explained by the issue. One file, one target, one comment; no workflow, sanitizer flag, suppression, timeout or source change — I verified that against the full patch. No CHANGELOG.md entry (correct). Single signed commit.
Blockers
None.
Nits
-
tests/tests.cmake:966— the deferred timeout question is already answered, and the body's framing of it is wrong. The body says:under ASan,
wdsp_wbfm_testtook 103.7 s against its 120 s TIMEOUT … the lane may hit that timeout next. I have not changed the timeout; that is a call for the maintainer.It isn't, because
TIMEOUT 120is not the effective limit in an instrumented build.tests/tests.cmake:8400setsAETHER_SANITIZER_TIMEOUT_SCALE 4, andtests/tests.cmake:8520-8528multiplies every declared TIMEOUT by it when_aether_tests_instrumentedis ON. That flag is set attests/tests.cmake:8419-8421whenAETHERSDR_SANITIZERis set or-fsanitize=appears in the compile flags — and the comment at:8396-8397names this case explicitly ("or in the global C/C++ flags (how sanitizers.yml builds)").sanitizers.yml:125-128passes the sanitizer flags viaCFLAGS/CXXFLAGSrather than-DAETHERSDR_SANITIZER, which is exactly that branch. So the lane runs this test at 480 s, and your own 103.7 s local run — same-Doptions and CFLAGS, per your validation note — was already measured against 480, not 120. There is ~4.6x of headroom; even a 4x hosted-runner penalty on top of your box lands inside it. No timeout change is needed in this PR or a follow-up, and nothing for a maintainer to rule on. Worth correcting in the body so the next person reading these stickies doesn't go chasing it. -
tests/tests.cmake:964-965vsCMakeLists.txt:155-162— the in-tree note added by #6136 prescribes the other remedy for exactly this situation: "A test that calls WDSP directly while linking only aethercore links against the static core and fails here; go through core/dsp/WdspChannel.h." This PR takes the dual-link route instead, whichrtl_wfm_pipeline_test(:1665) andrtl_receive_pipeline_test(:1671) already established. Neither is canon — I looked and found no CONSTITUTION/AGENTS/CONTRIBUTING sentence on it — so this is a convention collision, not a violation, and the precedent argument is good enough for a link fix. But the two now say different things in the same tree; a sentence onCMakeLists.txt:158acknowledging the dual-link precedent would stop the next contributor from reading that note as a prohibition. -
The disclosed vacuity of the "refused preparation leaves existing ring and allocations intact" assertion (
tests/wdsp_wbfm_test.cpp:619-621) under shared core is real, honestly stated in the body, and still checked in the default static build. Noting it only so it's on the record — not something to fix here.
What I tried to break
- Cross-copy state corruption, the failure mode this fix could plausibly introduce. Dual-linking gives the test executable its own
ch[MAX_CHANNELS](third_party/wdsp/upstream/channel.c:29) distinct from the shared core's, so any direct call touching channel state would act on a different instance thanWdspChanneldoes — silently wrong, not merely vacuous. I extracted all 127 function declarations fromthird_party/wdsp/include/aether_wdsp.hand regex-swepttests/wdsp_wbfm_test.cppfor every one: the only hits areOpenChannelWithExchangeDepthat lines 610 and 615. Both are refusals, andchannel.c:126-131returns 0 on the pure argument check (channel < 0 || channel >= MAX_CHANNELS || exchangeDepth < 2 || exchangeDepth > 8) beforeopen_channel, so neither reachesch[]. Every tested depth (-1, 0, 1, 9, INT_MAX) is outside[2,8]and both ids (-1,32, withMAX_CHANNELS == 32atcomm.h:108) are out of range. The private copy only ever executes the early return — exposure is bounded to the disclosed vacuity. - Sibling targets left unfixed — the same break hiding elsewhere, which would leave the lane red after merge. Only four test sources reach a WDSP header:
wdsp_wbfm_test,wdsp_allocation_scope_test(:1660, linksaether_wdspalready),rtl_receive_pipeline_test(:1671, already), andnnr_controls_test(:999,aethercoreonly) — but that last one's only mention ofaether_wdsp.his in a comment at line 10, and the facade sweep finds no WDSP call in it. Nothing else to fix, which matches yourninja -k 0finding that this was the single failing edge. - A new configure-time ratchet miss from the changed link line.
aether_assert_tests_retrofitted(tests/tests.cmake:8542-8564) requiresAETHER_WDSP_FFTW_TIMELIMIT=/AETHER_WDSP_WISDOM_DIR=in every test's ENVIRONMENT and flagsaethercore + Qt6::Widgetstargets lackingaetherdesktop_support; both gates read properties that this diff doesn't touch, and neither keys offaether_wdsp. Consistent with Static checks and Sanitizer-option-configures going green. - Regression in the default static build — the "nothing changes there" claim.
aethercoreis STATIC by default and carriesaether::wdspas aPRIVATEdep, which CMake still exports as$<LINK_ONLY:aether_wdsp>in the usage requirements, so the archive is already on the link line and naming it again is a no-op. No duplicate-definition risk (same archive, one set of objects). The three release lanes being green (build,check-macos,check-windowsond046e57) is consistent — though note none of them configuresAETHER_SHARED_CORE=ON, so per-PR CI never exercises the thing this PR fixes; the first real proof is the next weekly sanitizer run. - Could not check: anything at runtime. No build, no
ctest, no shared-core link here — every statement above is reasoned from the sources in/tmp/aetherclaude/pr-6309plus the two issue bodies and the check-run list, not reproduced. Your local ASan/TSan evidence I took as reported; I verified only that it is internally consistent with the CMake it describes (and found the 120-vs-480 discrepancy that way).
Recommendation
Approve with nits. The link fix is minimal, correct, and the one place the dual-link duplication could have bitten — direct calls touching real channel state — I swept exhaustively and it does not. Scope is a single line plus a comment that explains why, which is the house style. The only substantive thing to do before merge is drop the timeout caveat from the body: AETHER_SANITIZER_TIMEOUT_SCALE 4 already gives this test 480 s in the lane, so there is no maintainer call pending there and no follow-up needed. Optionally add the one-sentence cross-reference on CMakeLists.txt:158. Then this should go in so the next weekly run can get past Build and show what's actually behind it.
Thanks for the unusually complete writeup — pre-disclosing the shared-core vacuity trade-off and the alternative you rejected (...ForTest() forwarder) is exactly what makes a build-config fix reviewable without a local build, and the ninja -k 0 "only failing edge" evidence saved real work.
🤖 aethersdr-agent · cost: $4.7346 · model: claude-opus-5
…. Principle VIII. The AETHER_SHARED_CORE note in CMakeLists.txt pointed only at WdspChannel, while wdsp_wbfm_test links aether_wdsp itself. The note now names both routes and what the second one costs: the test holds its own copy of WDSP's state. wdsp_wbfm_test's exchangePreparation() says the same at the direct calls: the refusals are proved in every build, but the "existing ring intact" check only observes the copy WdspChannel uses in the static-core build. Comments only; no build or test behaviour changes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Every finding from both reviews (ten9876 and aethersdr-agent, both on
Proof at
The bridge does not apply: these are comment-only changes on top of a link fix. |
…ok, so its ring check is real in the shared-core lanes. Principle VIII. The test called OpenChannelWithExchangeDepth directly. Linking it against aether_wdsp (the earlier commit on this branch) let the shared-core sanitizer lanes build, but the test then drove its own copy of WDSP, so "a refused preparation leaves the existing ring intact" could not see the channel the core had opened and proved nothing there. WdspChannel::openWithExchangeDepthForTest() forwards to the WDSP the core drives, with the prepared-WBFM geometry, next to the other *ForTest hooks. The test now links aethercore only, the same as main. Proof (shared-core ASan, the lane's flags): links without aether_wdsp. Mutating channel.c so a bad depth rebuilds the live channel and still returns 0 now fails "refused preparation leaves existing ring and allocations intact"; under the old double link the same mutation failed no assertion. ASan: wdsp_wbfm_test, wdsp_allocation_scope_test, rtl_wfm_pipeline_test and rtl_receive_pipeline_test pass; wdsp_channel_test passed 2/2 when re-run alone (one real-time sweep check failed at load average ~10 under parallel builds; it does not use this code). TSan (lane options + suppressions): wdsp_wbfm_test passes with no warnings. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…king aether_wdsp. Principle VIII. Every per-PR build uses the static core, so a test that calls WDSP directly while linking only aethercore builds there and breaks only the weekly shared-core sanitizer lanes (#6283/#6284). tools/check_wdsp_test_links.py reads tests/tests.cmake, follows each test target's sources through the repo's quoted includes, and fails a target that includes a WDSP header but does not link aether_wdsp; a WDSP-including test no target names is an error too (fail safe). Stdlib-only, ~0.4 s, so it runs in the required Static checks job instead of a second shared-core build on every PR. Proof: fails on origin/main's state, naming wdsp_wbfm_test; fails when aether_wdsp is removed from rtl_receive_pipeline_test and when a WDSP-including source hides behind a variable; passes on this head (672 targets). Limit: a hand-written extern "C" declaration is not seen. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Maintainer decisions are folded into this PR, so no follow-up issues were filed.
The TSan timeout is unchanged, as decided. The lane limit is 480 s, and the test runs in 187 s locally. |
… probe can never open a channel. Principle VIII. rtl_receive_pipeline_test and rtl_wfm_pipeline_test linked aether_wdsp, so under AETHER_SHARED_CORE each held a second WDSP the core never touches; rtl_receive_pipeline_test's TLS warm-up call landed on that copy. threadAllocationSequenceForTest() reaches the core's, and both tests drop the link. openWithExchangeDepthForTest() takes g_setupMutex and loads wisdom as open() does, and refuses (kOpenProbeWouldOpen, never reaching WDSP) any call WDSP would accept, so it cannot open an orphan channel or rebuild a live one. open(), the hook and validateConfig()'s WBFM clause now share one geometry and one Config-to-OpenChannel helper. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… checker. Principle VIII. tools/check_wdsp_test_links.py now fails a test source that includes a WDSP header, directly or through quoted repo headers, and a test target that links aether_wdsp, bar a commented allowlist (wdsp_allocation_scope_test, a pure WDSP unit test without aethercore). Every file under tests/ is scanned through the same include walk. Only final results are memoised, so an include cycle can no longer cache "no WDSP" for a file that reaches it. tools/test_check_wdsp_test_links.py covers the cycle, a transitive header, the link rule and the allowlist on synthetic trees; it runs as a ctest and as a Static checks step. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Answers to the automated code-review pass on Maintainer decision: fold in the sibling fix (
Checker rule (
1. Hook without 2. Hard-coded WBFM geometry. Fixed in
3. The hook could open an unowned channel or clobber a live one. Fixed in
4. Cycle guard memoised
5. The orphan scan only checked direct includes. Fixed in Refuted (maintainer decision): a shared-core link-only build would be the "real" invariant. That is true and was considered. Jeremy chose the static check after weighing what a second shared-core build costs on every PR. The checker docstring ( Evidence at
A pre-existing flake, not from this PR. |
Summary
Both weekly sanitizer lanes stopped at Build, at link, so no tests ran (#6283 ASan+UBSan, #6284 TSan, fingerprint
b83f0666e782). #5973 addedwdsp_wbfm_test, which callsOpenChannelWithExchangeDepthdirectly while linking onlyaethercore. The lanes build withAETHER_SHARED_CORE=ON, wherelibaethercore.sokeeps WDSP's symbols hidden (a PRIVATE dependency with hidden visibility), so the link failed:Every per-PR build uses the static core, where the test links fine, so nothing caught it before merge.
Linking
aether_wdspinto the test would build, but the test would then drive a second copy of WDSP that the core never touches. Its checks against that copy would prove nothing. Two other tests already did this. So every test now reaches WDSP throughcore/dsp/WdspChannel.h, and Static checks enforce it.Changes
WdspChanneltest hooks:openWithExchangeDepthForTest()runsOpenChannelWithExchangeDepthwith the prepared-WBFM geometry, on the WDSP the core drives. It takesg_setupMutexand loads wisdom in the same order asopen().kOpenProbeWouldOpenand never reaches WDSP, so it cannot open an orphan channel or rebuild a live one.threadAllocationSequenceForTest()reads the core WDSP's per-thread allocation counter.open(), the hook andvalidateConfig()'s WBFM clause share one set of constants, and one helper turns aConfiginto theOpenChannelcall.wdsp_wbfm_testgoes through the hook, and now also checks that a probe WDSP would accept is refused.rtl_receive_pipeline_testwarms the core WDSP's TLS through the new hook.rtl_receive_pipeline_testandrtl_wfm_pipeline_testno longer linkaether_wdsp. All three linkaethercoreonly.tools/check_wdsp_test_links.py, run by the "WDSP test links" step in the required Static checks job:aether_wdsp;tests/, whether or not a target lists it literally;wdsp_allocation_scope_test, a pure WDSP unit test with noaethercore. An entry that linksaethercore, stops including WDSP, or names a missing target is itself a finding;extern "C"declaration, and the other PRIVATE statics (zlibstatic,mspack_static).tools/test_check_wdsp_test_links.pytests the checker on synthetic trees: include cycles, transitive headers, the link rule and the allowlist. It runs as thewdsp_test_links_checkerctest and as a Static checks step.AETHER_SHARED_COREnote inCMakeLists.txtand the/pr-reviewbuild recipe now state the rule.Testing
All runs at this head; sanitizer builds used the shared-core configuration with the lane's flags.
ASAN/UBSAN/LSAN_OPTIONS, serial): all six pass. They arewdsp_wbfm_test,wdsp_channel_test,wdsp_allocation_scope_test,rtl_wfm_pipeline_test,rtl_receive_pipeline_testandwdsp_test_links_checker. No WDSP symbols are linked into the two RTL tests.tests/sanitizers/tsan.supp):wdsp_wbfm_testpasses with no ThreadSanitizer warnings.channel.crebuilds the live channel on a bad depth and still returns 0: fails "refused preparation leaves existing ring and allocations intact".iobuffs.c:732.tests/scan.origin/main(4 findings) and on the previous head of this PR (3). It passes on this head (672 targets).Fixes #6283
Fixes #6284
🤖 Generated with Claude Code