Skip to content

fix(tests): tests reach WDSP only through WdspChannel test hooks, and Static checks enforce it. Principle VIII. - #6309

Merged
ten9876 merged 6 commits into
mainfrom
fix/6283-sanitizer-wbfm-link
Oct 9, 2026
Merged

ten9876 merged 6 commits into
mainfrom
fix/6283-sanitizer-wbfm-link

Conversation

@ten9876

@ten9876 ten9876 commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Both weekly sanitizer lanes stopped at Build, at link, so no tests ran (#6283 ASan+UBSan, #6284 TSan, fingerprint b83f0666e782). #5973 added wdsp_wbfm_test, which calls OpenChannelWithExchangeDepth directly while linking only aethercore. The lanes build with AETHER_SHARED_CORE=ON, where libaethercore.so keeps WDSP's symbols hidden (a PRIVATE dependency with hidden visibility), so the link failed:

tests/wdsp_wbfm_test.cpp:610: undefined reference to `OpenChannelWithExchangeDepth'

Every per-PR build uses the static core, where the test links fine, so nothing caught it before merge.

Linking aether_wdsp into 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 through core/dsp/WdspChannel.h, and Static checks enforce it.

Changes

  • WdspChannel test hooks:
    • openWithExchangeDepthForTest() runs OpenChannelWithExchangeDepth with the prepared-WBFM geometry, on the WDSP the core drives. It takes g_setupMutex and loads wisdom in the same order as open().
    • It is a refusal probe only. A call WDSP would accept (id 0..31 with depth 2..8) returns kOpenProbeWouldOpen and 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.
  • One geometry: open(), the hook and validateConfig()'s WBFM clause share one set of constants, and one helper turns a Config into the OpenChannel call.
  • Tests:
    • wdsp_wbfm_test goes through the hook, and now also checks that a probe WDSP would accept is refused.
    • rtl_receive_pipeline_test warms the core WDSP's TLS through the new hook.
    • rtl_receive_pipeline_test and rtl_wfm_pipeline_test no longer link aether_wdsp. All three link aethercore only.
  • tools/check_wdsp_test_links.py, run by the "WDSP test links" step in the required Static checks job:
    • fails a test source that includes a WDSP header, directly or through quoted repo headers;
    • fails a test target that links aether_wdsp;
    • scans every file under tests/, whether or not a target lists it literally;
    • allows only a commented allowlist: wdsp_allocation_scope_test, a pure WDSP unit test with no aethercore. An entry that links aethercore, stops including WDSP, or names a missing target is itself a finding;
    • is stdlib-only and takes about 0.4 s. It was chosen instead of a shared-core link build on every PR, on per-PR cost. Its docstring names the known limits: a hand-written extern "C" declaration, and the other PRIVATE statics (zlibstatic, mspack_static).
  • tools/test_check_wdsp_test_links.py tests the checker on synthetic trees: include cycles, transitive headers, the link rule and the allowlist. It runs as the wdsp_test_links_checker ctest and as a Static checks step.
  • The AETHER_SHARED_CORE note in CMakeLists.txt and the /pr-review build 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 (lane ASAN/UBSAN/LSAN_OPTIONS, serial): all six pass. They are wdsp_wbfm_test, wdsp_channel_test, wdsp_allocation_scope_test, rtl_wfm_pipeline_test, rtl_receive_pipeline_test and wdsp_test_links_checker. No WDSP symbols are linked into the two RTL tests.
  • TSan (lane options plus tests/sanitizers/tsan.supp): wdsp_wbfm_test passes with no ThreadSanitizer warnings.
  • Static core, lean RelWithDebInfo: the same six pass.
  • Mutations:
    • channel.c rebuilds the live channel on a bad depth and still returns 0: fails "refused preparation leaves existing ring and allocations intact".
    • The hook's refusal guard removed: fails "test probe refuses a preparation WDSP would accept" 4×, and UBSan reports undefined behaviour in iobuffs.c:732.
    • In the checker, each of these fails the self-test: memoising cut results, dropping the link rule, or dropping the tests/ scan.
  • The checker fails on origin/main (4 findings) and on the previous head of this PR (3). It passes on this head (672 targets).
  • Static checks: every script passes. The engine-boundary, touchpoint-manifest, capability-record and command-plane outputs are identical between merge base and head.

Fixes #6283
Fixes #6284

🤖 Generated with Claude Code

@ten9876
ten9876 marked this pull request as ready for review October 9, 2026 02:46
@ten9876
ten9876 requested a review from a team as a code owner October 9, 2026 02:46
@ten9876 ten9876 self-assigned this Oct 9, 2026
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>
@ten9876
ten9876 force-pushed the fix/6283-sanitizer-wbfm-link branch from 464c2bb to d046e57 Compare October 9, 2026 02:47

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:610 and :619. I verified that the executable and shared core contain separate OpenChannelWithExchangeDepth and ch symbols. The direct refusals target the executable's copy, while WdspChannel'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_test and 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_test in a lean Linux / Qt 6.12 shared-core build, reusing only an owned scratch build cache checked out at d046e57fb. 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, and rtl_receive_pipeline_test passed 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.a once. Inspected channel.c:127: all direct calls in this fixture supply an invalid depth or channel, and the guard returns before open_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.

@ten9876 ten9876 removed their assignment Oct 9, 2026

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_test took 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 120 is not the effective limit in an instrumented build. tests/tests.cmake:8400 sets AETHER_SANITIZER_TIMEOUT_SCALE 4, and tests/tests.cmake:8520-8528 multiplies every declared TIMEOUT by it when _aether_tests_instrumented is ON. That flag is set at tests/tests.cmake:8419-8421 when AETHERSDR_SANITIZER is set or -fsanitize= appears in the compile flags — and the comment at :8396-8397 names this case explicitly ("or in the global C/C++ flags (how sanitizers.yml builds)"). sanitizers.yml:125-128 passes the sanitizer flags via CFLAGS/CXXFLAGS rather 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 -D options 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-965 vs CMakeLists.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, which rtl_wfm_pipeline_test (:1665) and rtl_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 on CMakeLists.txt:158 acknowledging 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 than WdspChannel does — silently wrong, not merely vacuous. I extracted all 127 function declarations from third_party/wdsp/include/aether_wdsp.h and regex-swept tests/wdsp_wbfm_test.cpp for every one: the only hits are OpenChannelWithExchangeDepth at lines 610 and 615. Both are refusals, and channel.c:126-131 returns 0 on the pure argument check (channel < 0 || channel >= MAX_CHANNELS || exchangeDepth < 2 || exchangeDepth > 8) before open_channel, so neither reaches ch[]. Every tested depth (-1, 0, 1, 9, INT_MAX) is outside [2,8] and both ids (-1, 32, with MAX_CHANNELS == 32 at comm.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, links aether_wdsp already), rtl_receive_pipeline_test (:1671, already), and nnr_controls_test (:999, aethercore only) — but that last one's only mention of aether_wdsp.h is in a comment at line 10, and the facade sweep finds no WDSP call in it. Nothing else to fix, which matches your ninja -k 0 finding 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) requires AETHER_WDSP_FFTW_TIMELIMIT=/AETHER_WDSP_WISDOM_DIR= in every test's ENVIRONMENT and flags aethercore + Qt6::Widgets targets lacking aetherdesktop_support; both gates read properties that this diff doesn't touch, and neither keys off aether_wdsp. Consistent with Static checks and Sanitizer-option-configures going green.
  • Regression in the default static build — the "nothing changes there" claim. aethercore is STATIC by default and carries aether::wdsp as a PRIVATE dep, 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-windows on d046e57) is consistent — though note none of them configures AETHER_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-6309 plus 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>
@ten9876
ten9876 requested a review from a team as a code owner October 9, 2026 03:57
@ten9876

ten9876 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Every finding from both reviews (ten9876 and aethersdr-agent, both on d046e57fb) is answered below. One commit was added on top, fb4d79fa9. It changes comments only and is signed and verified. No workflow, timeout, flag or suppression changed.

# Source Finding Disposition
1 ten9876, nit 1 / bot, nit 3 Make the shared-build coverage limitation visible at tests/wdsp_wbfm_test.cpp:610/:619 Fixed in fb4d79fa9. tests/wdsp_wbfm_test.cpp:609-613 now says at the direct calls that under AETHER_SHARED_CORE they reach the test's own WDSP copy. The refusals are still proved, but the "existing ring intact" check only means something in the static-core build. The wrapper-level seam that would make that check real in the sanitizer lanes is not in this PR. Both reviews said it need not be bundled, and whether to track it is left to the maintainer.
2 ten9876, nit 2 / bot, nit 1 103.7 s against "120 s TIMEOUT"; the body's framing was wrong The bot is right, and it is now verified locally. tests/tests.cmake:8400 (AETHER_SANITIZER_TIMEOUT_SCALE 4), :8418-8421 (instrumented when -fsanitize= is in the global flags, which is how sanitizers.yml builds) and :8520-8528 (TIMEOUT × scale). Configure prints Sanitizer-instrumented build: test TIMEOUTs scaled x4 in both trees, and ctest --show-only=json-v1 -R '^wdsp_wbfm_test$' shows TIMEOUT 480.0. An ASan run took 124.10 s and still passed, which is past the nominal 120 s, so the scaled limit is the one in force. PR body corrected to state the 480 s limit and the measured times. No timeout change.
3 bot, nit 2 The CMakeLists.txt:155-162 note prescribes WdspChannel only, which collides with the dual-link route Fixed in fb4d79fa9. CMakeLists.txt:156-160 names both routes. A test that calls WDSP directly goes through core/dsp/WdspChannel.h, or links aether_wdsp itself and then holds its own copy of WDSP's state, which cannot see the channels the core opened. Kept to five lines (AGENTS.md § Comments).
4 my code-review pass Comment hygiene: the first draft named a consumer and ran past five lines; one draft claimed the RTL tests call WDSP directly Fixed before push (amended locally; fb4d79fa9 is the only commit added). nm on rtl_receive_pipeline_test/rtl_wfm_pipeline_test shows no direct WDSP calls. They link the archive but do not call it, so the comment cites no consumer, and the body now says only that they link aether_wdsp the same way.
5 my code-review pass The real fix would be a WdspChannel::…ForTest() forwarder; pre-merge CI never builds shared-core test targets Not done here. These are a design choice and a workflow change, so the maintainer decides. The body already records the forwarder as the alternative not taken.

Proof at fb4d79fa9 (local Arch, Qt 6.12.0, mold, the lane's -D options and CFLAGS/CXXFLAGS/LDFLAGS, AETHER_SHARED_CORE=ON):

  • ninja rebuilt wdsp_wbfm_test wdsp_channel_test wdsp_allocation_scope_test rtl_wfm_pipeline_test rtl_receive_pipeline_test in build-asan (ASan + UBSan, Debug) and build-tsan (TSan, RelWithDebInfo). Both exited 0, and the CMake edit forced a reconfigure that printed scaled x4.
  • Under the lane's ASAN_OPTIONS/UBSAN_OPTIONS/LSAN_OPTIONS, serially, all 5/5 passed: wdsp_wbfm_test 108.48 s, wdsp_channel_test 92.26 s, wdsp_allocation_scope_test 0.01 s, rtl_wfm_pipeline_test 23.44 s, rtl_receive_pipeline_test 9.86 s.
  • Under the lane's TSAN_OPTIONS, wdsp_wbfm_test passed in 168.55 s, with no ThreadSanitizer warning. An earlier run under heavier load took 209.46 s. The box was shared with other builds, at load average 7–15.
  • Static-checks scripts: all 19 in static-checks.yml exit 0 (engine boundary, touchpoint manifest, seam probe table and its self-test, docs reference, log rules, test registration, capability records and parser, command plane, CI test gate, network timeouts, QSettings, theme seed, shader dialects, Qt pin and self-test, build options and self-test). The colour ratchet and a11y do not apply because nothing under src/ changed.

The bridge does not apply: these are comment-only changes on top of a link fix.

ten9876 and others added 2 commits October 8, 2026 21:25
…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>
@ten9876 ten9876 changed the title fix(tests): link wdsp_wbfm_test against aether_wdsp. Principle VIII. fix(tests): wdsp_wbfm_test reaches WDSP through a WdspChannel test hook, and Static checks catch the next one. Principle VIII. Oct 9, 2026
@ten9876

ten9876 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer decisions are folded into this PR, so no follow-up issues were filed.

  • a9dc24a48: the double link is replaced by WdspChannel::openWithExchangeDepthForTest(), so the ring check now proves something under the shared core. The mutation evidence is in the PR body.
  • 64a6c5e1c: a new tools/check_wdsp_test_links.py step in Static checks, so the next test that calls WDSP without linking it fails on the PR that adds it, not in the weekly lane.

The TSan timeout is unchanged, as decided. The lane limit is 480 s, and the test runs in 187 s locally.

ten9876 and others added 2 commits October 8, 2026 22:32
… 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>
@ten9876 ten9876 changed the title fix(tests): wdsp_wbfm_test reaches WDSP through a WdspChannel test hook, and Static checks catch the next one. Principle VIII. fix(tests): tests reach WDSP only through WdspChannel test hooks, and Static checks enforce it. Principle VIII. Oct 9, 2026
@ten9876

ten9876 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Answers to the automated code-review pass on 64a6c5e1c. Two commits on top: 62a50fed1 (code and tests) and 4339ebe94 (checker, self-test and Static checks). Both are signed. Line numbers are at 4339ebe94.

Maintainer decision: fold in the sibling fix (62a50fed1)

  • rtl_receive_pipeline_test and rtl_wfm_pipeline_test linked aether_wdsp, so each held its own WDSP. rtl_receive_pipeline_test.cpp:525 now warms the core's TLS through WdspChannel::threadAllocationSequenceForTest() (WdspChannel.h:489). Both tests now link aethercore only (tests/tests.cmake:1663, :1669).
  • Audit: no other test includes a WDSP header or links aether_wdsp. The one exception is wdsp_allocation_scope_test, a pure WDSP unit test that does not link aethercore; it is the only allowlist entry (tools/check_wdsp_test_links.py:55, with its reason).
  • Proof: in the ASan build, nm shows no wdspPort* symbols in either RTL binary, and build.ninja links no libaether_wdsp into them. Both tests pass under the lane options.

Checker rule (4339ebe94)

  • The rule is now: a test source must not include a WDSP header, directly or through quoted repo headers, and a test target must not link aether_wdsp. Only the allowlist is exempt (:188, :202).
  • The error message points at WdspChannel.h and explains the second-copy hazard. It never suggests adding the link.
  • The allowlist validates itself. An entry that also links aethercore, no longer includes WDSP, or names a missing target is a finding.
  • This makes the link-credit findings (INTERFACE/OBJECT/if(), ${var} links) moot: no link line counts as credit any more. A source listed only through a variable is still scanned, because every file under tests/ is.

1. Hook without g_setupMutex or wisdom. Fixed in 62a50fed1. WdspChannel.cpp:1217-1238 now takes g_setupMutex and calls loadWisdomOnce() before WDSP, in the same order as open().

2. Hard-coded WBFM geometry. Fixed in 62a50fed1.

  • One constexpr set (WdspChannel.cpp:42-60) is used by validateConfig() (:1319-1323), by open() (:1511, depth kPreparedWbfmExchangeDepth) and by the hook.
  • One helper, openWdspChannel() (:239), turns a Config into the OpenChannel/OpenChannelWithExchangeDepth call for both open() and the hook. The mute timings come from Config's defaults rather than literals.

3. The hook could open an unowned channel or clobber a live one. Fixed in 62a50fed1. Jeremy chose a runtime guard over a test-only compile flag.

  • Refusing every owned id would have masked the test: it probes invalid depths on the live channel's id, and that is how its ring check reaches WDSP's own refusal.
  • So the hook refuses any call WDSP would accept (id 0..31 with depth 2..8). Those return kOpenProbeWouldOpen (-1) and never reach WDSP (WdspChannel.cpp:1222-1225, WdspChannel.h:495). Owned and unowned ids are covered alike.
  • wdsp_wbfm_test.cpp:616-624 pins this on the live channel's id and on a free slot, at depths 2 and 8.
  • Proof by mutation: with the guard removed, test probe refuses a preparation WDSP would accept fails 4×, then refused preparation leaves existing ring and allocations intact fails, and UBSan reports undefined behaviour at iobuffs.c:732. That is the hazard this guard closes.

4. Cycle guard memoised None for an unfinished file. Fixed in 4339ebe94.

  • Includes.wdsp_header (tools/check_wdsp_test_links.py:96-130) keeps a separate depth-indexed visiting map. It memoises a "yes", or a "no" whose scan cut no cycle above itself. A provisional "no" is never cached.
  • tools/test_check_wdsp_test_links.py covers this on synthetic trees, 14/14:
    • the cycle case: A.h → B.h → A.h, where A reaches WDSP and a second test includes only B.h;
    • a transitive src/ header;
    • the link rule;
    • the allowlist, in four cases;
    • comment handling, and a ${var} source.
  • static-checks.yml has no generic tools/test_*.py runner. Jeremy approved a dedicated "WDSP test-links checker self-test" step (static-checks.yml:272), and the test is also the wdsp_test_links_checker ctest (tests/tests.cmake:4823).
  • Mutations: restoring the always-memoise guard fails the cycle case. Dropping the link rule fails the link case. Dropping the tests/ scan fails the ${var} and tests/ header cases.

5. The orphan scan only checked direct includes. Fixed in 4339ebe94. Every file under tests/, headers included, goes through the same transitive include walk (:202). The self-test case "a header under tests/ that reaches WDSP" covers it (tests/support/Helper.h → core/Leaf.h → aether_wdsp.h).

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 (:32-37) records this decision. It also names the other PRIVATE statics, zlibstatic and mspack_static, as the same unguarded shape.

Evidence at 4339ebe94

  • ASan+UBSan, shared core, lane ASAN/UBSAN/LSAN_OPTIONS, serial: 6/6 pass. They are wdsp_wbfm_test (103 s), wdsp_channel_test, wdsp_allocation_scope_test, rtl_wfm_pipeline_test, rtl_receive_pipeline_test and wdsp_test_links_checker.
  • TSan (lane TSAN_OPTIONS plus tests/sanitizers/tsan.supp): wdsp_wbfm_test passes with 0 ThreadSanitizer warnings (167 s).
  • Static core, lean build: the same 6/6 pass.
  • channel.c mutation (a bad depth rebuilds the live channel and still returns 0): refused preparation leaves existing ring and allocations intact FAILS. The ring check still bites through the hook.
  • TLS warm-up mutation (deleting rtl_receive_pipeline_test.cpp:525): no test observes it. The test's instrument counts operator new in the callback, and first-touch TLS in a shared object does not go through it. The routing is correct by construction (the old call read the test's own copy), but no assertion proves it.
  • The checker: it fails on origin/main (4 findings) and on 64a6c5e1c (3), and passes on this head (672 targets).
  • Static checks: all 20 scripts pass. check_engine_boundary --strict, gen_touchpoint_manifest --check, check_capability_records --strict and check_command_plane --strict give byte-identical output at merge base 888ac58bf and at head. No ratchet moved.

A pre-existing flake, not from this PR. rtl_receive_pipeline_test fails about a third of the time in a shared-core ASan Debug build. The failures are at measureParkAndResume ("returning capture rebuilds the preserved receiver", then "only a captured receiver emits slice or speaker PCM", :415/:434), which runs before anything changed here. The previous head 64a6c5e1c fails 5/12 the same way, and this head 4/12, so the earlier note that this test passes under ASan was a single good run. It is not in #4703. It is reported to the maintainer rather than filed from here.

@ten9876
ten9876 merged commit fcf9a94 into main Oct 9, 2026
8 checks passed
@ten9876
ten9876 deleted the fix/6283-sanitizer-wbfm-link branch October 9, 2026 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[sanitizer] TSan weekly run failure (fp b83f0666e782) [sanitizer] ASan + UBSan weekly run failure (fp b83f0666e782)

1 participant