Conversation
10 tasks
Tests are yet to be implemented, so the code remains in an early stage, completely untested even for basic functionality
Improved SELECT result handling based on best practices, fixed typos, fixed potential password leaks from printing the database url and other minor issues
bburda
force-pushed
the
postgres-fault-storage-followup
branch
from
September 20, 2026 12:01
942c45b to
3fceb80
Compare
…outage - Define POSTGRES_SUPPORT only when the option is ON. The node checks it with #ifdef, so a build with the option OFF compiled the PostgreSQL code and failed to link. With the option OFF, storage_type postgres now stops the node at startup with a clear error. - Build libpqxx static and add it with EXCLUDE_FROM_ALL, so nothing of it is installed with this package. ament on Lyrical defaults BUILD_SHARED_LIBS to ON, so the option is forced OFF around it. CMake before 3.28 uses FetchContent_Populate and add_subdirectory. Remove the libpqxx-dev dependency, which the build does not use. Warnings-as-errors, clang-tidy and include-what-you-use are switched off for libpqxx only and restored afterwards; a cached -D value no longer reaches libpqxx or gets lost for the package's own targets. - Keep the FaultStorage getters const. The PostgreSQL connection is a mutable member, so the other backends and their tests stay as on main. - Start without the database when the server cannot be reached (refused, timeout, unknown host, wrong password, missing database or role). Stop only on a wrong configuration: a connection string libpq rejects, a server that accepts the connection but refuses the schema, or an existing table that lacks a column the node uses. The schema is created on the first connection that succeeds. The startup near-miss trim and HEALED reclassification are skipped while the server is unreachable, and snapshot capture ids are seeded on the first capture. - A node that cannot start logs the reason and exits with code 1. It no longer aborts with a core dump. - Bound the wait on an unreachable server. connect_timeout defaults to 2 s, tcp_user_timeout to 5000 ms and keepalives_idle to 5 s unless database_url sets them (PGCONNECT_TIMEOUT also sets connect_timeout); a libpq service keeps the values of its service file. After a failed round, requests fail at once for 5 s, then each round makes one attempt. The backoff also starts when the retries of a transaction run out, and a lost connection is retried without an extra sleep. - Never log or return the connection string. Error text from the server is logged and returned with the password from database_url or PGPASSWORD replaced by ***. The node logs host, port, database and user, or the service name; an explicit empty host or port is shown as given. - ClearFault clears the storage before it asks the correlation engine, so a clear the storage refuses leaves muted symptoms and clusters as they were. - Remove the early returns in the node constructor, which left the node running with no services after a storage error. - When the database is unreachable, services with an error field answer success=false and "Fault storage unavailable: ...". - Catch storage errors in RosbagCapture at the end of the post-fault recording and in the auto-cleanup on clear. Both run where an exception stops the process. - The time-based confirmation UPDATE uses the same last_failed_ns > 0 condition as its SELECT. - Docs: fix the empty database_url default that broke the Sphinx build. Describe the build option, libpq-dev, PostgreSQL 14 as the oldest server, one database per fault manager, what stops the node at startup and what does not, and the timeouts. - CI: a postgres job per distro (humble, jazzy, lyrical) builds the fault manager with -DPOSTGRES_SUPPORT=ON and runs its tests against a postgres:14 service. The other jobs keep the option OFF. - Fix the lint_cmake finding in the flag regex, a deprecated libpqxx call in the tests, and comments that described the SQLite backend. - The auto-confirm visibility test checks the confirmation window with the two node timestamps on the event: the event time and the fault's last occurrence. It used a clock in the test process, and a step of the system clock inside the window failed it. - Fix the compiler and clang-tidy warnings in the package: a useless cast on Humble, rclcpp::spin_some deprecated on Lyrical, and missing special members on a test helper. - The background-capture test waits until the graph carries the topic type. The constructor subscribes only to a topic whose type it can resolve, and a publisher reaches the graph some time after create_publisher returns.
bburda
force-pushed
the
postgres-fault-storage-followup
branch
from
September 20, 2026 13:24
3fceb80 to
6c68b15
Compare
Two of the three suites in the lifecycle-handler test initialised rclcpp and left it initialised. The suite that ran last therefore reached exit() with a valid default context, whose destructor calls Context::shutdown() after exit() has destroyed the thread_local state that call reads. The binary passed all its tests and then died with SIGSEGV. Both suites now shut the context down in TearDownTestSuite, as the first suite in the file already did.
This branch has not been deployed
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
2 participants
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.
Summary
The commits of PR 651 plus one commit on top of them, with fixes for the build, the startup and outages.
PR 651 stays open and this draft does not replace it. It exists so that CI runs: our workflows only trigger for pull requests against
main.The commit message lists the changes, and
docs/config/fault-manager.rstdescribes the behaviour.Issue
Type
Testing
Package tests with
POSTGRES_SUPPORTON and OFF on Humble, Jazzy and Lyrical (ON against PostgreSQL 14, Jazzy also against 18), and end to end runs with the real node and a PostgreSQL container.Checklist