Dump a backtrace when CAMP dies (#217) - #218
Merged
Merged
Conversation
added 30 commits
August 26, 2026 00:28
Crash handlers in main.cpp: SIGSEGV/SIGABRT via backtrace_symbols_fd + std::set_terminate for uncaught exceptions, writing to both stderr and a per-run file under rclcpp::get_logging_directory(). No ADR (declined, diagnostics not architecture).
The plan review checked this plan's API claims against the installed Jazzy headers and gtest source. Three did not hold, and each would have shipped a defective artifact: - get_logging_directory() returns the BASE log dir, not the per-run timestamped directory. Files land flat and accumulate; the docs bullet must name the base path or a field agent looks where the file never is. - get_logging_directory() throws RCLError. Unhandled, an unresolvable log dir would abort CAMP before QApplication -- a diagnostics feature turned into a startup failure on the hosts it exists to serve. - gtest wraps death-test statements in try/catch, so a plain throw never reaches std::terminate and the set_terminate handler -- the half that targets #207 -- would have shipped untested. Also folds in the review's hardening items: sigaction + sigaltstack (a stack-overflow SIGSEGV is otherwise uncatchable), SIG_DFL restored on handler entry, backtrace() warm-up at install time, a guard against the terminate path emitting a second stack through abort(), a test assertion that actually covers the ENABLE_EXPORTS regression, O_CLOEXEC, SIGBUS/ SIGFPE/SIGILL, and a docs note that the output is mangled. Part of #217.
CAMP is a colcon-built binary, and apport discards crashes from unpackaged binaries outright (/usr/share/apport/apport:1136). No core is ever written, so when CAMP segfaulted five times on the operator station during the 2026-08-25 deployment it left nothing but "process has died [pid N, exit code -11]". Fixing that at the host level is root-level administration of a field host, which is not available mid-deployment -- so CAMP explains its own death instead. New camp_crash::install_crash_handlers(fd) covers fatal signals (SEGV, ABRT, BUS, FPE, ILL) and std::terminate, writing a backtrace to stderr -- where the ros2 launch log captures it, next to the "process has died" line -- and to <base ROS log dir>/camp_crash_<pid>.log. The terminate handler also names the active exception, which is the missing line for the abort-on-close class (#207). Handlers re-raise, so exit status and any supervisor respawn behavior are unchanged. Robustness details, each chosen against the heap-corruption threat model this exists to diagnose: - write()-only output throughout; backtrace_symbols_fd, never backtrace_symbols, which allocates. - SIG_DFL restored as the handler's FIRST statement, so a handler that itself faults dies rather than recursing. - sigaction + sigaltstack/SA_ONSTACK, without which a stack-overflow SIGSEGV cannot be caught at all. - backtrace() warmed up at install time; its first call may dlopen and allocate. - An already_dumped guard, so the terminate path's abort() does not emit a second backtrace rooted in abort() itself. - Log-dir resolution wrapped in try/catch: get_logging_directory() throws, and a diagnostics feature must never become a startup failure. The path is the BASE ROS log dir, not the per-run timestamped one -- launch does not export that to children -- so files land flat and the <pid> correlates them to the launch log. ENABLE_EXPORTS on the executable so symbols resolve instead of bare addresses. Death tests raise real SIGSEGV/SIGABRT and throw across a noexcept boundary (gtest catches a plain throw, which would leave the set_terminate half untested), and assert a RESOLVED SYMBOL NAME rather than merely non-empty output -- so dropping ENABLE_EXPORTS fails the test. Closes #217.
A blocked or broken stderr must not cost the durable crash file. Under ros2 launch stderr is a pipe to the launch parent; in the abort-on-close class (#207) that parent may be gone (SIGPIPE kills the process mid-dump and rewrites the exit status to 13) or not draining (the ~10-20 KB dump blocks in-handler forever, so CAMP hangs instead of dying and no "process has died" line is ever logged). emit()/emit_backtrace() now write g_crash_fd first and stderr second, and install_crash_handlers() sets SIGPIPE to SIG_IGN.
…inate (#217) on_terminate set g_already_dumped and then ran the allocating current_exception()/rethrow_exception() machinery before emitting any stack. If that machinery faults under the heap corruption this feature exists to diagnose, the SIGSEGV handler sees the guard already set, suppresses its output, and the crash produces a header line and no stack at all. Backtrace now goes out first; the reason line follows on its own '=== CAMP terminate reason: ... ===' line. This is the Plan Review should-fix 'order the terminate handler's output' that was implemented only in its already_dumped half.
…#217) sigaltstack() is per-thread and pthread_create() does not inherit it, so installing one on the main thread covered only the main thread — while the comment, the header and the operator-facing .agents/README.md bullet presented the stack-overflow SIGSEGV class as handled everywhere. - the alt stack is now thread_local, behind a new exported install_thread_alt_stack(); - camp_ros::NodeThread::start() calls it, so the ROS node thread is covered too; - the rclcpp-internal threads (MultiThreadedExecutor workers, the TransformListener thread) offer no entry hook and are documented as a known gap in both the header and the README bullet, rather than left implied as covered. sigaltstack()'s return value is now checked; a rejected stack releases the buffer instead of masquerading as a live one.
…st target (#217) The regression guard that test_crash_handler.cpp and the CMake comment both advertised did not exist: CMakeLists.txt sets ENABLE_EXPORTS on test_crash_handler independently, so deleting it from CCOMAutonomousMissionPlanner left every test green while every field crash dump reverted to bare addresses. Adds a check_camp_exports CTest that runs readelf --dyn-syms against $<TARGET_FILE:CCOMAutonomousMissionPlanner> and requires a camp_crash* symbol in .dynsym, plus cmake/check_dynamic_symbols.cmake. Skipped with a warning if no readelf is available. Both misleading comments corrected, and the ENABLE_EXPORTS symbol-interposition tradeoff is now recorded next to the flag.
…und (#217) Review suggestions, all on the handler path: - the already_dumped guard was a non-atomic test-and-set, so two threads faulting in the same window both passed and interleaved two dumps; now __atomic_test_and_set(__ATOMIC_ACQ_REL), which is lock-free and async-signal-safe; - SA_SIGINFO, so si_code/si_addr are reported — often the datum that separates a null this->member from a use-after-free before anyone reads the stack; - the header line now names the faulting thread (gettid), so a multi-threaded crash does not silently attribute the dump to whichever thread won the guard; - alarm(10) after the SIG_DFL restore bounds the handler: backtrace() takes glibc's loader locks, and a fault while another thread holds one in a GDAL/Qt-plugin dlopen would otherwise hang forever, which is worse than the silent death it replaces; - SIGSTKSZ (a sysconf() call on glibc >= 2.34) is evaluated once, so the allocation and the kernel's ss_size cannot disagree; - every sigaction() failure is now reported at install time instead of silently dropping a crash class; - SA_RESTART dropped (inert on handlers that never return) in favour of SA_RESETHAND, and the handler is extern "C" as sa_sigaction wants; - the raise() comment no longer implies host core handling is unaffected — the core would be taken at the raise site. Behaviour deliberately unchanged: no core is written on these hosts anyway. Adds an async-signal-safe emit_ulong() (no snprintf on the handler path).
Pre-opening with O_CREAT|O_TRUNC left a zero-byte camp_crash_<pid>.log behind after every clean run. In a ~/.ros/log holding 10k+ entries that makes 'no crash' indistinguishable from 'crashed before the first write' and buries the real reports; and on a host with the stock pid_max, a recycled pid silently truncated an earlier genuine crash report. open_crash_log_fd() becomes crash_log_path(), which resolves the path (the part that needs rclcpp and allocates) but creates nothing. install_crash_handlers() takes the path; the handler opens it on the first crash with open(2), which is async-signal-safe. The fd overload is kept for callers with no ROS logging directory and for tests. Open flags hardened at the same time: O_APPEND instead of O_TRUNC (a recycled pid now appends rather than destroying the older report), O_NOFOLLOW and mode 0600 — the path derives from $ROS_LOG_DIR/$ROS_HOME, so a pre-planted symlink in a shared log dir must not redirect the write, and the dump embeds full install paths. main.cpp also installs stderr-only handlers BEFORE rclcpp::init(), which is itself a documented thrower: an exception escaping it would otherwise reach std::terminate with the default handler and die as silently as before #217. The death tests lose their open_temp() helper (its EXPECT_GE ran inside the forked child where gtest failures are invisible, and its predictable non-O_EXCL name sat in a world-writable directory) — the handler now creates the file itself.
… paths (#217) - all three ASSERT_EXIT matchers were "", so deleting every write(STDERR_FILENO, ...) would have left the suite green — while the stderr copy is the half that lands in the ros2 launch log next to 'process has died'. Now matched against the real preamble; - a crash on a worker thread, which is where CAMP actually crashes (ROS callbacks never run on the installing thread) and what would have surfaced the main-thread-only alternate signal stack; - NoCrashFileMeansNoFile pins the zero-byte-litter fix: installing must create nothing; - crash_log_path() had no coverage at all, nor did the fd == -1 / unopenable-path degradation that crash_handler.h promises must never keep CAMP from reporting. 8 cases in test_crash_handler now, from 3.
'apport discards crashes from unpackaged binaries outright' overstated it: verified at /usr/share/apport/apport:1135-1142, that branch drops the crash *report* but still writes a core if the user configured one. The conclusion holds under the default ulimit -c 0, but as written a reader concludes cores are impossible and skips a working local option. Also: crash_handler.cpp restored to the alphabetical SOURCES ordering, and the stray double blank line after the ENABLE_EXPORTS block removed.
Steps 2, 3, 4 and 5, the Files to Change table, the ADR Compliance citation and the Consequences table now describe what was actually built: lazy crash-file creation, per-thread alternate signal stacks, the backtrace-before-introspection ordering, the check_camp_exports guard, and the pre-rclcpp::init() stderr-only install. A second Revisions table records each Round-1 finding against where the plan was corrected, plus the one declined suggestion and why.
…217) si_code is an int and is negative for every user-generated signal (SI_TKILL is -6), so the unsigned formatter printed [si_code=18446744073709551610] on every abort()-path dump — the #207 abort-on-close class this feature targets. Add an async-signal-safe signed formatter. si_addr was gated on the signal number rather than on si_code > 0, so a raise()d SIGSEGV printed the siginfo union's _kill member (uid << 32 | pid) as a fault address: verified as si_addr=0x3e8000f7924. Gate on si_code > 0, which is what "the kernel generated this" means. Both are now asserted in the SIGSEGV and SIGABRT death tests; reverting either half fails them.
on_terminate() runs the same emit_backtrace() as the signal path and can deadlock on the same glibc loader lock, but it had no alarm() bound: it reaches the signal handler's alarm only via abort(), downstream of the hazard. An uncaught exception thrown while another thread sits in dlopen() hung CAMP forever, with no "process has died" line ever logged. The existing comment's reasoning was also wrong. It claimed the alarm was safe because SIG_DFL had been restored — but that restores the FAULTING signal's disposition, never SIGALRM's, so a blocked or handled SIGALRM voided the bound silently. arm_watchdog() now restores SIGALRM to SIG_DFL and unblocks it before arming, and both paths call it. The new death test stages the hang with a pipe nobody drains, and blocks and handles SIGALRM in the child, so it fails on either half being removed (it hangs to ctest's timeout rather than failing fast — that is inherent to testing a bound).
CrashOnANonMainThreadIsStillReported claimed to be "the test that would have caught the alternate signal stack being main-thread-only". It is not: the worker raise()s on a healthy stack, and sigaltstack() only changes where the handler frame is pushed. Deleting install_thread_alt_stack() from both workers was verified to leave it green. StackOverflowOnANonMainThreadIsReported exhausts a worker thread's stack for real. Without an alternate stack the kernel cannot push a handler frame and the process dies of SIGSEGV having written nothing at all — verified: the death test's stderr comes back empty and the matcher fails. It also asserts si_addr IS printed here, which is the kernel-generated half of the si_code gate. The old test's comment now says what it actually covers.
#217) Seven Round-2 suggestions, all in crash_handler.cpp: - install_thread_alt_stack() reported nothing on either failure path, so a thread could silently lose stack-overflow coverage. Both now print, with the tid, at install time. - SIGSTKSZ is sysconf(_SC_SIGSTKSZ) on glibc >= 2.34 and returns -1 on failure, which static_cast<size_t> turned into SIZE_MAX. Treat -1 as a failure and floor the size at 64 KB: the pre-2.34 8 KB constant is tight against the 128-entry frame array, backtrace_symbols_fd's formatting and the unwinder. - signal(SIGPIPE) was unchecked, and its process-wide/execve-inherited scope and its overwrite of Qt Network's and GDAL-curl's disposition were unrecorded. - install_crash_handlers(int) overwrote g_crash_fd without closing an fd a handler had opened. Track ownership and close only ours. - No release barrier between the path memcpy and g_crash_path_valid; volatile orders the compiler, not another CPU. Release store + acquire load. - O_NOFOLLOW rejects only a final-component symlink; a hardlink or a symlinked parent directory still landed the append. fstat() after open() and require a regular, single-link, self-owned file. - The thread_local alt stack is leaked deliberately (a thread_local destructor would hand the kernel a dangling ss_sp while the thread can still take a signal). Say so, so it is not "fixed" into a use-after-free.
…ranch (#217) - Crash files were written to predictable /tmp/camp_crash_test_<tag>_<pid>.log in a world-writable directory, so a local user could pre-fill them and the content assertions would pass without the handler writing anything. One mkdtemp() directory (0700) per run, removed at exit. - NoCrashFileMeansNoFile installed set_terminate, five sigactions and SIGPIPE -> SIG_IGN in the gtest PARENT and relied on a trailing reset. It now runs in a forked child and the parent only inspects the filesystem. - crash_log_path()'s catch branch had no coverage while the test file claimed it did. CrashLogPathDegradesToEmptyRatherThanThrowing unsets ROS_LOG_DIR, ROS_HOME and HOME (restored by scope) and asserts the documented empty return. Verified non-vacuous: with the catch removed it fails with rclcpp::exceptions::RCLError "rcutils_expand_user failed".
…lf fails (#217) check_camp_exports was simply not registered when neither readelf nor llvm-readelf was found, announced by a configure-time message(WARNING) that nobody reads in colcon output — leaving a fully green run with the ENABLE_EXPORTS guard silently gone. Register it always and pass READELF through even as READELF_EXECUTABLE-NOTFOUND; the script now recognizes that and FATAL_ERRORs with the remedy. Verified both ways by running the script directly.
…ced (#217) crash_handler.h's opening summary still said the handlers write "to a pre-opened file" and ordered the writes stderr-then-file. Round 1 inverted both — the file is created at crash time and written first — and the same header contradicted itself 18 lines later, as did crash_handler.cpp. This is the first file a future reader opens. Also strike crash_log_path()'s "Must be called after rclcpp::init()": the only test of that function calls it with no init in the binary and passes, because it reads ROS_LOG_DIR/ROS_HOME/HOME directly.
Round 1 closed the per-thread alt-stack must-fix on a gap enumeration naming only rclcpp-internal threads. Verified against the tree, that is wrong twice over: - camp::ros::GraphThread (src/camp_map/ros/graph_thread.cpp), live in the shipped app via MainWindow, HAS an entry hook — run() — and does not call install_thread_alt_stack(). - Every QtConcurrent::run() worker is uncovered, and that pool is where the GDAL/raster/tile work implicated in #215 executes. Both are in camp_map / camp_map_ros. crash_handler.cpp is compiled only into the executable and the test target (CMakeLists.txt:72,458), never into those installed, exported libraries, so a call from them would be an undefined symbol. Closing it means promoting the crash handler into a library and changing that library's public surface — a design decision beyond this issue, so this commit corrects the documentation rather than making it silently, and the plan carries it as a follow-up. Also softened "still dies silently" to "may die silently, depending on which worker picks the callback up": MultiThreadedExecutor::spin() runs one worker inline on the calling thread, which here IS the ROS node thread and does have an alternate stack. The header's rule no longer reads as a universal claim — it names the one thread the executable starts.
…checkable (#217) The claim was load-bearing three times: an operator instruction, half the feature's justification, and the recorded basis for declining the raise-vs-return suggestion. Measured on the dev workstation deadpool on 2026-08-26: `ulimit -c` is unlimited (soft and hard), /proc/sys/kernel/core_pattern pipes to apport, and /var/lib/apport/coredump/ already holds five cores written by this feature's own death tests. Reading /usr/share/apport/apport confirms why: the unpackaged branch calls write_coredump_callback() BEFORE returning, and write_user_coredump() writes whenever the core ulimit is non-zero. Report and core are separate outcomes. So: state that no report is ever filed (which is true and is the real justification), state what was measured with host and date, and tell the reader to check `ulimit -c` themselves rather than hand-typing a value for a host nobody measured (AGENTS.md, Documentation Accuracy). The conclusion survives on its own terms — no report, and a core that needs debug symbols and gdb is not something an operator collects mid-deployment. The ::raise(sig) decline is re-argued on grounds that do not depend on the ulimit: returning from the handler only works for a hardware fault. For a DELIVERED signal — kill(1), or abort()'s own raise() on the #207 path — it resumes the interrupted code, leaving CAMP alive in an undefined state. Doing it right means branching on si_code in the death path. Also cite apport by branch name and installed version instead of a line range that moves.
Static initialization across ~100 translation units, Qt resource registration and GDAL driver registration have already run by the time main() starts, so the first install pass is as early as CAMP can reach — not earlier than any possible fault. Say which.
Corrects the stale prose the earlier fix passes never reached ("Both fds are
opened once at startup", "the pre-opened crash-file fd", the apport line
citation, "no core is ever written"), rewrites the sigaltstack and
handler-hardening bullets to describe what is built, and adds a third Revisions
table mapping every Round-2 finding to where it was addressed.
Also: re-argues the declined ::raise(sig) suggestion on grounds that do not
depend on any host's ulimit; records the two undocumented behaviors (PATH_MAX
truncation, the readelf requirement); records the raw-addresses-plus-
/proc/self/maps alternative as considered and declined on a verified premise
(backtrace_symbols_fd already prints the object-relative offset addr2line
needs); and opens a Follow-ups section for the camp_map alt-stack gap and the
missing operator-manual troubleshooting section.
added 7 commits
August 26, 2026 10:05
install_thread_alt_stack() has to be callable from the first statement of every thread CAMP starts, and CAMP starts threads in camp_map and camp_map_ros — which cannot call into the executable that links them. Compiled into the executable only, those threads had no symbol to call and no report for a stack-overflow SIGSEGV. Round 2 recorded that as a follow-up; this closes it. The split is mechanism from policy, and the reason is a dependency boundary. camp_map is ROS-free by design (ADR-0002) and now links this library, so anything libcamp_crash links, camp_map links. crash_log_path() is the one piece of #217 that needs rclcpp, so it moves the other way — into src/camp/crash_log_path.cpp, in the executable, which already depends on rclcpp. Reimplementing rcl's ROS_LOG_DIR / ROS_HOME / ~/.ros precedence to keep the library whole was considered and rejected: a second copy of upstream policy, free to drift out of agreement with the directory ros2 launch actually writes to, is a diagnostics feature failing quietly. One namespace (camp_crash), two targets, for that stated reason. test_crash_handler now links the shipped library rather than recompiling its source: the artifact camp_map links is the one that has to work, and a private recompile could stay green while the installed library did not.
sigaltstack(2) is a per-thread attribute that neither pthread_create(3) nor QThread inherits, so a thread without its own alternate stack cannot report a stack-overflow SIGSEGV at all: the kernel has no room left on the faulting stack to push a handler frame. Before this, that was every thread camp_map and camp_map_ros start — GraphThread and all seven QtConcurrent worker entry points, which is where the GDAL / raster / tile work implicated in #215 runs, and where a deep recursion is most plausible. Now reachable because the handler lives in libcamp_crash rather than in the executable. The QtConcurrent call is per-pool-thread, not per-task: the function is idempotent (a thread_local pointer test), so a pool thread is covered from its first CAMP task onward and pays a compare after that. Still uncovered, and stated no more broadly than it is true: rclcpp's spawned executor workers and the tf2_ros::TransformListener thread, which offer no thread-entry hook. Stack-overflow SIGSEGV only, on those threads; the sigaction() handlers stay process-wide for every other crash class.
…ndary (#217) Two new guards, and a repair to the one that was already there. check_worker_alt_stacks — nothing about writing a new QtConcurrent::run() call announces that its worker owes an install_thread_alt_stack(), and the failure is invisible: it builds, it tests green, and the only symptom is a crash that is never reported, on a boat, months later. The rule is deliberately coarse (a per-file count of thread entry points vs. install calls, not call-graph resolution) and fail-closed; CMake script is the wrong tool for resolving which function a run() dispatches to, and a check that guesses wrong in the permissive direction is worse than none. check_crash_lib_deps — camp_map links libcamp_crash, so libcamp_crash's dependencies are camp_map's, and camp_map is ROS-free by design (ADR-0002). That boundary erodes one plausible #include at a time, so it is asserted against the built .so's DT_NEEDED list rather than trusted. check_camp_exports — repaired, and it needed it. It looked for any camp_crash symbol in the executable's dynamic symbol table as proof that ENABLE_EXPORTS was still on. Once the handler moved into a library those symbols became undefined imports, which every dynamically linked executable carries: the guard would have passed with ENABLE_EXPORTS deleted. It now requires a DEFINED (non-UND) crash_log_path symbol — the function that stays in the executable precisely because it needs rclcpp. readelf also needs -W: without it the name is elided to _ZN10camp_crash1[...], short enough to still satisfy a substring match and long enough to hide which symbol matched. All three verified non-vacuous by making each one fail: the call deleted from grid_map.cpp; the deps check pointed at libcamp_map.so (7 violations); the exports check fed a UND-only symbol table.
.agents/README.md — the crash-backtrace pitfall said the alternate-stack coverage stopped at the main and ROS node threads and that closing the camp_map half was a follow-up. It no longer does; rewritten to what is true, with the rclcpp-internal residue stated as narrowly as it holds. A new bullet records why libcamp_crash is ROS-free and Qt-free, since that constraint is invisible from the code and is what the three CTest guards protect. camp_crash added to the target inventory and repository layout. plan.md — Files to Change rebuilt around the split; a Round 3 section records the three options weighed for the rclcpp dependency and why two were rejected; the alt-stack follow-up is struck as done in this PR and replaced with the rclcpp-thread residue and the two mechanisms that could close it; Estimated Scope corrected from nine files to nineteen.
…ts (#217) The alternate stacks were leaked deliberately, and the reasoning behind that was sound but incomplete: a thread_local destructor runs while the thread can still take a signal, so freeing there hands the kernel a dangling alternate stack — a use-after-free reachable only from a signal handler. sigaltstack(SS_DISABLE) FIRST, and free only if the kernel accepted it, closes exactly that window. A signal in between costs the stack-overflow coverage on a thread that is exiting anyway; it never costs a write into freed memory. The leak was affordable while every caller was a thread that lives as long as the process. Promoting the handler into a library changed that: most callers are now QtConcurrent workers, and QThreadPool expires an idle thread after 30 s and builds a fresh one for the next task (measured here: expiryTimeout() 30000 ms, maxThreadCount() 16). Bursty tile and raster work across a day-long deployment churns through thousands of threads at 64 KB each — a diagnostics feature quietly eating an operator station's memory is not a trade worth making for it. AltStacksAreReleasedWhenTheirThreadExits pins it over 2000 short-lived threads. Written first against RSS, it passed with the leak deliberately reintroduced: an alternate stack is allocated and never written to unless a signal lands on it, so its pages never become resident and RSS measures nothing. It reads virtual size, and was then verified failing with the leak and passing without it.
check_crash_lib_deps missed librosidl, libtf2 and libament — a type-support or transforms dependency would have passed a guard whose whole job is to catch a ROS dependency reaching camp_map. check_worker_alt_stacks globs only .cpp files, so a QThread subclass with run() defined inline in its header escapes it. Nothing in CAMP does that; the blind spot is now stated on the script rather than left for someone to discover, along with why the fix would be to define run() in a .cpp (the house style) rather than to widen the pattern to headers, where it would match every declaration. ADR-0002 describes the library stack and the ROS boundary; libcamp_crash now sits below camp_map and that boundary is load-bearing for it too. Recorded as a dated addendum which explicitly does not revise the decision — the boundary has not moved, it has gained a second dependent and a test that asserts it against the built .so.
Owner
Author
Round 3 — the worker-thread gap is closed, not carriedRound 2 closed the alternate-signal-stack must-fix by correcting the The design decision round 2 said should not be made in passing
Recorded as a dated addendum on ADR-0002 — the boundary has not moved, it has Now coveredEvery thread CAMP's own sources start installs an alternate signal stack on Still not covered, stated as narrowly as it is truerclcpp's spawned executor workers and the A leak the promotion turned into a real oneThe per-thread stacks were deliberately leaked, on reasoning that was sound but Guards, each verified non-vacuous by making it fail
VerificationClean rebuild from scratch, Authored-By: |
|
This appeared on my timeline, just wanted to drop by and mention |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
When CAMP dies on a field host it leaves nothing behind that says where.
On 2026-08-25 it segfaulted five times on the operator station during the
Appledore deployment (#215). All five produced no evidence from the process
itself: the per-node ROS log stops mid-line, and the launch log records only
process has died [pid N, exit code -11]. Identifying the fault addresses meantreading
/var/log/kern.log.The reason no crash report exists is not configuration drift.
kernel.core_patternpipes crashes to apport, and apport discards the report for unpackaged
binaries (
/usr/share/apport/apport:1135-1142) — CAMP is colcon-built, so itqualifies, unconditionally. Changing that means
systemd-coredumpor/etc/apport/settings, i.e. root-level administration of a field host, which isout of scope by workspace rule and unavailable mid-deployment.
So CAMP explains its own death instead.
What this does
Handlers for fatal signals (
SIGSEGV/SIGABRT/SIGBUS/SIGFPE/SIGILL) andfor
std::terminate, writing a backtrace to stderr — where theros2 launchlog captures it, next to the "process has died" line — and to
<base ROS log dir>/camp_crash_<pid>.log. The terminate handler also names theactive exception, which is the missing line for the abort-on-close class (#207).
Handlers re-raise, so exit status and supervisor respawn behaviour are unchanged.
Chosen against the heap-corruption threat model this exists to diagnose:
write()-only output;backtrace_symbols_fd, neverbacktrace_symbols,which allocates.
ros2 launchstderr is a pipe toa parent that may be gone (SIGPIPE kills mid-handler and changes the exit
status) or not draining (a full pipe hangs the handler, so the supervisor never
even logs the death).
SIGPIPEis ignored.SIG_DFLrestored as the handler's first statement, so a handler that itselffaults dies rather than recursing.
alarm()watchdog on both paths:backtrace()takes loader locks, andif another thread holds one inside a GDAL/Qt-plugin
dlopen(), a silent deathwould become a silent hang — strictly worse.
sigaction+ per-threadsigaltstack, without which a stack-overflow SIGSEGVcannot be reported at all.
after every clean run, and a recycled pid could truncate a real report.
try/catch—get_logging_directory()throws,and a diagnostics feature must never become a startup failure.
Coverage gap, stated rather than implied
Stack-overflow SIGSEGV is reported only on threads that have an alternate signal
stack, which
pthread_create(3)does not inherit. Covered: the main thread andcamp_ros::NodeThread. Not covered: rclcpp-internal threads(
MultiThreadedExecutorworkers,TransformListener),camp::ros::GraphThread,and the QtConcurrent workers. The latter two have entry hooks but live in
camp_map/camp_map_ros, andcrash_handler.cppis compiled only into theexecutable and the test target — calling it from those libraries would be an
undefined symbol. Closing that means promoting the crash handler into a library
and enlarging its public surface, which this PR should not decide in passing; it
is recorded as a follow-up in the plan. Every other crash class on those threads
is reported normally.
No ADR — considered and declined
camp's ADRs 0002-0015 are all map/scene/raster architecture. This adds
diagnostics: it does not change CAMP's architecture or constrain future design.
The decision is recorded in the work plan's Context and ADR Compliance sections
rather than as a separate ADR file, so the skip is deliberate and traceable.
Test plan
./ui_ws/test.sh camp→ 306 tests, 0 errors, 0 failures, 1 skipped(pre-change baseline 297).
test_crash_handlercontributes 11 gtest cases plusone CTest.
The tests are death tests that raise real signals and exhaust a real stack in
forked children. Each was demonstrated to fail with its fix removed —
recorded per-test in
.agent/work-plans/issue-217/progress.md, after two reviewrounds found three separate guards that could not fail:
si_codeassertions: without the signed formatter and thesi_code > 0gate,dumps print
[si_code=18446744073709551610 si_addr=0x3e8000f7924]— thataddress being literally
uid << 32 | pid.install_thread_alt_stack()removed from theworker, it fails with empty stderr — the silent-death class reproduced.
arm_watchdog()removed, it hangs; with the alarmreduced to a bare
alarm(), it also hangs, so the SIGALRM restore/unblock isload-bearing.
check_camp_exportsrunsreadelf --dyn-symsover the shipped executable,so deleting
ENABLE_EXPORTSfrom it fails the build's tests rather thansilently reverting the dumps to bare addresses.
Verification note for merge
camp is a project repo, so per ADR-0018
merging on a local attestation requires a full-scope
ci_local.shrun on the PRhead. That has not been run yet — it is a separate gate, not covered by the
local test run above.
Review trail
Reviewed locally before pushing: issue review, plan review (changes-requested,
3 must-fixes), and two pre-push code-review rounds (4 then 6 must-fixes, all
resolved). The full timeline, including the findings declined and why, is in
.agent/work-plans/issue-217/progress.md; the work plan carries## Revisionstables mapping each finding to its correction.
Closes #217.
Authored-By:
Claude Code AgentModel:
Claude Opus 5 (1M context)