Repository navigation
gateway: answer a peer's GET /faults without fanning out again - #721
Open
mfaferek93 wants to merge 2 commits into
Open
mfaferek93 wants to merge 2 commits into
mfaferek93 wants to merge 2 commits into
Conversation
list_all_faults ignored X-Medkit-No-Fan-Out and fanned out on a request that came from an aggregating peer. Two gateways that aggregate each other bounced the request until they ran out of workers and answered empty. A request with the header is now answered from the local store.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
OpenAPI and documentation remain inconsistent with the new behavior, and required test conventions are unmet.
2 open findings
What changed in this PR
Prevents recursive global fault fan-out between mutually peered gateways.
Changes:
- Suppresses peer fan-out when the request carries
X-Medkit-No-Fan-Out. - Adds a two-gateway regression test.
- Registers the test with two isolated ROS domains.
| File | Description |
|---|---|
fault_handlers.cpp |
Adds global fault fan-out suppression. |
test_mutual_peer_faults.test.py |
Tests mutual-peer loop prevention. |
CMakeLists.txt |
Registers the two-domain test. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return [f.get('fault_code', f.get('code', '')) for f in response.json().get('items', [])] | ||
|
|
||
|
|
||
| class TestMutualPeerFaults(unittest.TestCase): |
Comment on lines
+494
to
+497
| // A request carrying X-Medkit-No-Fan-Out comes from an aggregating peer. | ||
| // Answering it with our own fan-out sends it back, so two gateways that | ||
| // aggregate each other would bounce it until the peer timeouts. | ||
| auto * agg = raw_req.has_header("X-Medkit-No-Fan-Out") ? nullptr : ctx_.aggregation_manager(); |
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
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.
Problem
GET /faults(list_all_faults) fans out to peers throughAggregationManager::fan_out_get, which marks the request withX-Medkit-No-Fan-Out. The handler on the receiving side never checked that header, so it fanned out again. The other collection handlers already honour it throughmerge_peer_items, only this path calledfan_out_getdirectly.With two gateways that aggregate each other (A lists B, B lists A) a single
GET /faultsbounces between them. Each hop holds a worker while it waits, so both gateways run out of workers. In the test below A then answers 200 with an empty item list, and every peer call logsFailed to connect to peer.Fix
A request carrying
X-Medkit-No-Fan-Outis answered from the local fault manager only. A request without it fans out as before.Tests
New
test_mutual_peer_faults(two gateways on two domains, each the other's static peer, B runs the faulty lidar):test_no_fan_out_request_answers_local_faults_only: A with the header returns only its own faults.test_mutual_peers_answer_without_bouncing:GET /faultson A returnsLIDAR_RANGE_INVALIDexactly once, not partial, faster than one peer timeout.Without the fix both cases fail: the header request returns B's faults, and the plain request returns an empty list after the bounce. With the fix: 3 of 3 runs pass.
test_cross_ecu_fanout,test_peer_aggregation,test_daisy_chain_aggregation,test_aggregator_fault_streamandtest_fault_handlers(38) still pass. clang-format-18 and ament_flake8 clean.Note: #700 and #680 also touch
fault_handlers.cpp. This change is one small hunk inlist_all_faults, so a rebase either way should be trivial.