2 Commits

Include renames

Author	SHA1	Message	Date
Jonathan BennettandClaude Opus 5	329b7ea918	Fail the native simulator job on a sanitizer fault, and fix the one it found (#12205) ... * Fail the native simulator job on a sanitizer fault, and fix the one it found The integration step backgrounded meshtasticd and ended in `wait "$PID" 2>/dev/null || true`, which discards the simulator's exit status. The coverage env is built with -fsanitize=address, so a fault there prints a full report and exits non-zero on the way out, after the Python harness has already printed its own success - and the job concluded green with an AddressSanitizer heap-use-after-free sitting in the log. Gate it: capture the simulator's output, read the status `wait` reports, and scan the output for a sanitizer report. The pattern moves to bin/lib/sanitizer.sh so bin/run-tests.sh and the new bin/check-sanitizer-log.sh share one matcher rather than drifting apart; the output is uploaded as an artifact, since a fault on the exit path prints after the last test has passed. Leak detection is off, as in bin/test-config-check.sh - exit_simulator exit()s with the node's globals still live. The fault it had been hiding: OSThread::run() points currentThread at the running thread for the whole of runOnce(), and exit_simulator calls exit(0) from AdminModule while ServerAPI is that thread. ServerAPI is declared `public StreamAPI, private OSThread`, so its Thread base frees ThreadName first and PhoneAPI::~PhoneAPI's own LOG_DEBUG then reads it back through currentThread. Clear the pointer in ~OSThread when it names the thread being destroyed, which covers any thread deleted from inside its own runOnce, not just this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjAvRt6xuztC3hKqWkW9Dw * Detach observers when an Observable is destroyed The new sanitizer gate turned the simulator job red on a second heap-use-after-free on the same exit path, this one in Observable::removeObserver. ~Observer removes itself from every observable it watches, but Observable has no destructor and dies with its watchers still pointing at it. exit_simulator exit()s from AdminModule, and static destruction takes the global xModem - whose packetReady is an Observable that PhoneAPI::handleStartConfig attached to - before the heap-allocated ServerAPI that is watching it. PhoneAPI::close() then calls unobserve(&xModem.packetReady) on the dead observable, and ~Observer would walk it again afterwards. Both halves of the contract, so neither side has to outlive the other: ~Observable drops itself from each watcher's list, and unobserve() checks membership before reading through the pointer it is given. Also take CodeRabbit's grep -m 4 in bin/check-sanitizer-log.sh: with more than four matching lines, head closing the pipe is a SIGPIPE that pipefail reports and set -e exits on, so the script gave 141 where it documents 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjAvRt6xuztC3hKqWkW9Dw --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-10-20 14:07:34 +00:00
Ben MeadorsandClaude Opus 5	83fd62b756	test(native): add 14 suites for routing, persistence, parsing and identity gaps (#11515) ... * test(native): add 14 suites for routing, persistence, parsing and identity gaps Coverage audit of the native test tree; adds the highest-value untested logic as 11 new suites and extends 3 existing ones (200 test functions). New: test_stream_framing, test_nodedb_boot_recovery, test_nodedb_legacy_migration, test_nodedb_v25_roundtrip, test_nodedb_identity_hygiene, test_channel_keys, test_reliable_ack_matrix, test_hop_start_policy, test_routing_response_hops, test_phone_api_config_dump, test_observer. Extended: test_rtc, test_mqtt, test_xmodem. Two source changes the audit produced: - StreamAPI::handleRecStream copied stream->read()'s `cInt < 0` EOF check into the buffer-fed path, where there is no EOF sentinel; with signed char any byte >= 0x80 (START1 is 0x94) aborted the parse. Read the byte as uint8_t directly. Latent on develop (no callers), pinned by test_stream_framing. - Extract the post-decode pre-hop predicate from Router::handleReceived into shouldSkipHandleForPostDecodeHop() (NodeDB.h) so test_hop_start_policy drives the exact expression the router calls. No behavior change. test/state-manifest.tsv declares the suites that construct a NodeDB. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): address review - harden observer dispatch, trim comments Review follow-ups on the coverage-audit suites: - Observable::notifyObservers() erased list nodes while holding an iterator into them, so an observer that unobserves itself from onNotify corrupted the dispatch. Today the only self-detacher (PhoneAPI::onNotify -> checkConnectionTimeout -> close -> unobserve) survives solely because it returns -1 and aborts the chain before the increment; that unwritten contract is now gone. Removal during a dispatch nulls the entry and the outermost notify sweeps afterwards, which keeps self-detach, next-detach and destruction-during-notify all safe without an allocation. Hoisting the next iterator instead would have inverted the hazard and broken the existing next-detach case. Two regression tests added. - Correct the documented caller of shouldSkipHandleForPostDecodeHop: the call is in Router::dispatchReceived, not handleReceived. - Cast hop fields to unsigned at the %u call site in test_hop_start_policy. - Trim the new suites' file headers to the one-or-two-line rule in AGENTS.md. - Rename eight test functions whose names were exactly `test_` + 35 chars: that is the shape of a Lob API key, so trufflehog flagged them as secrets and failed the Trunk CI check. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): revert the observer dispatch change, keep the contract test Backs out the notifyObservers() deferred-removal hardening from the previous commit. It was reviewer-driven scope creep: nothing in the coverage audit needed it, no test required it, and it changes dispatch semantics in a header with ~76 observe() call sites on native verification alone. The hazard it addressed is not reachable today. The only observer that unobserves itself from onNotify is PhoneAPI (onNotify -> checkConnectionTimeout -> close -> unobserve), and it returns -1, which aborts the chain before the iterator is advanced past the erased node. test_self_detach_with_abort_during_notify stays: it passes against the unmodified dispatch and pins that the -1 is load-bearing, so a later cleanup that "simplifies" it away goes red. The unsafe variant (self-detach returning 0) is documented in a comment rather than tested, since asserting it would be asserting UB. * fix(serial): recover the frame behind a stray framing marker A byte that failed the START2 check was discarded rather than re-tested as a possible START1, so 0x94 0x94 0xc3 ... lost the real frame: one corrupted byte on a noisy UART silently dropped the frame behind it. Re-test the byte in place instead. Applied to both copies of the receive state machine. readStream() is the one that matters in the field - it is the serial path every phone client uses - while handleRecStream() still has no callers on develop. Strictly widens what the parser accepts; no frame that parsed before parses differently. test_stream_framing covers it on both receive paths, plus a run of stray markers and a START1-then-unrelated-byte resync. This was originally documented as a known gap in the framing suite. Fixing it instead was NomDeTom's call on review: a passing test asserting the bad behavior is what makes it hard to change later, and it is the same defect shape as the signedness fix three functions away. Also: use Throttle::deadlinePassed() in test_reliable_ack_matrix rather than a bare millis() compare, matching the house deadline rule. * test(native): cover the stray-marker resync on the buffer path too The stray-marker fix went into both copies of the receive state machine, but only test_stray_start1_before_frame_still_delivers drove both. The repeated- marker and unrelated-byte cases drove readStream() alone, so a regression in handleRecStream() would have gone unnoticed by two of the three. Verified load-bearing: reverting only the handleRecStream() half of the fix turns test_repeated_stray_start1_before_frame_still_delivers red on the new assertion. test_start1_then_unrelated_byte_resyncs stays green under that mutation by design - its failing byte is 0x00, where both branches reset to 0 - and covers the other half of the ternary. Also drops the stale header on test_stray_start1_before_frame_still_delivers, which still described the gap as pinned-as-is after the fix landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(native): make the hop-start truth table assert the rows it prints test_truth_table_summary was six TEST_MESSAGE lines and no assertion, so it reported as a case that could not fail - the anti-pattern #11517 names in its unfinished assertion-presence lint, and the one exception to NomDeTom's "no RUN_TEST without an assertion" pass over this PR. The printed row and the checked expectation now come from one struct, so the summary cannot narrate a table the predicates no longer implement. It also covers the consequence columns the per-row tests do not assert together: classifyHopStart, shouldDropPacketForPreHop and shouldSkipHandleForPostDecodeHop for the same packet, with the expectations gated on MESHTASTIC_PREHOP_DROP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-18 12:41:08 +00:00