From 3d38df6910e41fec7111fd4e8373fc37fe7c3807 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 31 Aug 2026 16:37:43 +0200 Subject: [PATCH 1/8] ci: name a budget overrun as one, and stop stranding shared memory A test step killed by its cap is marked failed by the runner rather than cancelled, and the test that was in flight leaves no result file. `colcon test-result` then reports one erroring test named `.xunit.missing_result` - a test that never failed - among thousands that never ran, while every package queued behind it disappears without trace. Read on its own that is indistinguishable from a single flake, so the budget gets re-run instead of raised. Measured over one month: 17 of 25 killed jobs died on a step cap, and every timeout in this file has been raised at least once. The step's own log is not readable from inside the job, so the overrun is detected by the clock: the start is recorded before the test step and compared against that step's cap afterwards. The same check warns at 80% of the cap, which is the number nobody had when the coverage job reached twelve seconds of margin. It deliberately does not count registered tests against result files - `test_dds_domain_allocation` is registered in every package and writes no xunit, so that comparison reports missing results on a run where everything passed. Container jobs now ask for 1 GB of /dev/shm instead of Docker's default 64 MB. Fast DDS puts a 512 KB segment plus port files there per participant, measured at about 0.65 MB, and a participant killed rather than shut down never gets them back: 15 live participants take 9.6 MB, and 125 killed ones fill 63 MB of 64. Locally, scripts/test.sh now reclaims that memory before a run through `fastdds shm clean`, the vendor's own tool, which decides a segment is stale by taking an exclusive non-blocking flock on its lock file - a lock the kernel drops when a process dies, SIGKILL included. Measured: eight killed participants stranded 5.24 MB across 66 files, all of it reclaimed. --- .github/workflows/ci.yml | 42 ++++++++++++++++++ scripts/ci_test_completeness.sh | 77 +++++++++++++++++++++++++++++++++ scripts/sweep_shm.sh | 62 ++++++++++++++++++++++++++ scripts/test.sh | 6 +++ 4 files changed, 187 insertions(+) create mode 100755 scripts/ci_test_completeness.sh create mode 100755 scripts/sweep_shm.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a791b10d..ead6724dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,13 @@ jobs: os_image: ubuntu:resolute container: image: ${{ matrix.os_image }} + # A container gets 64 MB of /dev/shm by default, which is Docker's number and + # not a considered one. Fast DDS puts a 512 KB segment plus port files there + # per participant - measured at ~0.65 MB - and a participant killed rather + # than shut down never gets its segments back. Measured: 15 live + # participants take 9.6 MB, and 125 killed ones fill 63 MB. 1g is tmpfs, so + # it costs only what is used, against 16 GB of runner memory. + options: --shm-size=1g # Must stay above the build plus the test step's own budget below, or the job cap kills the # run before that step's cap can - which loses the "which step ran long" answer. The build # side of this job is around 18 minutes. @@ -88,6 +95,9 @@ jobs: ccache -s ./scripts/ccache_report.sh "${{ matrix.ros_distro }}" + - name: Record the test step start + run: echo "TEST_STEP_START=$(date +%s)" >> "$GITHUB_ENV" + - name: Run unit and integration tests # The suite keeps outgrowing this budget as packages land, and every overrun so far has # been real work finishing rather than a hang: 15 minutes was raised to 25 when lyrical @@ -116,6 +126,17 @@ jobs: --ctest-args -LE linter \ --event-handlers console_direct+ + - name: Check the run completed, and record the margin + if: always() + run: | + # A step killed by its cap is reported as a failure, not a cancellation, + # and colcon then names whichever test was in flight. Without this, the + # overrun reads as one flaky test out of thousands, and the packages + # queued behind it leave no trace at all. + ./scripts/ci_test_completeness.sh "${TEST_STEP_START}" 45 + df -h /dev/shm + echo "shm segments: $(ls /dev/shm | wc -l)" + - name: Show test results if: always() run: colcon test-result --verbose @@ -159,6 +180,13 @@ jobs: ccache_prefix: ccache-jazzy-test- container: image: ${{ matrix.os_image }} + # A container gets 64 MB of /dev/shm by default, which is Docker's number and + # not a considered one. Fast DDS puts a 512 KB segment plus port files there + # per participant - measured at ~0.65 MB - and a participant killed rather + # than shut down never gets its segments back. Measured: 15 live + # participants take 9.6 MB, and 125 killed ones fill 63 MB. 1g is tmpfs, so + # it costs only what is used, against 16 GB of runner memory. + options: --shm-size=1g timeout-minutes: 90 defaults: run: @@ -270,6 +298,13 @@ jobs: runs-on: ubuntu-latest container: image: ubuntu:noble + # A container gets 64 MB of /dev/shm by default, which is Docker's number and + # not a considered one. Fast DDS puts a 512 KB segment plus port files there + # per participant - measured at ~0.65 MB - and a participant killed rather + # than shut down never gets its segments back. Measured: 15 live + # participants take 9.6 MB, and 125 killed ones fill 63 MB. 1g is tmpfs, so + # it costs only what is used, against 16 GB of runner memory. + options: --shm-size=1g # 90, like build-and-test and graph-watchdog. This job covers both the build # and the test phase, a cold ccache makes the build phase the variable one, # and at 60 it had been finishing in 54 to 56 minutes on main - close enough @@ -389,6 +424,13 @@ jobs: runs-on: ubuntu-latest container: image: ubuntu:noble + # A container gets 64 MB of /dev/shm by default, which is Docker's number and + # not a considered one. Fast DDS puts a 512 KB segment plus port files there + # per participant - measured at ~0.65 MB - and a participant killed rather + # than shut down never gets its segments back. Measured: 15 live + # participants take 9.6 MB, and 125 killed ones fill 63 MB. 1g is tmpfs, so + # it costs only what is used, against 16 GB of runner memory. + options: --shm-size=1g timeout-minutes: 90 defaults: run: diff --git a/scripts/ci_test_completeness.sh b/scripts/ci_test_completeness.sh new file mode 100755 index 000000000..133ce0aec --- /dev/null +++ b/scripts/ci_test_completeness.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# Say plainly when a test step ran out of time, instead of letting it read as a +# test failure. +# +# A step killed by its timeout is marked FAILED by the runner, not cancelled, and +# the in-flight test leaves no result file. `colcon test-result` then reports that +# as one erroring test named `.xunit.missing_result`: a test that never +# failed, among thousands that never ran. On its own that is indistinguishable +# from a single flake, which is how a budget overrun gets filed as a test problem +# while every package queued behind it disappears without trace. +# +# The step's own log is not readable from inside the job, so the overrun is +# detected by the clock instead: the caller records a start timestamp before the +# test step and passes the step's cap here. Elapsed at or above the cap means the +# runner killed it. +# +# What this deliberately does NOT do is count registered tests against result +# files. Some tests legitimately write none - `test_dds_domain_allocation` is +# registered by ros2_medkit_cmake in every package and produces no xunit - so +# that comparison reports missing results on a run where everything passed. +# +# Usage: ci_test_completeness.sh +set -euo pipefail + +START_EPOCH="${1:-}" +CAP_MINUTES="${2:-}" + +if [[ -z "${START_EPOCH}" || -z "${CAP_MINUTES}" ]]; then + echo "usage: ci_test_completeness.sh " >&2 + exit 2 +fi + +elapsed=$(( $(date +%s) - START_EPOCH )) +cap_seconds=$(( CAP_MINUTES * 60 )) +# The runner kills at the cap, and this script starts a moment later, so treat +# anything within a minute of the cap as an overrun rather than demanding it be +# over exactly. +overrun_threshold=$(( cap_seconds - 60 )) + +empty=0 +found=0 +while IFS= read -r xml; do + found=$((found + 1)) + if [[ ! -s "${xml}" ]]; then + echo "::error::empty result file: ${xml}" + empty=$((empty + 1)) + fi +done < <(find build -path '*/test_results/*' -name '*.xml' 2>/dev/null) + +missing=$(find build -path '*/test_results/*' -name '*.missing_result' 2>/dev/null | wc -l) + +printf 'test step: %ds elapsed of a %ds cap (%d%%); results: %d written, %d empty, %d missing_result\n' \ + "${elapsed}" "${cap_seconds}" "$(( elapsed * 100 / cap_seconds ))" "${found}" "${empty}" "${missing}" + +status=0 + +if [[ "${found}" -eq 0 ]]; then + echo "::error::no test result files at all - the run never reached the tests" + status=1 +fi + +if [[ "${empty}" -gt 0 ]]; then + status=1 +fi + +if [[ "${elapsed}" -ge "${overrun_threshold}" ]]; then + echo "::error::the test step reached its ${CAP_MINUTES} minute cap. Any missing_result above belongs to the test that was interrupted, and the packages queued behind it did not run at all. This is a budget overrun, not a test verdict." + status=1 +elif [[ "${elapsed}" -ge $(( cap_seconds * 8 / 10 )) ]]; then + echo "::warning::the test step used $(( elapsed * 100 / cap_seconds ))% of its ${CAP_MINUTES} minute cap - it will start being killed before anyone decides to raise it" +fi + +if [[ "${missing}" -gt 0 && "${elapsed}" -lt "${overrun_threshold}" ]]; then + echo "::warning::${missing} test(s) produced no result file without the step running out of time - investigate the test, not the budget" +fi + +exit "${status}" diff --git a/scripts/sweep_shm.sh b/scripts/sweep_shm.sh new file mode 100755 index 000000000..c91382918 --- /dev/null +++ b/scripts/sweep_shm.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# Reclaim shared-memory segments left behind by DDS participants that were killed +# rather than shut down. +# +# A participant that exits cleanly frees its segments; one that dies on SIGKILL +# does not, and nothing reclaims them afterwards. Measured on Fast DDS: about +# 0.65 MB per participant, and 125 killed participants filled 63 MB of a 64 MB +# /dev/shm. Test runs that kill nodes on purpose - the aggregation suites do - +# accumulate this across every run on a developer machine, and the failure it +# eventually produces looks like anything but a full tmpfs. +# +# The reclaiming is done by `fastdds shm clean`, which is the vendor's own tool +# and ships with every distro we support. It decides a segment is stale by taking +# an exclusive non-blocking flock on the segment's lock file; the kernel drops +# that lock when a process dies, SIGKILL included, so a segment is removed only +# when nothing holds it. Each distro's copy matches its own file naming +# (fastrtps_* on 2.x, fastdds_* on 3.x), so the tool from the sourced +# distribution is the one to call - there is nothing for us to match on. +# +# Refuses to run while ROS processes are alive: the flock test protects live +# segments, but a sweep in the middle of a test run is still a lie about what the +# run measured. +set -euo pipefail + +usage() { + echo "usage: sweep_shm.sh [--force]" >&2 + echo " --force sweep even if ROS processes are running" >&2 +} + +FORCE=0 +case "${1:-}" in + --force) FORCE=1 ;; + "") ;; + *) usage; exit 2 ;; +esac + +shm_bytes() { + du -sb /dev/shm 2>/dev/null | cut -f1 || echo 0 +} + +if [[ "${FORCE}" -eq 0 ]]; then + # Match on the command line rather than the process name: colcon runs as + # python3, and a pattern that also matches this script would match itself. + if pgrep -f '[g]ateway_node|[f]ault_manager_node|[c]test' >/dev/null 2>&1; then + echo "sweep_shm: ROS processes are running - refusing to sweep (use --force if you mean it)" >&2 + exit 1 + fi +fi + +before="$(shm_bytes)" + +if command -v fastdds >/dev/null 2>&1; then + fastdds shm clean || echo "sweep_shm: 'fastdds shm clean' reported a problem, continuing" >&2 +else + echo "sweep_shm: no 'fastdds' on PATH - source a ROS distribution first" >&2 + exit 1 +fi + +after="$(shm_bytes)" +reclaimed=$((before - after)) + +printf 'sweep_shm: /dev/shm %s -> %s bytes (reclaimed %s)\n' "${before}" "${after}" "${reclaimed}" diff --git a/scripts/test.sh b/scripts/test.sh index 7e75f93ff..488a428ec 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -57,6 +57,12 @@ COMMON_ARGS=(--event-handlers console_direct+ --parallel-workers "$(nproc)" --re # being able to ask for on its own. The reasoning lives there. "$HERE/drop_stale_results.sh" build +# Reclaim shared memory stranded by participants that were killed rather than shut +# down - the aggregation suites kill peers on purpose, and nothing gives those +# segments back. Tolerated when it declines: it refuses to sweep while ROS +# processes are alive, and that refusal must not stop a test run. +"$HERE/sweep_shm.sh" || true + # Run tests, capture exit code so we always show results even on failure. set +e case "$PRESET" in From d9fe062a00ee34e3cd059eac6b5b8b368c9d7c28 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 31 Aug 2026 17:09:09 +0200 Subject: [PATCH 2/8] ci: shard the coverage job and merge the tracefiles The coverage job finished 89m48s against its 90 minute cap on 2026-08-27 - twelve seconds - and four of its last five runs on main sat between 86 and 90 minutes. Its test step grew from 19m13s to 51m07s in a month, 166%, while every other test-bearing job stayed flat. The cause is one line: build-and-test and jazzy-test both skip ros2_medkit_graph_watchdog, whose suite is 24 minutes on its own, and coverage skips only ros2_medkit_opcua. It alone kept running the suite the others carved out on 2026-08-24, which is the date its step time steps up. Measured test time per package on one run: integration_tests 1050s, graph_watchdog about 1440s, gateway plus fault_manager 389s, and everything else together 85s. The four shards follow those numbers, and their selections partition the workspace - checked against src/, 19 packages, none uncovered and no name that does not exist. Each shard builds the whole workspace because every package needs its dependencies, and tests only its own selection. Minutes are free on a public repository and wall clock is not, so paying for the build four times to quarter the wall clock is the trade worth making. fail-fast is off: with it, one failing shard cancels the others, and a cancelled job cannot be told apart from one killed by its cap. The completeness gate, the HTML report and the Codecov upload move to a merge job, because a shard's tracefile is legitimately partial and no single shard can answer whether the report still describes the whole workspace. lcov -a sums execution counts for matching files, so a line covered by only one shard is covered in the merged report - verified on two tracefiles where a line uncovered in one and covered in the other merges to covered, and the totals sum. The cap stays at 90 for now. The worst shard should land near 45, but the first run starts from a cold per-shard ccache, so the number to lower it by is one to read from the margin the test step now records, not one to guess. --- .github/workflows/ci.yml | 105 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 100 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ead6724dc..e0245e37c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -420,8 +420,41 @@ jobs: # ctest -LE linter per package, a superset of the selection here - they do # not skip ros2_medkit_opcua - so the asserts are still exercised against # unit and integration tests on every PR. + # + # Sharded because this job reached 89m48s against its 90 minute cap - twelve + # seconds - and its test step grew 166% in a month. It is the only test-bearing + # job that does not skip ros2_medkit_graph_watchdog, so it alone absorbed the + # 24 minute suite the others carved out. Measured test time per package on one + # run: integration_tests 1050s, graph_watchdog ~1440s, gateway + fault_manager + # 389s, everything else 85s. The shards below follow those numbers. + # + # Each shard builds the whole workspace, because every package needs its + # dependencies built; only the test selection differs. Minutes are free on a + # public repository and wall clock is not, so paying for the build four times + # to quarter the wall clock is the right trade here. + name: coverage (${{ matrix.shard }}) if: github.event_name == 'push' && github.ref == 'refs/heads/main' runs-on: ubuntu-latest + strategy: + # Mandatory. With fail-fast a single failing shard cancels the others, and a + # cancelled job is indistinguishable from one killed by its cap. + fail-fast: false + matrix: + include: + - shard: integration + select: --packages-select ros2_medkit_integration_tests + - shard: graph-watchdog + select: --packages-select ros2_medkit_graph_watchdog + - shard: gateway + select: --packages-select ros2_medkit_gateway ros2_medkit_fault_manager + - shard: rest + select: >- + --packages-skip ros2_medkit_opcua ros2_medkit_integration_tests + ros2_medkit_graph_watchdog ros2_medkit_gateway ros2_medkit_fault_manager + # Left at 90 deliberately. The measured worst shard should land near 45, but + # the first run after this change starts from a cold per-shard ccache, so the + # honest sequence is to keep the cap, read the margin the test step now + # records, and lower it once there are numbers rather than expectations. container: image: ubuntu:noble # A container gets 64 MB of /dev/shm by default, which is Docker's number and @@ -460,8 +493,9 @@ jobs: uses: actions/cache@v4 with: path: /root/.cache/ccache - key: ccache-coverage-${{ github.sha }} + key: ccache-coverage-${{ matrix.shard }}-${{ github.sha }} restore-keys: | + ccache-coverage-${{ matrix.shard }}- ccache-coverage- - name: Install dependencies @@ -497,7 +531,7 @@ jobs: # build and could not fail CI on a failure - it read as a gate # without being one. colcon test --return-code-on-test-failure \ - --packages-skip ros2_medkit_opcua \ + ${{ matrix.select }} \ --ctest-args -LE linter \ --event-handlers console_direct+ @@ -538,12 +572,74 @@ jobs: fi lcov --list coverage.info + + # No completeness gate here: a shard only ever exercises its own packages, so + # every shard's tracefile is legitimately partial. The gate runs on the + # merged report in coverage-merge below, which is the first place the + # question "did every package reach the report" can be asked. + - name: Upload this shard's tracefile + uses: actions/upload-artifact@v4 + with: + name: coverage-tracefile-${{ matrix.shard }} + path: coverage.info + retention-days: 1 + + # The shards each measure their own packages, so no single one of them can + # answer "does the report still describe the whole workspace". This job is the + # first place that question exists, so the completeness gate, the HTML report + # and the Codecov upload all live here rather than being repeated four times + # against four partial answers. + coverage-merge: + name: coverage (merge) + if: github.event_name == 'push' && github.ref == 'refs/heads/main' + needs: coverage + runs-on: ubuntu-latest + container: + image: ubuntu:noble + timeout-minutes: 30 + defaults: + run: + shell: bash + + steps: + - name: Install tools + run: | + apt-get update + # gpg is required by the codecov action's dependency check. + apt-get install -y git lcov gpg + + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Download every shard's tracefile + uses: actions/download-artifact@v4 + with: + pattern: coverage-tracefile-* + path: tracefiles + + - name: Merge the tracefiles + run: | + # lcov -a sums execution counts for matching file and test names, which + # is exactly right here: the shards measure disjoint packages, and any + # file two of them both touched should carry the sum of both. + mapfile -t files < <(find tracefiles -name coverage.info | sort) + if [ "${#files[@]}" -eq 0 ]; then + echo "::error::no shard tracefiles were downloaded - the shards produced nothing to merge" + exit 1 + fi + echo "merging ${#files[@]} tracefiles" + args=() + for f in "${files[@]}"; do args+=(-a "$f"); done + lcov "${args[@]}" --output-file coverage.info --ignore-errors empty,corrupt + lcov --list coverage.info genhtml coverage.info --output-directory coverage_html --ignore-errors source # A package that never gets --coverage emits no .gcda and drops out of the # report entirely - out of the numerator and the denominator both, so the - # percentage silently stops describing the workspace. Assert the report - # still covers every package the source tree says holds production C++. + # percentage silently stops describing the workspace. Assert the merged + # report still covers every package the source tree says holds production + # C++. Sharding makes this stricter, not weaker: a shard that silently + # tested nothing now shows up as a package missing from the merge. - name: Verify every C++ package reached the coverage report run: | ./scripts/check_coverage_packages.sh coverage.info \ @@ -556,7 +652,6 @@ jobs: path: coverage_html/ - name: Upload coverage to Codecov - if: github.event_name == 'push' && github.ref == 'refs/heads/main' uses: codecov/codecov-action@v7 with: token: ${{ secrets.CODECOV_TOKEN }} From f566f0053d18069f3f73c84c6bb3accf782732fb Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 31 Aug 2026 20:45:55 +0200 Subject: [PATCH 3/8] ci: install curl in the coverage merge job The codecov action's dependency check fails the step outright when curl is absent, rather than skipping the upload, so the merge job died at the last step with every tracefile already merged and the completeness gate already green. --- .github/workflows/ci.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e0245e37c..6aa70fe5d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -605,8 +605,9 @@ jobs: - name: Install tools run: | apt-get update - # gpg is required by the codecov action's dependency check. - apt-get install -y git lcov gpg + # gpg and curl are both required by the codecov action's dependency + # check, which fails the step rather than skipping the upload. + apt-get install -y git lcov gpg curl - name: Checkout repository uses: actions/checkout@v4 From 7bd176a2927eb2af579cacbee523d9302e0dd5be Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 31 Aug 2026 14:41:58 +0200 Subject: [PATCH 4/8] test(integration): report the stack when a demo node dies on a fatal signal A node killed by SIGSEGV during startup left nothing to read: no output, and no core file, because a container cannot set the host's core_pattern. An exit status alone cannot separate a defect in this repository from one below it, in rclcpp, rmw or the DDS implementation. run_demo_node() now installs a handler for SIGSEGV, SIGBUS and SIGABRT that writes the frames to stderr behind a MEDKIT-CRASH marker and then lets the original signal terminate the process, so the exit status a test asserts on is unchanged. Sanitizer builds keep their own handler: this one stands down there, and the tests assert that case too rather than going quiet under it. The launch-domain probe child spun under a bare rclpy.spin, so CPython re-raised SIGINT and the process died from the signal with its participant never destroyed. A participant that is not destroyed strands its shared-memory segments, measured at roughly 0.5 MB each against the 64 MB a container gets for the whole job. The child now answers both SIGINT and SIGTERM through the same path and shuts down, and the test asserts an exit status of zero instead of accepting any non-crash. CI records /dev/shm and the installed rmw packages after the test step, because neither is recoverable once a run has finished. --- .github/workflows/ci.yml | 8 +- .../test/test_domain_launch.test.py | 31 ++++- .../CMakeLists.txt | 13 ++ src/ros2_medkit_integration_tests/README.md | 24 ++++ .../crash_backtrace.hpp | 130 ++++++++++++++++++ .../demo_node_main.hpp | 7 + .../test/test_crash_backtrace.cpp | 119 ++++++++++++++++ 7 files changed, 327 insertions(+), 5 deletions(-) create mode 100644 src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp create mode 100644 src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6aa70fe5d..c120e01b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,7 +126,7 @@ jobs: --ctest-args -LE linter \ --event-handlers console_direct+ - - name: Check the run completed, and record the margin + - name: Check the run completed, and record the environment if: always() run: | # A step killed by its cap is reported as a failure, not a cancellation, @@ -134,8 +134,14 @@ jobs: # overrun reads as one flaky test out of thousands, and the packages # queued behind it leave no trace at all. ./scripts/ci_test_completeness.sh "${TEST_STEP_START}" 45 + # A DDS participant killed rather than shut down leaves its segments + # behind for the rest of the job, and the installed middleware decides + # which transport those segments belong to. Neither is recoverable from + # a finished run, so both are recorded rather than assumed. df -h /dev/shm echo "shm segments: $(ls /dev/shm | wc -l)" + ls -la /dev/shm | head -20 + dpkg -l | grep -E 'rmw-fastrtps|fastdds|cyclonedds' || true - name: Show test results if: always() diff --git a/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py b/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py index 6ba9fb642..ec615689d 100644 --- a/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py +++ b/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py @@ -43,8 +43,13 @@ TOPIC = '/medkit_launch_domain_probe' +# Spinning under a bare `rclpy.spin` lets CPython re-raise SIGINT and die from +# the signal, which skips the participant's destructor. A DDS participant that +# is never destroyed leaves its shared-memory segments behind, and a CI +# container gets 64 MB of /dev/shm for the whole job, so the child shuts itself +# down instead. CHILD = ( - 'import os, sys, rclpy;' + 'import os, signal, sys, rclpy;' 'from rclpy.node import Node;' 'from std_msgs.msg import String;' "print('CHILD_ROS_DOMAIN_ID=' + str(os.environ.get('ROS_DOMAIN_ID')), flush=True);" @@ -53,7 +58,18 @@ f"pub = node.create_publisher(String, '{TOPIC}', 10);" "msg = String(data='from-the-child');" 'timer = node.create_timer(0.1, lambda: pub.publish(msg));' - 'rclpy.spin(node)' + # launch escalates to SIGTERM when SIGINT is not answered in time, and + # Python's default action for that one is to die on the spot. Routing it + # through the same handler SIGINT uses keeps both paths ending in the + # shutdown below. + 'signal.signal(signal.SIGTERM, signal.default_int_handler)\n' + 'try:\n' + ' rclpy.spin(node)\n' + 'except KeyboardInterrupt:\n' + ' pass\n' + 'finally:\n' + ' node.destroy_node()\n' + ' rclpy.try_shutdown()\n' ) @@ -111,5 +127,12 @@ def test_the_child_node_is_reachable_on_that_domain(self): class TestChildShutdown(unittest.TestCase): def test_the_child_was_stopped(self, proc_info, child): - # SIGINT/SIGTERM during launch shutdown, not a crash. - self.assertIn(proc_info[child].returncode, (0, -2, -15, 130, 143)) + # Zero, not merely "not a crash": the child handles the shutdown signal + # and destroys its participant. Dying from the signal instead would + # strand this participant's shared-memory segments for the rest of the + # job, which is what this assertion exists to catch. + self.assertEqual( + proc_info[child].returncode, + 0, + 'the child did not shut down cleanly, so its DDS participant leaked', + ) diff --git a/src/ros2_medkit_integration_tests/CMakeLists.txt b/src/ros2_medkit_integration_tests/CMakeLists.txt index 168664b7d..f7a2ee95d 100644 --- a/src/ros2_medkit_integration_tests/CMakeLists.txt +++ b/src/ros2_medkit_integration_tests/CMakeLists.txt @@ -206,6 +206,15 @@ if(BUILD_TESTING) set_tests_properties(test_test_utils_constants PROPERTIES LABELS "unit") medkit_test_needs_no_domain(test_test_utils_constants) + # The crash reporter every demo binary installs. Death tests, because the + # behaviour under test only exists in a process that is being killed - a + # handler that is merely installed proves nothing about what a crash leaves + # behind. ROS-free: it creates no node, only signals. + find_package(ament_cmake_gmock REQUIRED) + medkit_add_gmock(test_crash_backtrace test/test_crash_backtrace.cpp) + target_include_directories(test_crash_backtrace PRIVATE ${_demo_include_dir}) + medkit_test_needs_no_domain(test_crash_backtrace) + # Configures every launch-test-registering package this file can reach for # real, twice each (ordinary workspace conditions and system-package # conditions), and compares what each run actually registered. Lives here @@ -439,6 +448,10 @@ if(BUILD_TESTING) endif() endforeach() endif() + + # gtest/gmock vendor sources are built as subdirectory targets and inherit + # this package's promoted warnings, which they do not compile clean under. + ros2_medkit_relax_vendor_warnings() endif() ament_package() diff --git a/src/ros2_medkit_integration_tests/README.md b/src/ros2_medkit_integration_tests/README.md index 65cd04722..902d00ec6 100644 --- a/src/ros2_medkit_integration_tests/README.md +++ b/src/ros2_medkit_integration_tests/README.md @@ -78,6 +78,30 @@ ros2 launch ros2_medkit_integration_tests demo_nodes.launch.py | `long_calibration` | `/powertrain/engine` | Action | Fibonacci-based long-running action | | `dual_calibration` | `/testrig/dual` | Services + Actions | `left/calibrate` and `right/calibrate`, `left/sweep` and `right/sweep` - one provider carrying each operation short name twice, so the ROS path is the only id that separates the copies | +### Crash reports + +Every demo binary goes through `run_demo_node()` in +`include/ros2_medkit_integration_tests/demo_node_main.hpp`, which installs a +fatal-signal handler before anything else runs. A node killed by `SIGSEGV`, +`SIGBUS` or `SIGABRT` writes its stack to stderr, so launch_testing captures it +with the rest of the process output: + +``` +MEDKIT-CRASH signal=SIGSEGV +/path/to/demo_brake_pressure_sensor(+0x129cb) [0x5603199349cb] +/opt/ros//lib/libfastdds.so.3(...) [0x...] +MEDKIT-CRASH end +``` + +Grep a CI log for `MEDKIT-CRASH` to find one. Frames from our own binaries come +back as `binary(+0xOFFSET)` because the release build has no exported symbols; +resolve them with `addr2line -e `. The process still dies from +the original signal, so the exit status a test asserts on is unchanged. + +This exists because a node that dies during startup otherwise leaves nothing at +all: no output, and no core file, since a container cannot set the host's +`core_pattern`. + ## Writing New Tests ### Feature Test Template diff --git a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp new file mode 100644 index 000000000..d2bb72b63 --- /dev/null +++ b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp @@ -0,0 +1,130 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include + +#include +#include + +// A sanitizer owns the fatal signals and reports far more than a bare stack, so +// this file stands down when one is present. GCC and Clang announce that +// differently, and __has_feature has to be probed in its own directive because +// GCC expands it eagerly inside a compound #if. +#if defined(__SANITIZE_ADDRESS__) || defined(__SANITIZE_THREAD__) +#define MEDKIT_CRASH_BACKTRACE_SANITIZED 1 +#elif defined(__has_feature) +#if __has_feature(address_sanitizer) || __has_feature(thread_sanitizer) +#define MEDKIT_CRASH_BACKTRACE_SANITIZED 1 +#endif +#endif + +namespace ros2_medkit_integration_tests { + +/// Marker that prefixes every frame, so one grep separates a crash report from +/// the surrounding node output. +inline constexpr const char kCrashMarker[] = "MEDKIT-CRASH"; + +namespace detail { + +inline constexpr int kMaxFrames = 64; + +/// Storage for the frame addresses. A signal handler must not allocate, so the +/// buffer is reserved up front and reused. +inline void ** crash_frame_buffer() { + static void * frames[kMaxFrames]; + return frames; +} + +inline void write_literal(const char * text) { + const ssize_t written = ::write(STDERR_FILENO, text, ::strlen(text)); + static_cast(written); +} + +/// Writes the marker, the signal number and the backtrace, then returns so the +/// default disposition installed by SA_RESETHAND can terminate the process with +/// the original signal. That keeps the exit status the test harness sees +/// unchanged: a segfault still reports as -11, now with frames attached. +inline void crash_handler(int signum) { + write_literal(kCrashMarker); + switch (signum) { + case SIGSEGV: + write_literal(" signal=SIGSEGV\n"); + break; + case SIGBUS: + write_literal(" signal=SIGBUS\n"); + break; + case SIGABRT: + write_literal(" signal=SIGABRT\n"); + break; + default: + write_literal(" signal=other\n"); + break; + } + + void ** frames = crash_frame_buffer(); + const int depth = ::backtrace(frames, kMaxFrames); + // backtrace_symbols_fd writes through the raw fd and allocates nothing, which + // is what makes it usable here; backtrace_symbols would call malloc. + ::backtrace_symbols_fd(frames, depth, STDERR_FILENO); + write_literal(kCrashMarker); + write_literal(" end\n"); +} + +} // namespace detail + +/// Report the stack on a fatal signal instead of dying silently. +/// +/// A process killed by SIGSEGV during startup leaves nothing behind: no output, +/// no core file (a container cannot set the host's core_pattern), and the test +/// harness reports only the exit status. Without frames there is no way to tell +/// a defect in this repository from one below it, in rclcpp, rmw or the DDS +/// implementation. +/// +/// Call this before any other work in main(). The first `backtrace` call +/// resolves the unwinder's lazy relocations, which does allocate - so it is made +/// here, at install time, and never inside the handler. +inline void install_crash_backtrace() { +#ifdef MEDKIT_CRASH_BACKTRACE_SANITIZED + return; +#else + void ** frames = detail::crash_frame_buffer(); + static_cast(::backtrace(frames, 1)); + + struct sigaction action {}; + action.sa_handler = &detail::crash_handler; + ::sigemptyset(&action.sa_mask); + // SA_RESETHAND restores the default disposition before the handler runs, so + // returning from it re-raises the signal and the process dies the way it + // would have without us. + action.sa_flags = SA_RESETHAND; + + ::sigaction(SIGSEGV, &action, nullptr); + ::sigaction(SIGBUS, &action, nullptr); + ::sigaction(SIGABRT, &action, nullptr); +#endif +} + +/// True when install_crash_backtrace() installs handlers in this build. +inline constexpr bool crash_backtrace_is_active() { +#ifdef MEDKIT_CRASH_BACKTRACE_SANITIZED + return false; +#else + return true; +#endif +} + +} // namespace ros2_medkit_integration_tests diff --git a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/demo_node_main.hpp b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/demo_node_main.hpp index dcbe2cc0c..db2992c96 100644 --- a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/demo_node_main.hpp +++ b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/demo_node_main.hpp @@ -21,6 +21,8 @@ #include +#include "ros2_medkit_integration_tests/crash_backtrace.hpp" + namespace ros2_medkit_integration_tests { /** @@ -67,6 +69,11 @@ namespace ros2_medkit_integration_tests { * freely. */ inline int run_demo_node(int argc, char ** argv, const std::function()> & node_factory) { + // Installed first: a fatal signal anywhere below, including inside the DDS + // stack while the participant is being created, then names its frames instead + // of leaving an exit status and no output. + install_crash_backtrace(); + sigset_t mask; sigset_t old; sigemptyset(&mask); diff --git a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp new file mode 100644 index 000000000..74f6247cb --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp @@ -0,0 +1,119 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include + +#include +#include + +#include "ros2_medkit_integration_tests/crash_backtrace.hpp" + +using ros2_medkit_integration_tests::crash_backtrace_is_active; +using ros2_medkit_integration_tests::install_crash_backtrace; + +namespace { + +constexpr bool kHandlerActive = crash_backtrace_is_active(); + +void crash_by_null_write() { + install_crash_backtrace(); + // Both qualifiers are load-bearing and were picked by measuring, not by + // reasoning: a store through a plain `int * volatile` is deleted at -O2 as an + // erroneous path and the process then does not crash at all. Marking the + // pointee volatile makes the store itself one the compiler must emit, and + // marking the pointer volatile stops it being folded to a known constant. + volatile int * volatile target = nullptr; + *target = 1; +} + +} // namespace + +// The claim under test is not "a handler is installed" but "a process that dies +// on a fatal signal leaves frames behind". Only killing a process proves it, +// which is what a death test does: the body runs in a forked child and the +// assertion matches that child's stderr. +// +// Every case asserts in both build configurations rather than standing down +// under one of them. In a sanitizer build the promise is the opposite one - the +// sanitizer owns the fatal signals and this handler must stay out of its way - +// so the absence of our marker is the thing worth pinning there. Replacing a +// sanitizer's report with a plainer stack is the way this file could do harm, +// and a test that went quiet under sanitizers would be blind to exactly that. +TEST(CrashBacktrace, SegvIsReported) { + if constexpr (kHandlerActive) { + ASSERT_DEATH(crash_by_null_write(), "MEDKIT-CRASH signal=SIGSEGV"); + } else { + ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + } +} + +TEST(CrashBacktrace, SegvReportsResolvableFrames) { + if constexpr (kHandlerActive) { + // The marker alone would be satisfied by an empty stack. What makes a + // report useful is a frame carrying an object and an offset, because that + // pair is what addr2line turns back into a location. Asserting on a symbol + // NAME would pin the wrong thing: a release build without -rdynamic reports + // offsets for this binary's own frames, and the frames worth reading here + // belong to libraries below us anyway. + // + // glibc's spacing between the offset and the address differs by release + // (resolute writes ") [0x", noble writes ")[0x"), so the separator is + // matched loosely rather than pinning one distribution's formatting. + ASSERT_DEATH(crash_by_null_write(), R"(\(\+0x[0-9a-f]+\) ?\[0x[0-9a-f]+\])"); + } else { + EXPECT_FALSE(crash_backtrace_is_active()) << "a sanitizer build must not install the handler"; + ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + } +} + +TEST(CrashBacktrace, AbortIsReportedToo) { + if constexpr (kHandlerActive) { + ASSERT_DEATH( + { + install_crash_backtrace(); + std::abort(); + }, + "MEDKIT-CRASH signal=SIGABRT"); + } else { + ASSERT_DEATH( + { + install_crash_backtrace(); + std::abort(); + }, + ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + } +} + +// Exit status is what ctest and launch_testing report, and a handler that +// swallowed the signal would turn a crash into a clean exit and hide it. +TEST(CrashBacktrace, ProcessStillDiesFromTheOriginalSignal) { + if constexpr (kHandlerActive) { + EXPECT_EXIT(crash_by_null_write(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); + } else { + // A sanitizer reports first and then exits on its own terms, so the death + // itself is what stays assertable here. + EXPECT_DEATH(crash_by_null_write(), ".*"); + } +} + +TEST(CrashBacktrace, InstallingTwiceIsHarmless) { + install_crash_backtrace(); + install_crash_backtrace(); + if constexpr (kHandlerActive) { + ASSERT_DEATH(crash_by_null_write(), "MEDKIT-CRASH signal=SIGSEGV"); + } else { + ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + } +} From 7e5d5fe1b80b9e6a5a113364be9ae3a6be76f5c4 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 1 Sep 2026 12:13:18 +0200 Subject: [PATCH 5/8] fix: correct the overrun guard, the coverage merge and the crash handler **The overrun guard reported false overruns on short caps.** It treated "within a minute of the cap" as an overrun, so a one-minute cap flagged a two-second run, and at a five-minute cap the 80% warning and the overrun error collided on the same second. The runner kills a step AT its cap and this check runs afterwards, so the test is simply elapsed at or past the cap - no subtraction. Its inputs are now validated too: a zero cap divided by zero while still printing an overrun, and a start from another clock read as an enormous one. Verified across nine bands, from a two-second run to a cap reached exactly. **The cap was written in two places.** The step's timeout-minutes and the guard's argument could drift, and either direction is silent: raise the timeout and a long-but-legal run reports an overrun, lower it and a real one reports as an ordinary test failure. Both now read one job-level value. **A failing step cost the environment evidence.** If anything before the tests failed, the recorded start was missing, the guard died on its usage error, and the /dev/shm and middleware census below it never ran. The guard is now skipped with a notice when there is no start, and the census always runs. **A failed shard suppressed the whole merge.** With `needs: coverage` alone, one broken shard skipped the merged report, the completeness gate and the upload - losing the diagnostic exactly when a shard broke. The merge job now always runs, reports what it could merge, refuses to publish a partial report as the workspace figure, and fails on the shard's behalf. **Four ccache entries per commit.** Every shard builds the same workspace with the same flags, so per-shard keys quadrupled the cache against a 10 GB repository limit and would evict the entries keeping builds warm. One key for the matrix. **The crash handler could hang instead of reporting.** Unwinding from a signal handler is not async-signal-safe: backtrace() may need the loader or an allocator lock, and a fault that interrupts either deadlocks the handler - in exactly the startup paths this exists to diagnose, turning a crash into a test that hangs to its timeout with nothing to read. It now sets a five-second alarm first, so a wedged unwinder becomes a dead process with whatever was already written. The handler also blocks the other fatal signals while it runs, because SA_RESETHAND resets only the signal that fired and two threads taking different signals would interleave their reports through one static buffer. strlen is gone from the handler; the lengths come from the literals' types. **Two tests proved less than they claimed.** The frame assertion required a frame with no symbol name, so on a build that exports symbols it was satisfied only by libc's frames while ours went unchecked - measured: 2 matching lines instead of 7. And the repeated-install test installed twice in the parent, while the helper installs again inside the dying child, so it discriminated nothing. Both fixed, and the crash tests still pass on jazzy at -O2, with -rdynamic, under ASan, and in a lyrical container. --- .github/workflows/ci.yml | 45 ++++++++++++++++--- scripts/ci_test_completeness.sh | 36 +++++++++++---- .../crash_backtrace.hpp | 26 +++++++++-- .../test/test_crash_backtrace.cpp | 25 +++++++---- 4 files changed, 108 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c120e01b2..7913d7d82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,6 +30,10 @@ jobs: # run before that step's cap can - which loses the "which step ran long" answer. The build # side of this job is around 18 minutes. timeout-minutes: 90 + env: + # One source for the test step's cap. The step's timeout-minutes and the + # guard that reports the margin both read this. + TEST_STEP_CAP_MINUTES: 45 defaults: run: shell: bash @@ -105,7 +109,9 @@ jobs: # arrived (lyrical 22m50s before them, jazzy 28m15s after). The jazzy job below caps the # whole job instead of this step, so only these two distros can be killed mid-package - # which reads as a test failure and hides every test the kill cut off. - timeout-minutes: 45 + # The cap lives in the job's env, referenced here and by the guard below, + # so raising one raises the other and they cannot drift apart. + timeout-minutes: ${{ env.TEST_STEP_CAP_MINUTES }} env: # FastRTPS 2.6 on Humble has a known use-after-free in the # discovery-teardown path (EDP::unpairWriterProxy) that segfaults @@ -133,7 +139,14 @@ jobs: # and colcon then names whichever test was in flight. Without this, the # overrun reads as one flaky test out of thousands, and the packages # queued behind it leave no trace at all. - ./scripts/ci_test_completeness.sh "${TEST_STEP_START}" 45 + # The start is missing only when an earlier step failed before it ran. + # Say so and carry on to the census, rather than dying on a usage error + # and taking the environment evidence down with it. + if [ -n "${TEST_STEP_START:-}" ]; then + ./scripts/ci_test_completeness.sh "${TEST_STEP_START}" "${TEST_STEP_CAP_MINUTES}" || guard_rc=$? + else + echo "::notice::no test step start was recorded - the run failed before the tests" + fi # A DDS participant killed rather than shut down leaves its segments # behind for the rest of the job, and the installed middleware decides # which transport those segments belong to. Neither is recoverable from @@ -142,6 +155,7 @@ jobs: echo "shm segments: $(ls /dev/shm | wc -l)" ls -la /dev/shm | head -20 dpkg -l | grep -E 'rmw-fastrtps|fastdds|cyclonedds' || true + exit "${guard_rc:-0}" - name: Show test results if: always() @@ -499,9 +513,13 @@ jobs: uses: actions/cache@v4 with: path: /root/.cache/ccache - key: ccache-coverage-${{ matrix.shard }}-${{ github.sha }} + # One entry for the whole matrix, not one per shard: every shard builds + # the same workspace with the same flags, so per-shard entries would + # quadruple this cache against a 10 GB repository limit and evict the + # very entries that keep the builds warm. Concurrent shards racing to + # save the same key is benign - the first wins, the rest log it. + key: ccache-coverage-${{ github.sha }} restore-keys: | - ccache-coverage-${{ matrix.shard }}- ccache-coverage- - name: Install dependencies @@ -597,7 +615,11 @@ jobs: # against four partial answers. coverage-merge: name: coverage (merge) - if: github.event_name == 'push' && github.ref == 'refs/heads/main' + # always(), so a failed shard does not take the merged report, the + # completeness gate and the Codecov upload down with it. Losing the + # diagnostic exactly when a shard broke is the wrong way round; the job + # reports what it merged and then fails on the shard's behalf. + if: always() && github.event_name == 'push' && github.ref == 'refs/heads/main' needs: coverage runs-on: ubuntu-latest container: @@ -623,6 +645,9 @@ jobs: with: pattern: coverage-tracefile-* path: tracefiles + # A shard that failed uploaded nothing; merge what exists rather than + # failing here, so the report still describes the shards that ran. + merge-multiple: false - name: Merge the tracefiles run: | @@ -658,7 +683,17 @@ jobs: name: coverage-report path: coverage_html/ + # Last, so everything above is recorded first. The merged report is still + # worth having when a shard broke - it just does not describe the whole + # workspace, and saying so is this job's business. + - name: Fail if any shard did not succeed + if: needs.coverage.result != 'success' + run: | + echo "::error::a coverage shard did not succeed (matrix result: ${{ needs.coverage.result }}). The merged report above covers only the shards that finished, so its percentage is a floor, not the workspace figure." + exit 1 + - name: Upload coverage to Codecov + if: needs.coverage.result == 'success' uses: codecov/codecov-action@v7 with: token: ${{ secrets.CODECOV_TOKEN }} diff --git a/scripts/ci_test_completeness.sh b/scripts/ci_test_completeness.sh index 133ce0aec..5965b29b8 100755 --- a/scripts/ci_test_completeness.sh +++ b/scripts/ci_test_completeness.sh @@ -25,17 +25,33 @@ set -euo pipefail START_EPOCH="${1:-}" CAP_MINUTES="${2:-}" -if [[ -z "${START_EPOCH}" || -z "${CAP_MINUTES}" ]]; then +usage() { echo "usage: ci_test_completeness.sh " >&2 + echo " both are positive integers; the start is seconds since the epoch, as" >&2 + echo " written by date +%s before the step being measured" >&2 +} + +# Validated rather than trusted: a start value from a different clock, or a cap +# of zero, otherwise turns this from a diagnostic into a source of false +# verdicts - a monotonic start reads as an enormous overrun, and a zero cap +# divides by zero while still printing an overrun. +if [[ ! "${START_EPOCH}" =~ ^[0-9]+$ || ! "${CAP_MINUTES}" =~ ^[0-9]+$ ]]; then + usage + exit 2 +fi +if [[ "${START_EPOCH}" -eq 0 || "${CAP_MINUTES}" -eq 0 ]]; then + usage + exit 2 +fi + +now=$(date +%s) +if [[ "${START_EPOCH}" -gt "${now}" ]]; then + echo "::error::the recorded start (${START_EPOCH}) is in the future - the clock moved, so no margin can be computed" exit 2 fi -elapsed=$(( $(date +%s) - START_EPOCH )) +elapsed=$(( now - START_EPOCH )) cap_seconds=$(( CAP_MINUTES * 60 )) -# The runner kills at the cap, and this script starts a moment later, so treat -# anything within a minute of the cap as an overrun rather than demanding it be -# over exactly. -overrun_threshold=$(( cap_seconds - 60 )) empty=0 found=0 @@ -63,14 +79,18 @@ if [[ "${empty}" -gt 0 ]]; then status=1 fi -if [[ "${elapsed}" -ge "${overrun_threshold}" ]]; then +# The runner kills the step AT its cap and this check runs afterwards, so an +# overrun is simply elapsed at or past the cap. An earlier version subtracted a +# fixed minute, which made every cap below five minutes report an overrun on a +# run that had barely started. +if [[ "${elapsed}" -ge "${cap_seconds}" ]]; then echo "::error::the test step reached its ${CAP_MINUTES} minute cap. Any missing_result above belongs to the test that was interrupted, and the packages queued behind it did not run at all. This is a budget overrun, not a test verdict." status=1 elif [[ "${elapsed}" -ge $(( cap_seconds * 8 / 10 )) ]]; then echo "::warning::the test step used $(( elapsed * 100 / cap_seconds ))% of its ${CAP_MINUTES} minute cap - it will start being killed before anyone decides to raise it" fi -if [[ "${missing}" -gt 0 && "${elapsed}" -lt "${overrun_threshold}" ]]; then +if [[ "${missing}" -gt 0 && "${elapsed}" -lt "${cap_seconds}" ]]; then echo "::warning::${missing} test(s) produced no result file without the step running out of time - investigate the test, not the budget" fi diff --git a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp index d2bb72b63..f131b5df8 100644 --- a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp +++ b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp @@ -15,7 +15,7 @@ #pragma once #include -#include +#include #include #include @@ -49,8 +49,11 @@ inline void ** crash_frame_buffer() { return frames; } -inline void write_literal(const char * text) { - const ssize_t written = ::write(STDERR_FILENO, text, ::strlen(text)); +/// Writes a string literal. The length comes from the type, so nothing in the +/// handler calls strlen - which POSIX does not list as async-signal-safe. +template +inline void write_literal(const char (&text)[N]) { + const ssize_t written = ::write(STDERR_FILENO, text, N - 1); static_cast(written); } @@ -59,6 +62,16 @@ inline void write_literal(const char * text) { /// the original signal. That keeps the exit status the test harness sees /// unchanged: a segfault still reports as -11, now with frames attached. inline void crash_handler(int signum) { + // Unwinding from a signal handler is not async-signal-safe: backtrace() can + // need the loader or an allocator lock, and if the faulting thread already + // held one it deadlocks here - in exactly the startup paths this exists to + // diagnose. alarm() is async-signal-safe and SIGALRM's disposition is the + // default, so a wedged unwinder becomes a dead process after five seconds + // instead of a test that hangs to its timeout with nothing to read. The exit + // status is then SIGALRM rather than the original signal, which is the + // trade: a wrong status beats no output and no status at all. + ::alarm(5); + write_literal(kCrashMarker); switch (signum) { case SIGSEGV: @@ -106,7 +119,14 @@ inline void install_crash_backtrace() { struct sigaction action {}; action.sa_handler = &detail::crash_handler; + // The handler writes into one static frame buffer, and SA_RESETHAND only + // resets the signal that fired. Without this, a second thread taking a + // DIFFERENT fatal signal would re-enter concurrently and interleave the two + // reports. ::sigemptyset(&action.sa_mask); + ::sigaddset(&action.sa_mask, SIGSEGV); + ::sigaddset(&action.sa_mask, SIGBUS); + ::sigaddset(&action.sa_mask, SIGABRT); // SA_RESETHAND restores the default disposition before the handler runs, so // returning from it re-raises the signal and the process dies the way it // would have without us. diff --git a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp index 74f6247cb..cd96aa493 100644 --- a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp +++ b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp @@ -68,10 +68,12 @@ TEST(CrashBacktrace, SegvReportsResolvableFrames) { // offsets for this binary's own frames, and the frames worth reading here // belong to libraries below us anyway. // - // glibc's spacing between the offset and the address differs by release - // (resolute writes ") [0x", noble writes ")[0x"), so the separator is - // matched loosely rather than pinning one distribution's formatting. - ASSERT_DEATH(crash_by_null_write(), R"(\(\+0x[0-9a-f]+\) ?\[0x[0-9a-f]+\])"); + // Two things are matched loosely on purpose. glibc's spacing between the + // offset and the address differs by release (resolute writes ") [0x", + // noble writes ")[0x"), and a frame carries a symbol name before the "+" + // whenever the binary exports its symbols - a link flag away, and not what + // this test is about. + ASSERT_DEATH(crash_by_null_write(), R"(\([^)]*\+0x[0-9a-fA-F]+\) ?\[0x[0-9a-fA-F]+\])"); } else { EXPECT_FALSE(crash_backtrace_is_active()) << "a sanitizer build must not install the handler"; ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); @@ -108,12 +110,19 @@ TEST(CrashBacktrace, ProcessStillDiesFromTheOriginalSignal) { } } +// The crash helper installs the handler itself, so a test that merely calls +// install twice beforehand proves nothing - the third install inside the child +// would carry it. The double install has to happen INSIDE the dying process. TEST(CrashBacktrace, InstallingTwiceIsHarmless) { - install_crash_backtrace(); - install_crash_backtrace(); + const auto crash_after_installing_twice = [] { + install_crash_backtrace(); + install_crash_backtrace(); + volatile int * volatile target = nullptr; + *target = 1; + }; if constexpr (kHandlerActive) { - ASSERT_DEATH(crash_by_null_write(), "MEDKIT-CRASH signal=SIGSEGV"); + EXPECT_EXIT(crash_after_installing_twice(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); } else { - ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + ASSERT_DEATH(crash_after_installing_twice(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); } } From 56834004495ac2d52e9a08e645d69664584e9065 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 1 Sep 2026 12:50:26 +0200 Subject: [PATCH 6/8] fix: repair the missing-result detector and make the crash tests assert what they claim **The missing-result detector never fired.** `missing_result` is not a filename: ament writes it as the `name` attribute of a `` inside the ordinary xunit file, and writes that file before running the command, so it is neither absent nor empty. `find -name '*.missing_result'` therefore returned zero always. Measured across five build trees: zero files named that, while one tree carried two genuine missing results in its XML. The guard now greps the XML, and on that tree reports both - the warning branch had been unreachable since it was written. **Four of the six crash cases never crashed under the sanitizer job.** That job builds `asan,ubsan`, and UBSan diagnoses a store through a null pointer before the store happens, so no signal was raised and the "no marker" assertions passed because nothing faulted rather than because the handler stood down. The crash now targets a non-null unmapped address, which UBSan has nothing to say about. **The sanitizer assertions took their oracle from the code under test.** Both the branch selector and the installer read one macro, so a regression in the detection flipped both and the test stayed green either way. The sanitizer branch now asserts the sanitizer's OWN report is present alongside our marker's absence, which does not depend on that macro. The SIGABRT case keeps the weaker assertion on purpose: a sanitizer does not take SIGABRT by default, so there is no report to require. **The frame assertion no longer proved anything about our frames.** Widening it for symbolised frames left it satisfiable by a single libc line. It is now anchored on this binary's own path, read from /proc/self/exe rather than written down. **The stated reason for the launch-test change was false.** Measured on jazzy, /dev/shm bytes before and after: a bare `rclpy.spin` killed by SIGINT returns to baseline exactly, because CPython finalises the interpreter before re-raising and the participant is destroyed on the way out. Only SIGKILL strands segments, which no handler in that child can affect. The tighter exit status still stands on its own - a fixture that dies from its shutdown signal reports the same status whether it shut down or was cut off - but the shared-memory rationale was wrong and is gone. **The sweep's refusal missed the two commands that run the suite.** `colcon test` and `launch_test` both went unmatched, so a sweep could run in the middle of another worktree's integration run. Verified per command line, with the patterns kept out of the checking harness's own argv - three earlier attempts reported every case as a hit because pgrep was matching the harness. Also: a corrupt tracefile no longer merges silently, the census no longer pipes through `head` under pipefail, shard artifacts live seven days rather than one, a shard whose package name rots now fails instead of testing nothing, and `ament_cmake_gmock` is declared where it is used. --- .github/workflows/ci.yml | 42 ++++++-- CONTRIBUTING.md | 13 +++ scripts/ci_test_completeness.sh | 6 +- scripts/sweep_shm.sh | 23 +++- .../test/test_domain_launch.test.py | 21 ++-- .../crash_backtrace.hpp | 31 +++++- src/ros2_medkit_integration_tests/package.xml | 1 + .../test/test_crash_backtrace.cpp | 101 +++++++++++++++--- 8 files changed, 201 insertions(+), 37 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7913d7d82..caa981dd4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -153,7 +153,10 @@ jobs: # a finished run, so both are recorded rather than assumed. df -h /dev/shm echo "shm segments: $(ls /dev/shm | wc -l)" - ls -la /dev/shm | head -20 + # No `head` in this pipeline: it closes the pipe early, SIGPIPEs `ls`, and + # the step's `pipefail` would then kill the census exactly when there are + # enough segments to be worth listing. + ls -la /dev/shm | tail -n +1 dpkg -l | grep -E 'rmw-fastrtps|fastdds|cyclonedds' || true exit "${guard_rc:-0}" @@ -471,10 +474,13 @@ jobs: select: >- --packages-skip ros2_medkit_opcua ros2_medkit_integration_tests ros2_medkit_graph_watchdog ros2_medkit_gateway ros2_medkit_fault_manager - # Left at 90 deliberately. The measured worst shard should land near 45, but - # the first run after this change starts from a cold per-shard ccache, so the - # honest sequence is to keep the cap, read the margin the test step now - # records, and lower it once there are numbers rather than expectations. + # Left at 90 deliberately. Measured on the first sharded run: the worst shard + # (graph-watchdog) took 3046s, then integration 2663s, gateway 2041s and rest + # 1462s, against 86-90 minutes serial before. Lower the cap once a few runs + # agree, not from one cold-cache sample. Note the margin guard in + # build-and-test does NOT run here: this job caps the job rather than a step, + # and a job-level kill stops every remaining step, `if: always()` included, so + # there is nothing a guard could report. container: image: ubuntu:noble # A container gets 64 MB of /dev/shm by default, which is Docker's number and @@ -559,6 +565,20 @@ jobs: --ctest-args -LE linter \ --event-handlers console_direct+ + # colcon WARNS and exits 0 on a --packages-select name that does not + # exist, so a rename would leave this shard testing nothing while the + # merged report still looked complete - another shard's tests execute + # the same installed code and put it back in the report. Name-aware, + # because a shard selecting two packages still produces results when + # only one of the names rotted. + for pkg in ${{ matrix.select }}; do + case "${pkg}" in --*) continue ;; esac + if [ -z "$(find "build/${pkg}/test_results" -type f -name '*.xml' 2>/dev/null | head -n 1)" ]; then + echo "::error::shard '${{ matrix.shard }}' selected ${pkg} but it produced no test results - the name may have rotted" + exit 1 + fi + done + - name: Generate coverage report run: | # --parallel: this single lcov --capture was measured at >99% of the @@ -606,7 +626,10 @@ jobs: with: name: coverage-tracefile-${{ matrix.shard }} path: coverage.info - retention-days: 1 + # Long enough to answer a coverage question raised after the weekend: + # merging again needs these, and at one day a Monday question could not be + # answered from artifacts at all. + retention-days: 7 # The shards each measure their own packages, so no single one of them can # answer "does the report still describe the whole workspace". This job is the @@ -662,7 +685,12 @@ jobs: echo "merging ${#files[@]} tracefiles" args=() for f in "${files[@]}"; do args+=(-a "$f"); done - lcov "${args[@]}" --output-file coverage.info --ignore-errors empty,corrupt + # `empty` only: a shard may legitimately measure little. `corrupt` was also + # here and had to go - a truncated tracefile would have been merged with + # its bad records skipped and no non-zero exit, so every package would + # still be present, the gate would pass, and Codecov would quietly get a + # lower number. + lcov "${args[@]}" --output-file coverage.info --ignore-errors empty lcov --list coverage.info genhtml coverage.info --output-directory coverage_html --ignore-errors source diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e52773219..3c098fa78 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -89,6 +89,14 @@ is the tally of the run you just started and not of everything tested since the rather than matching a name. Run it on its own with `./scripts/drop_stale_results.sh` if you invoke `colcon test` directly. +Every run also reclaims shared memory left behind by DDS participants that were killed +rather than shut down. The aggregation suites kill peers on purpose, and nothing gives +those segments back: about 0.65 MB each, and a container's `/dev/shm` is 64 MB by default, +so they accumulate across runs until something unrelated fails for lack of space. The +reclaiming is `fastdds shm clean`, the vendor's own tool, which removes a segment only when +no process holds its lock. Run it on its own with `./scripts/sweep_shm.sh`; it refuses +while ROS processes are alive, and `--force` overrides that. + #### Pre-commit and Pre-push Hooks ```bash @@ -174,6 +182,11 @@ genhtml coverage.info --output-directory coverage_html --ignore-errors source ./scripts/check_coverage_packages.sh coverage.info --skip ros2_medkit_opcua ``` +In CI this gate runs in the `coverage (merge)` job rather than alongside the tests: the +coverage run is sharded by package, so each shard's tracefile is legitimately partial and +the merged report is the first place the question "did every package reach the report" +can be asked. + Open `coverage_html/index.html` in your browser. `ros2_medkit_opcua` is skipped because it pulls `open62541pp` over the network diff --git a/scripts/ci_test_completeness.sh b/scripts/ci_test_completeness.sh index 5965b29b8..7b42b3d26 100755 --- a/scripts/ci_test_completeness.sh +++ b/scripts/ci_test_completeness.sh @@ -63,7 +63,11 @@ while IFS= read -r xml; do fi done < <(find build -path '*/test_results/*' -name '*.xml' 2>/dev/null) -missing=$(find build -path '*/test_results/*' -name '*.missing_result' 2>/dev/null | wc -l) +# `missing_result` is not a filename. ament writes it as the name attribute of a +# INSIDE the ordinary xunit file, before running the command, so the +# file exists and is not empty. Measured: across five build trees, zero files are +# named that, while one tree carried two genuine missing results in its XML. +missing=$(grep -l 'missing_result' -r build --include='*.xml' 2>/dev/null | wc -l) printf 'test step: %ds elapsed of a %ds cap (%d%%); results: %d written, %d empty, %d missing_result\n' \ "${elapsed}" "${cap_seconds}" "$(( elapsed * 100 / cap_seconds ))" "${found}" "${empty}" "${missing}" diff --git a/scripts/sweep_shm.sh b/scripts/sweep_shm.sh index c91382918..b59ff1896 100755 --- a/scripts/sweep_shm.sh +++ b/scripts/sweep_shm.sh @@ -35,13 +35,24 @@ case "${1:-}" in esac shm_bytes() { - du -sb /dev/shm 2>/dev/null | cut -f1 || echo 0 + # One value or nothing. An earlier form piped du into cut with `|| echo 0`, + # which under pipefail could emit cut's partial output AND the fallback, so the + # arithmetic below got a two-line operand and died with a bad math expression. + local out + if ! out=$(du -sb /dev/shm 2>/dev/null); then + echo unknown + return + fi + printf '%s' "${out}" | head -n 1 | cut -f1 } if [[ "${FORCE}" -eq 0 ]]; then # Match on the command line rather than the process name: colcon runs as # python3, and a pattern that also matches this script would match itself. - if pgrep -f '[g]ateway_node|[f]ault_manager_node|[c]test' >/dev/null 2>&1; then + # demo_ covers the launch fixtures the documented demo workflow starts, which + # the earlier pattern missed entirely - a developer running only demo nodes got + # a sweep in the middle of live work. + if pgrep -f '[g]ateway_node|[f]ault_manager_node|[d]emo_|[c]test|[l]aunch_test|[c]olcon test' >/dev/null 2>&1; then echo "sweep_shm: ROS processes are running - refusing to sweep (use --force if you mean it)" >&2 exit 1 fi @@ -57,6 +68,10 @@ else fi after="$(shm_bytes)" -reclaimed=$((before - after)) -printf 'sweep_shm: /dev/shm %s -> %s bytes (reclaimed %s)\n' "${before}" "${after}" "${reclaimed}" +if [[ "${before}" == unknown || "${after}" == unknown ]]; then + echo "sweep_shm: cleaned, but /dev/shm could not be measured (du declined)" >&2 + exit 0 +fi + +printf 'sweep_shm: /dev/shm %s -> %s bytes (reclaimed %s)\n' "${before}" "${after}" "$((before - after))" diff --git a/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py b/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py index ec615689d..1cef4214a 100644 --- a/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py +++ b/src/ros2_medkit_fault_reporter/test/test_domain_launch.test.py @@ -43,11 +43,15 @@ TOPIC = '/medkit_launch_domain_probe' -# Spinning under a bare `rclpy.spin` lets CPython re-raise SIGINT and die from -# the signal, which skips the participant's destructor. A DDS participant that -# is never destroyed leaves its shared-memory segments behind, and a CI -# container gets 64 MB of /dev/shm for the whole job, so the child shuts itself -# down instead. +# The child answers the shutdown signal instead of dying from it, so its exit +# status is a fact the test can pin rather than a range to tolerate. +# +# What this does NOT buy, despite the obvious guess: it strands no shared memory +# either way. Measured on jazzy, /dev/shm bytes before and after - a bare +# `rclpy.spin` killed by SIGINT returns to baseline exactly, because CPython +# finalises the interpreter before re-raising, and the participant is destroyed +# on the way out. Only SIGKILL strands segments (+0.65 MB, measured), and no +# handler in this child can affect that. CHILD = ( 'import os, signal, sys, rclpy;' 'from rclpy.node import Node;' @@ -127,10 +131,9 @@ def test_the_child_node_is_reachable_on_that_domain(self): class TestChildShutdown(unittest.TestCase): def test_the_child_was_stopped(self, proc_info, child): - # Zero, not merely "not a crash": the child handles the shutdown signal - # and destroys its participant. Dying from the signal instead would - # strand this participant's shared-memory segments for the rest of the - # job, which is what this assertion exists to catch. + # Zero, not merely "not a crash". A fixture that dies from its shutdown + # signal reports the same status whether it shut down or was cut off + # mid-flight, so the old range accepted both. Zero distinguishes them. self.assertEqual( proc_info[child].returncode, 0, diff --git a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp index f131b5df8..9252342a2 100644 --- a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp +++ b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp @@ -49,6 +49,23 @@ inline void ** crash_frame_buffer() { return frames; } +/// A stack of its own for the handler to run on. +/// +/// The commonest silent SIGSEGV is a stack overflow, and that is exactly the one +/// a handler on the ordinary stack cannot report: it needs stack to run, there is +/// none, so it faults again and - the disposition already reset - the process +/// dies having written nothing. Measured before this existed: a null dereference +/// produced 452 bytes and two markers, a stack overflow produced zero of both. +/// +/// SIGSTKSZ is not a compile-time constant on current glibc, so the size is +/// fixed here; 64 KB is far more than backtrace_symbols_fd needs. +inline constexpr std::size_t kAltStackBytes = 64 * 1024; + +inline char * crash_alt_stack() { + static char storage[kAltStackBytes]; + return storage; +} + /// Writes a string literal. The length comes from the type, so nothing in the /// handler calls strlen - which POSIX does not list as async-signal-safe. template @@ -117,6 +134,16 @@ inline void install_crash_backtrace() { void ** frames = detail::crash_frame_buffer(); static_cast(::backtrace(frames, 1)); + // sigaltstack is per-thread, and this runs on the thread that calls it - the + // main thread, where a node is constructed and where the startup crashes this + // exists for happen. A stack overflow on a DDS thread is still silent; that is + // a real limit, not an oversight. + stack_t alt{}; + alt.ss_sp = detail::crash_alt_stack(); + alt.ss_size = detail::kAltStackBytes; + alt.ss_flags = 0; + ::sigaltstack(&alt, nullptr); + struct sigaction action {}; action.sa_handler = &detail::crash_handler; // The handler writes into one static frame buffer, and SA_RESETHAND only @@ -130,7 +157,9 @@ inline void install_crash_backtrace() { // SA_RESETHAND restores the default disposition before the handler runs, so // returning from it re-raises the signal and the process dies the way it // would have without us. - action.sa_flags = SA_RESETHAND; + // SA_ONSTACK puts the handler on the alternate stack above, which is what lets + // it run at all when the ordinary stack is the thing that overflowed. + action.sa_flags = SA_RESETHAND | SA_ONSTACK; ::sigaction(SIGSEGV, &action, nullptr); ::sigaction(SIGBUS, &action, nullptr); diff --git a/src/ros2_medkit_integration_tests/package.xml b/src/ros2_medkit_integration_tests/package.xml index 6abce50ed..3ae4e8ae0 100644 --- a/src/ros2_medkit_integration_tests/package.xml +++ b/src/ros2_medkit_integration_tests/package.xml @@ -27,6 +27,7 @@ ament_cmake_pytest + ament_cmake_gmock python3-pytest launch_testing_ament_cmake launch_testing diff --git a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp index cd96aa493..bf7c300f3 100644 --- a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp +++ b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp @@ -12,8 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include +#include + +#include #include #include @@ -27,14 +31,53 @@ namespace { constexpr bool kHandlerActive = crash_backtrace_is_active(); -void crash_by_null_write() { +// Recursion the optimiser cannot flatten into a loop. Measured across -O0 to +// -O3: without the escaping address GCC turns this into a loop at -O2 and the +// process spins forever instead of overflowing, which made an earlier version of +// the test below hang rather than fail. +volatile char * stack_probe_sink = nullptr; + +__attribute__((noinline)) int recurse_until_the_stack_runs_out(int x) { + volatile char pad[4096]; + pad[0] = static_cast(x); + stack_probe_sink = pad; + // The barrier is what makes this recurse rather than loop. `noinline` and an + // escaping address are not enough inside a translation unit where the + // function has internal linkage: measured, GCC still turned it into a loop at + // -O2 and the process spun instead of overflowing. + asm volatile("" : : "r"(pad) : "memory"); + return recurse_until_the_stack_runs_out(x + pad[0]) + 1; +} + +/// Faults on an unmapped address that is NOT null. +/// +/// Null would be simpler, and was what this did first, but UBSan diagnoses a +/// store through a null pointer before the store happens - so under the ASan job, +/// which builds `asan,ubsan`, no signal was ever raised and four of the cases +/// below passed their "no marker" assertion because nothing crashed rather than +/// because the handler stood down. A non-null unmapped address gives UBSan +/// nothing to object to and still segfaults. +/// +/// Both volatile qualifiers are load-bearing and were picked by measuring: a +/// store through a plain `T * volatile` is deleted at -O2 as an erroneous path +/// and the process then does not crash at all. +/// A regex matching a backtrace frame that belongs to this test binary. +/// +/// The object and offset pair is what addr2line turns back into a location, and +/// glibc's spacing between the offset and the address differs by release +/// (resolute writes ") [0x", noble writes ")[0x"), so the separator is loose. A +/// frame carries a symbol name before the "+" whenever the binary exports its +/// symbols, which CMake does by default, so that half is loose too. +std::string own_binary_frame_pattern() { + char exe[PATH_MAX] = {}; + const ssize_t len = ::readlink("/proc/self/exe", exe, sizeof(exe) - 1); + const std::string self = len > 0 ? std::string(exe, static_cast(len)) : std::string(); + return self + R"(\([^)]*\+0x[0-9a-fA-F]+\) ?\[0x[0-9a-fA-F]+\])"; +} + +void crash_by_unmapped_write() { install_crash_backtrace(); - // Both qualifiers are load-bearing and were picked by measuring, not by - // reasoning: a store through a plain `int * volatile` is deleted at -O2 as an - // erroneous path and the process then does not crash at all. Marking the - // pointee volatile makes the store itself one the compiler must emit, and - // marking the pointer volatile stops it being folded to a known constant. - volatile int * volatile target = nullptr; + volatile int * volatile target = reinterpret_cast(0x1000); *target = 1; } @@ -53,9 +96,10 @@ void crash_by_null_write() { // and a test that went quiet under sanitizers would be blind to exactly that. TEST(CrashBacktrace, SegvIsReported) { if constexpr (kHandlerActive) { - ASSERT_DEATH(crash_by_null_write(), "MEDKIT-CRASH signal=SIGSEGV"); + ASSERT_DEATH(crash_by_unmapped_write(), "MEDKIT-CRASH signal=SIGSEGV"); } else { - ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + ASSERT_DEATH(crash_by_unmapped_write(), ::testing::AllOf(::testing::HasSubstr("Sanitizer"), + ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); } } @@ -73,10 +117,32 @@ TEST(CrashBacktrace, SegvReportsResolvableFrames) { // noble writes ")[0x"), and a frame carries a symbol name before the "+" // whenever the binary exports its symbols - a link flag away, and not what // this test is about. - ASSERT_DEATH(crash_by_null_write(), R"(\([^)]*\+0x[0-9a-fA-F]+\) ?\[0x[0-9a-fA-F]+\])"); + // Anchored on THIS binary's own path, read at runtime rather than written + // down: a bare offset pattern is satisfied by any frame, and libc's frames + // alone would pass it while saying nothing about whether our own frames came + // back resolvable. The path is not a name pin - it is whatever the test was + // built as. + ASSERT_DEATH(crash_by_unmapped_write(), own_binary_frame_pattern()); + } else { + ASSERT_DEATH(crash_by_unmapped_write(), ::testing::AllOf(::testing::HasSubstr("Sanitizer"), + ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); + } +} + +// A stack overflow is the commonest silent SIGSEGV, and it is the one a handler +// on the ordinary stack cannot report - it needs stack to run and there is none. +// This case shipped broken until it was measured: the null dereference above +// produced 452 bytes, and an overflow produced zero. +TEST(CrashBacktrace, SegvOnAnOverflowedStackIsStillReported) { + const auto overflow_the_stack = [] { + install_crash_backtrace(); + static_cast(recurse_until_the_stack_runs_out(1)); + }; + if constexpr (kHandlerActive) { + ASSERT_DEATH(overflow_the_stack(), "MEDKIT-CRASH signal=SIGSEGV"); } else { - EXPECT_FALSE(crash_backtrace_is_active()) << "a sanitizer build must not install the handler"; - ASSERT_DEATH(crash_by_null_write(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + ASSERT_DEATH(overflow_the_stack(), ::testing::AllOf(::testing::HasSubstr("Sanitizer"), + ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); } } @@ -89,6 +155,9 @@ TEST(CrashBacktrace, AbortIsReportedToo) { }, "MEDKIT-CRASH signal=SIGABRT"); } else { + // Only the absence of our marker here, unlike the SEGV cases: a sanitizer + // does not take SIGABRT by default (ASan's handle_abort is off), so there is + // no sanitizer report to require - and none for us to have clobbered. ASSERT_DEATH( { install_crash_backtrace(); @@ -102,11 +171,11 @@ TEST(CrashBacktrace, AbortIsReportedToo) { // swallowed the signal would turn a crash into a clean exit and hide it. TEST(CrashBacktrace, ProcessStillDiesFromTheOriginalSignal) { if constexpr (kHandlerActive) { - EXPECT_EXIT(crash_by_null_write(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); + EXPECT_EXIT(crash_by_unmapped_write(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); } else { // A sanitizer reports first and then exits on its own terms, so the death // itself is what stays assertable here. - EXPECT_DEATH(crash_by_null_write(), ".*"); + EXPECT_DEATH(crash_by_unmapped_write(), ".*"); } } @@ -123,6 +192,8 @@ TEST(CrashBacktrace, InstallingTwiceIsHarmless) { if constexpr (kHandlerActive) { EXPECT_EXIT(crash_after_installing_twice(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); } else { - ASSERT_DEATH(crash_after_installing_twice(), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); + ASSERT_DEATH( + crash_after_installing_twice(), + ::testing::AllOf(::testing::HasSubstr("Sanitizer"), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); } } From 22514bafa5950a0fcb828c4fe589c60ac3a14c6f Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 1 Sep 2026 13:31:06 +0200 Subject: [PATCH 7/8] fix: two regressions this branch put into CI, both mine **The workflow would not start.** `timeout-minutes: ${{ env.TEST_STEP_CAP_MINUTES }}` fails validation before any step runs - an expression yields a string and that key demands a number, so the run died with "The template is not valid ... Unexpected value '45'". `fromJSON` returns a number and is the documented way to spend an expression on a numeric key. The single source for the cap was worth having; the form it was written in was not checkable locally, and it took a red run to find out. **The completeness guard died silently on every healthy run.** The missing-result detector was changed to grep the XML - correct, since `missing_result` is a testcase name and never a filename - but `grep` exits 1 when it matches nothing, which is the healthy case, and under `set -e` with `pipefail` that killed the script before it printed anything. Measured: on a tree with no missing results it exited 1 with no output; on a tree with two it worked. So the guard failed exactly the runs it had nothing to say about, which is the worst possible polarity for a diagnostic. `|| true` now sits inside the braces, where the pipeline can see it. The lyrical job that surfaced both ran 5382 tests with zero failures. Nothing in the suite was broken; the job was red because of the two lines above. --- .github/workflows/ci.yml | 5 ++++- scripts/ci_test_completeness.sh | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index caa981dd4..c560968a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -111,7 +111,10 @@ jobs: # which reads as a test failure and hides every test the kill cut off. # The cap lives in the job's env, referenced here and by the guard below, # so raising one raises the other and they cannot drift apart. - timeout-minutes: ${{ env.TEST_STEP_CAP_MINUTES }} + # fromJSON, because an expression yields a string and this key demands a + # number: `${{ env.X }}` alone fails the workflow with "Unexpected value + # '45'" before any step runs. + timeout-minutes: ${{ fromJSON(env.TEST_STEP_CAP_MINUTES) }} env: # FastRTPS 2.6 on Humble has a known use-after-free in the # discovery-teardown path (EDP::unpairWriterProxy) that segfaults diff --git a/scripts/ci_test_completeness.sh b/scripts/ci_test_completeness.sh index 7b42b3d26..b4e46fa03 100755 --- a/scripts/ci_test_completeness.sh +++ b/scripts/ci_test_completeness.sh @@ -67,7 +67,11 @@ done < <(find build -path '*/test_results/*' -name '*.xml' 2>/dev/null) # INSIDE the ordinary xunit file, before running the command, so the # file exists and is not empty. Measured: across five build trees, zero files are # named that, while one tree carried two genuine missing results in its XML. -missing=$(grep -l 'missing_result' -r build --include='*.xml' 2>/dev/null | wc -l) +# `|| true` inside the braces, not after the pipe: grep exits 1 when it matches +# nothing, which is the healthy case, and under `set -e` with `pipefail` that +# killed this script before it printed anything at all - so a clean run reported +# nothing and failed the step. +missing=$( { grep -l 'missing_result' -r build --include='*.xml' 2>/dev/null || true; } | wc -l) printf 'test step: %ds elapsed of a %ds cap (%d%%); results: %d written, %d empty, %d missing_result\n' \ "${elapsed}" "${cap_seconds}" "$(( elapsed * 100 / cap_seconds ))" "${found}" "${empty}" "${missing}" From 78da68114be75644715e19481020f4cbc09db5b6 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 15:05:51 +0200 Subject: [PATCH 8/8] fix: keep the catch-all shard green and report a crash from every demo binary **The shard assertion would have failed main on the first push.** It walked every word of `matrix.select`, and the catch-all shard's is `--packages-skip ros2_medkit_opcua ...` - so the first name it checked was a package that job never builds. Coverage jobs do not run on pull requests, so this would have surfaced only after merge. The check now runs for `--packages-select` alone. **A failed shard hid its own cause.** The completeness gate and the HTML upload in the merge job had no `if: always()`, so a failing shard made the gate report a missing package and skipped both the report and the step that names the real reason. All three now run regardless. **The sanitizer stand-down is not tested, and the tests no longer claim it is.** Sanitizers are opt-in per package through `include(ROS2MedkitSanitizers)`; seven packages do it and this one does not, so `__SANITIZE_ADDRESS__` is never defined here and every `else` branch was unreachable. The stand-down stays in the header for the day this package opts in; the dead branches are gone. **`alarm(5)` changed the exit status it promised not to.** For a signal delivered by `raise`, `kill` or `pthread_kill` there is no faulting instruction to retry, so returning from the handler resumed the process and the armed alarm killed it five seconds later. Measured: `raise(SIGSEGV)` exited 142, not 139. The handler now ends with `::raise(signum)`, which covers both delivery paths, and the measurement is back to 139. **`sa_mask` cannot do what its comment claimed.** It blocks signals for the thread running the handler, so it does nothing about thread A in SIGSEGV while thread B takes SIGBUS - both would write one static frame buffer. An atomic flag now lets the first entrant report and the second say so in one line. **Two demo binaries died silently.** `unresponsive_param_node` and `managed_lifecycle_node` own their shutdown sequence and deliberately do not use `run_demo_node()`, so they never installed the handler. They install it themselves now, and the README sentence is true rather than nearly true. **`SA_RESETHAND | SA_ONSTACK` is unsigned going into an int field**, which `-Wsign-conversion` reports once per including translation unit. Cast. **The frame assertion was anchored on the wrong path.** glibc prints `argv[0]` for the main executable, not the realpath `/proc/self/exe` returns, so a symlinked workspace or a hand-run `./test_crash_backtrace` would have failed on a correct report. Anchored on the basename. **The sweep refused to run for its own caller.** `./scripts/test.sh test_demo_lifecycle` contains `demo_`, so every single-test preset got "ROS processes are running" and never swept, with `test.sh` swallowing the exit so the refusal was the only symptom. Our own process and our caller are excluded now. **The record-and-census pair lived in one job of three.** `jazzy-test` and `graph-watchdog` had the shm bump but no margin guard, so a kill there still showed up as a single `missing_result`. All three now share one composite action. --- .../test-margin-and-environment/action.yml | 43 ++++++ .github/workflows/ci.yml | 83 ++++++----- scripts/sweep_shm.sh | 10 +- .../CMakeLists.txt | 1 + src/ros2_medkit_integration_tests/README.md | 8 +- .../demo_nodes/managed_lifecycle_node.cpp | 6 + .../demo_nodes/unresponsive_param_node.cpp | 6 + .../crash_backtrace.hpp | 41 ++++- .../test/test_crash_backtrace.cpp | 141 +++++++----------- 9 files changed, 208 insertions(+), 131 deletions(-) create mode 100644 .github/actions/test-margin-and-environment/action.yml diff --git a/.github/actions/test-margin-and-environment/action.yml b/.github/actions/test-margin-and-environment/action.yml new file mode 100644 index 000000000..e27af4ee1 --- /dev/null +++ b/.github/actions/test-margin-and-environment/action.yml @@ -0,0 +1,43 @@ +name: Record the test margin and the DDS environment +description: >- + Says plainly whether a test step ran out of time, and records what /dev/shm and + the installed middleware looked like afterwards. Both are invisible once a run + has finished, and a step killed by its cap is reported by the runner as an + ordinary failure naming whichever test was in flight. + +inputs: + start: + description: Epoch seconds recorded immediately before the test step. + required: true + cap-minutes: + description: >- + The cap the guard measures against. Pass the step's timeout-minutes where + the step has one; where only the job is capped, pass the job's - the + overrun branch cannot fire there, because a job-level kill stops every + remaining step, but the margin warning still arrives in time to act on. + required: true + +runs: + using: composite + steps: + - shell: bash + run: | + # The start is missing only when an earlier step failed before it ran. + # Say so and carry on to the census, rather than dying on a usage error + # and taking the environment evidence down with it. + if [ -n "${{ inputs.start }}" ]; then + ./scripts/ci_test_completeness.sh "${{ inputs.start }}" "${{ inputs.cap-minutes }}" || guard_rc=$? + else + echo "::notice::no test step start was recorded - the run failed before the tests" + fi + # A DDS participant killed rather than shut down leaves its segments + # behind for the rest of the job, and the installed middleware decides + # which transport those segments belong to. + df -h /dev/shm + echo "shm segments: $(ls /dev/shm | wc -l)" + # No `head` in this pipeline: it closes the pipe early, SIGPIPEs `ls`, and + # the step's `pipefail` would then kill the census exactly when there are + # enough segments to be worth listing. + ls -la /dev/shm | tail -n +1 + dpkg -l | grep -E 'rmw-fastrtps|fastdds|cyclonedds' || true + exit "${guard_rc:-0}" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c560968a5..0c7888b0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -137,31 +137,10 @@ jobs: - name: Check the run completed, and record the environment if: always() - run: | - # A step killed by its cap is reported as a failure, not a cancellation, - # and colcon then names whichever test was in flight. Without this, the - # overrun reads as one flaky test out of thousands, and the packages - # queued behind it leave no trace at all. - # The start is missing only when an earlier step failed before it ran. - # Say so and carry on to the census, rather than dying on a usage error - # and taking the environment evidence down with it. - if [ -n "${TEST_STEP_START:-}" ]; then - ./scripts/ci_test_completeness.sh "${TEST_STEP_START}" "${TEST_STEP_CAP_MINUTES}" || guard_rc=$? - else - echo "::notice::no test step start was recorded - the run failed before the tests" - fi - # A DDS participant killed rather than shut down leaves its segments - # behind for the rest of the job, and the installed middleware decides - # which transport those segments belong to. Neither is recoverable from - # a finished run, so both are recorded rather than assumed. - df -h /dev/shm - echo "shm segments: $(ls /dev/shm | wc -l)" - # No `head` in this pipeline: it closes the pipe early, SIGPIPEs `ls`, and - # the step's `pipefail` would then kill the census exactly when there are - # enough segments to be worth listing. - ls -la /dev/shm | tail -n +1 - dpkg -l | grep -E 'rmw-fastrtps|fastdds|cyclonedds' || true - exit "${guard_rc:-0}" + uses: ./.github/actions/test-margin-and-environment + with: + start: ${{ env.TEST_STEP_START }} + cap-minutes: ${{ env.TEST_STEP_CAP_MINUTES }} - name: Show test results if: always() @@ -283,6 +262,9 @@ jobs: ccache -s ./scripts/ccache_report.sh "${{ matrix.ros_distro }}-graph-watchdog" + - name: Record the test step start + run: echo "TEST_STEP_START=$(date +%s)" >> "$GITHUB_ENV" + - name: Run graph_watchdog tests timeout-minutes: 45 env: @@ -298,6 +280,13 @@ jobs: --ctest-args -LE linter \ --event-handlers console_direct+ + - name: Check the run completed, and record the environment + if: always() + uses: ./.github/actions/test-margin-and-environment + with: + start: ${{ env.TEST_STEP_START }} + cap-minutes: '45' + - name: Show test results if: always() run: colcon test-result --verbose @@ -400,6 +389,9 @@ jobs: ccache -s ./scripts/ccache_report.sh jazzy-test + - name: Record the test step start + run: echo "TEST_STEP_START=$(date +%s)" >> "$GITHUB_ENV" + - name: Run unit and integration tests env: # FastRTPS has a known use-after-free in discovery-teardown that @@ -416,6 +408,13 @@ jobs: --ctest-args -LE linter \ --event-handlers console_direct+ + - name: Check the run completed, and record the environment + if: always() + uses: ./.github/actions/test-margin-and-environment + with: + start: ${{ env.TEST_STEP_START }} + cap-minutes: '90' + - name: Show test results if: always() run: colcon test-result --verbose @@ -571,16 +570,22 @@ jobs: # colcon WARNS and exits 0 on a --packages-select name that does not # exist, so a rename would leave this shard testing nothing while the # merged report still looked complete - another shard's tests execute - # the same installed code and put it back in the report. Name-aware, - # because a shard selecting two packages still produces results when - # only one of the names rotted. - for pkg in ${{ matrix.select }}; do - case "${pkg}" in --*) continue ;; esac - if [ -z "$(find "build/${pkg}/test_results" -type f -name '*.xml' 2>/dev/null | head -n 1)" ]; then - echo "::error::shard '${{ matrix.shard }}' selected ${pkg} but it produced no test results - the name may have rotted" - exit 1 - fi - done + # the same installed code and put it back in the report. + # + # Only for --packages-select. The catch-all shard uses --packages-skip, + # where the names that follow are the packages it must NOT test - the + # first of them is ros2_medkit_opcua, which this job never builds, so + # checking them would fail that shard on every run. + set -- ${{ matrix.select }} + if [ "$1" = "--packages-select" ]; then + shift + for pkg in "$@"; do + if [ -z "$(find "build/${pkg}/test_results" -type f -name '*.xml' 2>/dev/null | head -n 1)" ]; then + echo "::error::shard '${{ matrix.shard }}' selected ${pkg} but it produced no test results - the name may have rotted" + exit 1 + fi + done + fi - name: Generate coverage report run: | @@ -703,12 +708,18 @@ jobs: # report still covers every package the source tree says holds production # C++. Sharding makes this stricter, not weaker: a shard that silently # tested nothing now shows up as a package missing from the merge. + # always(), like the merge above: when a shard fails this gate reports a + # missing package, and without this the HTML upload and the step that names + # the real cause would both be skipped - leaving "package X missing" as the + # only visible message for "shard Y failed". - name: Verify every C++ package reached the coverage report + if: always() run: | ./scripts/check_coverage_packages.sh coverage.info \ --skip ros2_medkit_opcua - name: Upload coverage HTML report as artifact + if: always() uses: actions/upload-artifact@v4 with: name: coverage-report @@ -718,7 +729,7 @@ jobs: # worth having when a shard broke - it just does not describe the whole # workspace, and saying so is this job's business. - name: Fail if any shard did not succeed - if: needs.coverage.result != 'success' + if: always() && needs.coverage.result != 'success' run: | echo "::error::a coverage shard did not succeed (matrix result: ${{ needs.coverage.result }}). The merged report above covers only the shards that finished, so its percentage is a floor, not the workspace figure." exit 1 diff --git a/scripts/sweep_shm.sh b/scripts/sweep_shm.sh index b59ff1896..7a8800be7 100755 --- a/scripts/sweep_shm.sh +++ b/scripts/sweep_shm.sh @@ -50,9 +50,15 @@ if [[ "${FORCE}" -eq 0 ]]; then # Match on the command line rather than the process name: colcon runs as # python3, and a pattern that also matches this script would match itself. # demo_ covers the launch fixtures the documented demo workflow starts, which - # the earlier pattern missed entirely - a developer running only demo nodes got + # an earlier pattern missed entirely - a developer running only demo nodes got # a sweep in the middle of live work. - if pgrep -f '[g]ateway_node|[f]ault_manager_node|[d]emo_|[c]test|[l]aunch_test|[c]olcon test' >/dev/null 2>&1; then + # + # Our own process and our caller are excluded, because the caller's command + # line is often a match by itself: `./scripts/test.sh test_demo_lifecycle` + # contains demo_, so every single-test preset refused to sweep and the only + # symptom was a puzzling message. + if pgrep -f '[g]ateway_node|[f]ault_manager_node|[d]emo_|[c]test|[l]aunch_test|[c]olcon test' 2>/dev/null | + grep -qvE "^($$|${PPID})$"; then echo "sweep_shm: ROS processes are running - refusing to sweep (use --force if you mean it)" >&2 exit 1 fi diff --git a/src/ros2_medkit_integration_tests/CMakeLists.txt b/src/ros2_medkit_integration_tests/CMakeLists.txt index f7a2ee95d..dab635448 100644 --- a/src/ros2_medkit_integration_tests/CMakeLists.txt +++ b/src/ros2_medkit_integration_tests/CMakeLists.txt @@ -104,6 +104,7 @@ target_include_directories(demo_param_beacon_node PRIVATE ${_demo_include_dir}) medkit_target_dependencies(demo_param_beacon_node rclcpp) add_executable(managed_lifecycle demo_nodes/managed_lifecycle_node.cpp) +target_include_directories(managed_lifecycle PRIVATE ${_demo_include_dir}) medkit_target_dependencies(managed_lifecycle rclcpp rclcpp_lifecycle) # Proof fixture for GRAPH_NODE_UNREADABLE (see the file doc): a plain rclcpp::Node that diff --git a/src/ros2_medkit_integration_tests/README.md b/src/ros2_medkit_integration_tests/README.md index 902d00ec6..104326cf2 100644 --- a/src/ros2_medkit_integration_tests/README.md +++ b/src/ros2_medkit_integration_tests/README.md @@ -80,9 +80,11 @@ ros2 launch ros2_medkit_integration_tests demo_nodes.launch.py ### Crash reports -Every demo binary goes through `run_demo_node()` in -`include/ros2_medkit_integration_tests/demo_node_main.hpp`, which installs a -fatal-signal handler before anything else runs. A node killed by `SIGSEGV`, +Every demo binary installs a fatal-signal handler before anything else runs. +Most get it from `run_demo_node()` in +`include/ros2_medkit_integration_tests/demo_node_main.hpp`; the two that cannot +use that helper - `unresponsive_param_node` and `managed_lifecycle_node`, which +own their shutdown sequence - call `install_crash_backtrace()` themselves. A node killed by `SIGSEGV`, `SIGBUS` or `SIGABRT` writes its stack to stderr, so launch_testing captures it with the rest of the process output: diff --git a/src/ros2_medkit_integration_tests/demo_nodes/managed_lifecycle_node.cpp b/src/ros2_medkit_integration_tests/demo_nodes/managed_lifecycle_node.cpp index 41b35d705..c27aa8069 100644 --- a/src/ros2_medkit_integration_tests/demo_nodes/managed_lifecycle_node.cpp +++ b/src/ros2_medkit_integration_tests/demo_nodes/managed_lifecycle_node.cpp @@ -19,6 +19,8 @@ #include #include +#include "ros2_medkit_integration_tests/crash_backtrace.hpp" + using CallbackReturn = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn; // A managed lifecycle node for status integration tests. It DEFAULTS to staying @@ -62,6 +64,10 @@ class ManagedLifecycleNode : public rclcpp_lifecycle::LifecycleNode { }; int main(int argc, char ** argv) { + // This node does not go through run_demo_node(), so it installs the + // crash reporter itself - otherwise it would be the one demo binary that + // still dies without leaving a stack behind. + ros2_medkit_integration_tests::install_crash_backtrace(); // Wrap the body so no exception escapes main (bugprone-exception-escape): the // run_demo_node helper used by the other demo nodes only accepts an // rclcpp::Node, so a LifecycleNode needs its own main. diff --git a/src/ros2_medkit_integration_tests/demo_nodes/unresponsive_param_node.cpp b/src/ros2_medkit_integration_tests/demo_nodes/unresponsive_param_node.cpp index 74e12590c..b1da03e7c 100644 --- a/src/ros2_medkit_integration_tests/demo_nodes/unresponsive_param_node.cpp +++ b/src/ros2_medkit_integration_tests/demo_nodes/unresponsive_param_node.cpp @@ -44,6 +44,8 @@ #include +#include "ros2_medkit_integration_tests/crash_backtrace.hpp" + class UnresponsiveParamNode : public rclcpp::Node { public: UnresponsiveParamNode() : Node("unresponsive_param_node", rclcpp::NodeOptions().start_parameter_services(false)) { @@ -114,6 +116,10 @@ class UnresponsiveParamNode : public rclcpp::Node { }; int main(int argc, char ** argv) { + // This node does not go through run_demo_node(), so it installs the + // crash reporter itself - otherwise it would be the one demo binary that + // still dies without leaving a stack behind. + ros2_medkit_integration_tests::install_crash_backtrace(); // Deliberately NOT ros2_medkit_integration_tests::run_demo_node(): that // helper calls rclcpp::shutdown() only after executor.spin() has already // returned, which works for every other demo node but would deadlock this diff --git a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp index 9252342a2..4ccaf1d32 100644 --- a/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp +++ b/src/ros2_medkit_integration_tests/include/ros2_medkit_integration_tests/crash_backtrace.hpp @@ -14,6 +14,7 @@ #pragma once +#include #include #include @@ -49,6 +50,17 @@ inline void ** crash_frame_buffer() { return frames; } +/// Lets exactly one thread write a report. +/// +/// sa_mask does NOT do this: it blocks signals for the thread running the +/// handler only. Thread A faulting on SIGSEGV while thread B takes SIGBUS gives +/// two concurrent handlers writing one static frame buffer and interleaving +/// their output. The second entrant says so in one line and gets out of the way. +inline std::atomic_flag & crash_reporting_flag() { + static std::atomic_flag reporting = ATOMIC_FLAG_INIT; + return reporting; +} + /// A stack of its own for the handler to run on. /// /// The commonest silent SIGSEGV is a stack overflow, and that is exactly the one @@ -89,6 +101,13 @@ inline void crash_handler(int signum) { // trade: a wrong status beats no output and no status at all. ::alarm(5); + if (crash_reporting_flag().test_and_set()) { + write_literal(kCrashMarker); + write_literal(" second thread also faulted; its frames are not reported\n"); + ::raise(signum); + return; + } + write_literal(kCrashMarker); switch (signum) { case SIGSEGV: @@ -112,6 +131,16 @@ inline void crash_handler(int signum) { ::backtrace_symbols_fd(frames, depth, STDERR_FILENO); write_literal(kCrashMarker); write_literal(" end\n"); + + // Explicit, not implicit. Returning re-executes the faulting instruction, + // which re-raises a genuine SIGSEGV or SIGBUS - but a signal delivered by + // kill(), raise() or pthread_kill() has no instruction to retry, so the + // process would simply resume and then be killed by the alarm above five + // seconds later, reporting SIGALRM instead of the signal that happened. + // Measured before this line existed: raise(SIGSEGV) gave exit status 142, not + // 139. SA_RESETHAND has already restored the default disposition, so this + // terminates the process with the right status in both cases. + ::raise(signum); } } // namespace detail @@ -146,10 +175,9 @@ inline void install_crash_backtrace() { struct sigaction action {}; action.sa_handler = &detail::crash_handler; - // The handler writes into one static frame buffer, and SA_RESETHAND only - // resets the signal that fired. Without this, a second thread taking a - // DIFFERENT fatal signal would re-enter concurrently and interleave the two - // reports. + // Blocks the other fatal signals for the duration of the handler ON THIS + // THREAD. That is all sa_mask can do - the cross-thread case is what + // crash_reporting_flag() above is for. ::sigemptyset(&action.sa_mask); ::sigaddset(&action.sa_mask, SIGSEGV); ::sigaddset(&action.sa_mask, SIGBUS); @@ -159,7 +187,10 @@ inline void install_crash_backtrace() { // would have without us. // SA_ONSTACK puts the handler on the alternate stack above, which is what lets // it run at all when the ordinary stack is the thing that overflowed. - action.sa_flags = SA_RESETHAND | SA_ONSTACK; + // static_cast, because these constants are unsigned and sa_flags is int: + // -Wsign-conversion, which this workspace builds with, reports the change of + // value once per translation unit that includes this header. + action.sa_flags = static_cast(SA_RESETHAND | SA_ONSTACK); ::sigaction(SIGSEGV, &action, nullptr); ::sigaction(SIGBUS, &action, nullptr); diff --git a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp index bf7c300f3..e1836841a 100644 --- a/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp +++ b/src/ros2_medkit_integration_tests/test/test_crash_backtrace.cpp @@ -24,19 +24,22 @@ #include "ros2_medkit_integration_tests/crash_backtrace.hpp" -using ros2_medkit_integration_tests::crash_backtrace_is_active; using ros2_medkit_integration_tests::install_crash_backtrace; namespace { -constexpr bool kHandlerActive = crash_backtrace_is_active(); - // Recursion the optimiser cannot flatten into a loop. Measured across -O0 to // -O3: without the escaping address GCC turns this into a loop at -O2 and the // process spins forever instead of overflowing, which made an earlier version of // the test below hang rather than fail. volatile char * stack_probe_sink = nullptr; +// The recursion is the point of this function, so the compiler's warning about +// it is noise. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Winfinite-recursion" +#endif __attribute__((noinline)) int recurse_until_the_stack_runs_out(int x) { volatile char pad[4096]; pad[0] = static_cast(x); @@ -48,6 +51,9 @@ __attribute__((noinline)) int recurse_until_the_stack_runs_out(int x) { asm volatile("" : : "r"(pad) : "memory"); return recurse_until_the_stack_runs_out(x + pad[0]) + 1; } +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic pop +#endif /// Faults on an unmapped address that is NOT null. /// @@ -63,16 +69,18 @@ __attribute__((noinline)) int recurse_until_the_stack_runs_out(int x) { /// and the process then does not crash at all. /// A regex matching a backtrace frame that belongs to this test binary. /// -/// The object and offset pair is what addr2line turns back into a location, and +/// Anchored on the basename, not on /proc/self/exe: glibc prints argv[0] for the +/// main executable rather than its resolved path, so a symlinked workspace or a +/// plain `./test_crash_backtrace` would not match a realpath and the test would +/// fail on a perfectly good report. +/// +/// The object and offset pair is what addr2line turns back into a location. /// glibc's spacing between the offset and the address differs by release -/// (resolute writes ") [0x", noble writes ")[0x"), so the separator is loose. A -/// frame carries a symbol name before the "+" whenever the binary exports its -/// symbols, which CMake does by default, so that half is loose too. -std::string own_binary_frame_pattern() { - char exe[PATH_MAX] = {}; - const ssize_t len = ::readlink("/proc/self/exe", exe, sizeof(exe) - 1); - const std::string self = len > 0 ? std::string(exe, static_cast(len)) : std::string(); - return self + R"(\([^)]*\+0x[0-9a-fA-F]+\) ?\[0x[0-9a-fA-F]+\])"; +/// (resolute writes ") [0x", noble writes ")[0x"), and a frame carries a symbol +/// name before the "+" whenever the binary exports its symbols, which CMake does +/// by default - so both of those are matched loosely. +const char * own_binary_frame_pattern() { + return R"(test_crash_backtrace\([^)]*\+0x[0-9a-fA-F]+\) ?\[0x[0-9a-fA-F]+\])"; } void crash_by_unmapped_write() { @@ -88,45 +96,37 @@ void crash_by_unmapped_write() { // which is what a death test does: the body runs in a forked child and the // assertion matches that child's stderr. // -// Every case asserts in both build configurations rather than standing down -// under one of them. In a sanitizer build the promise is the opposite one - the -// sanitizer owns the fatal signals and this handler must stay out of its way - -// so the absence of our marker is the thing worth pinning there. Replacing a -// sanitizer's report with a plainer stack is the way this file could do harm, -// and a test that went quiet under sanitizers would be blind to exactly that. +// These cases cover the configuration this package is built in, and only that +// one. The header also stands down under a sanitizer so it cannot replace a +// sanitizer's own report - but sanitizers are opt-in per package here, through +// include(ROS2MedkitSanitizers), and this package does not opt in. So +// __SANITIZE_ADDRESS__ is never defined for this translation unit, the +// stand-down never engages, and a test written for it would assert against a +// branch that cannot be reached. The stand-down stays in the header for the day +// this package does opt in; it is not claimed to be tested. TEST(CrashBacktrace, SegvIsReported) { - if constexpr (kHandlerActive) { - ASSERT_DEATH(crash_by_unmapped_write(), "MEDKIT-CRASH signal=SIGSEGV"); - } else { - ASSERT_DEATH(crash_by_unmapped_write(), ::testing::AllOf(::testing::HasSubstr("Sanitizer"), - ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); - } + ASSERT_DEATH(crash_by_unmapped_write(), "MEDKIT-CRASH signal=SIGSEGV"); } TEST(CrashBacktrace, SegvReportsResolvableFrames) { - if constexpr (kHandlerActive) { - // The marker alone would be satisfied by an empty stack. What makes a - // report useful is a frame carrying an object and an offset, because that - // pair is what addr2line turns back into a location. Asserting on a symbol - // NAME would pin the wrong thing: a release build without -rdynamic reports - // offsets for this binary's own frames, and the frames worth reading here - // belong to libraries below us anyway. - // - // Two things are matched loosely on purpose. glibc's spacing between the - // offset and the address differs by release (resolute writes ") [0x", - // noble writes ")[0x"), and a frame carries a symbol name before the "+" - // whenever the binary exports its symbols - a link flag away, and not what - // this test is about. - // Anchored on THIS binary's own path, read at runtime rather than written - // down: a bare offset pattern is satisfied by any frame, and libc's frames - // alone would pass it while saying nothing about whether our own frames came - // back resolvable. The path is not a name pin - it is whatever the test was - // built as. - ASSERT_DEATH(crash_by_unmapped_write(), own_binary_frame_pattern()); - } else { - ASSERT_DEATH(crash_by_unmapped_write(), ::testing::AllOf(::testing::HasSubstr("Sanitizer"), - ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); - } + // The marker alone would be satisfied by an empty stack. What makes a + // report useful is a frame carrying an object and an offset, because that + // pair is what addr2line turns back into a location. Asserting on a symbol + // NAME would pin the wrong thing: a release build without -rdynamic reports + // offsets for this binary's own frames, and the frames worth reading here + // belong to libraries below us anyway. + // + // Two things are matched loosely on purpose. glibc's spacing between the + // offset and the address differs by release (resolute writes ") [0x", + // noble writes ")[0x"), and a frame carries a symbol name before the "+" + // whenever the binary exports its symbols - a link flag away, and not what + // this test is about. + // Anchored on THIS binary's own path, read at runtime rather than written + // down: a bare offset pattern is satisfied by any frame, and libc's frames + // alone would pass it while saying nothing about whether our own frames came + // back resolvable. The path is not a name pin - it is whatever the test was + // built as. + ASSERT_DEATH(crash_by_unmapped_write(), own_binary_frame_pattern()); } // A stack overflow is the commonest silent SIGSEGV, and it is the one a handler @@ -138,45 +138,22 @@ TEST(CrashBacktrace, SegvOnAnOverflowedStackIsStillReported) { install_crash_backtrace(); static_cast(recurse_until_the_stack_runs_out(1)); }; - if constexpr (kHandlerActive) { - ASSERT_DEATH(overflow_the_stack(), "MEDKIT-CRASH signal=SIGSEGV"); - } else { - ASSERT_DEATH(overflow_the_stack(), ::testing::AllOf(::testing::HasSubstr("Sanitizer"), - ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); - } + ASSERT_DEATH(overflow_the_stack(), "MEDKIT-CRASH signal=SIGSEGV"); } TEST(CrashBacktrace, AbortIsReportedToo) { - if constexpr (kHandlerActive) { - ASSERT_DEATH( - { - install_crash_backtrace(); - std::abort(); - }, - "MEDKIT-CRASH signal=SIGABRT"); - } else { - // Only the absence of our marker here, unlike the SEGV cases: a sanitizer - // does not take SIGABRT by default (ASan's handle_abort is off), so there is - // no sanitizer report to require - and none for us to have clobbered. - ASSERT_DEATH( - { - install_crash_backtrace(); - std::abort(); - }, - ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH"))); - } + ASSERT_DEATH( + { + install_crash_backtrace(); + std::abort(); + }, + "MEDKIT-CRASH signal=SIGABRT"); } // Exit status is what ctest and launch_testing report, and a handler that // swallowed the signal would turn a crash into a clean exit and hide it. TEST(CrashBacktrace, ProcessStillDiesFromTheOriginalSignal) { - if constexpr (kHandlerActive) { - EXPECT_EXIT(crash_by_unmapped_write(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); - } else { - // A sanitizer reports first and then exits on its own terms, so the death - // itself is what stays assertable here. - EXPECT_DEATH(crash_by_unmapped_write(), ".*"); - } + EXPECT_EXIT(crash_by_unmapped_write(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); } // The crash helper installs the handler itself, so a test that merely calls @@ -189,11 +166,5 @@ TEST(CrashBacktrace, InstallingTwiceIsHarmless) { volatile int * volatile target = nullptr; *target = 1; }; - if constexpr (kHandlerActive) { - EXPECT_EXIT(crash_after_installing_twice(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); - } else { - ASSERT_DEATH( - crash_after_installing_twice(), - ::testing::AllOf(::testing::HasSubstr("Sanitizer"), ::testing::Not(::testing::HasSubstr("MEDKIT-CRASH")))); - } + EXPECT_EXIT(crash_after_installing_twice(), ::testing::KilledBySignal(SIGSEGV), "MEDKIT-CRASH end"); }