fix(gateway): make executor_threads real and report a cancel timeout as a timeout - #593
Open
bburda wants to merge 17 commits into
Open
fix(gateway): make executor_threads real and report a cancel timeout as a timeout#593bburda wants to merge 17 commits into
bburda wants to merge 17 commits into
Conversation
…allback group server.executor_threads was decorative for RPC-response parallelism: the generic service clients, the per-action client trio, and the per-action status subscriptions all landed in the node default MutuallyExclusive callback group, so one long discovery refresh pass (e.g. a peer aggregation health check) stalled every in-flight service/action response regardless of the configured thread count - the response then surfaced as a bogus service timeout after the full budget (issue #575). Move the blocking-RPC clients (generic service clients + the per-action send_goal/get_result/cancel_goal trio) to one shared Reentrant group and the per-action /_action/status subscriptions to a dedicated MutuallyExclusive group - in-order delivery preserved, decoupled from the default group. Groups are created once at startup by a ros2_common factory (the issue #375 gate bans create_callback_group outside ros2_common/) and injected into the transports as constructor dependencies; the compat shim registers Humble clients into the passed group. Timers and the SSE-fault/trigger-fault/rosout subscriptions stay in the default group by design: refresh passes must remain serialized and those subscriptions rely on in-order delivery. Covered by an executor-starvation falsifier (black-hole aggregation peer keeps a refresh pass permanently in flight; a service-backed operation must still complete fast), a single-thread no-deadlock guard, an executor_threads clamp sweep over the documented [1, 256] range, and a unit pin of the group wiring itself.
…t rejection A CancelGoal round-trip that produced no response was collapsed into the same 400 x-medkit-ros2-action-rejected error as a definitive server rejection, and the tracked execution status was never reconciled with the /_action/status stream - the client was told the cancel was refused while the goal was in fact cancelling (issue #576). ActionCancelResult now carries an explicit CancelOutcome set by the transport at every exit, and both cancel entry points (DELETE execution and PUT-stop) share one mapping helper: - timeout: consult the tracked goal fed by the /_action/status stream - if it already shows CANCELING/CANCELED the cancellation is in progress (204, or 202 for PUT-stop); otherwise 504 with the standard SOVD not-responding code and a message pointing at the execution status resource. Tracked status is never hand-written on this path - the status stream stays the authority. - cancel service unavailable: 503 x-medkit-ros2-action-unavailable - transport failure: 500 x-medkit-ros2-action-unavailable - server rejection (return_code 1/2/3): 400 x-medkit-ros2-action-rejected, unchanged The 15s cancel budget floor is removed: cancel uses service_call_timeout_sec like every other action RPC - with timeouts reported honestly and reconciled against the status stream, the floor no longer papers over anything. The execution cancel/stop routes declare their real status codes in OpenAPI and the REST docs, and the stale Common Error Codes table now matches the implemented constants (phantom ERR_TIMEOUT/ERR_INVALID_INPUT/ERR_OPERATION_FAILED/ ERR_INVALID_ENTITY_ID rows replaced, not-responding -> 504 added). Covered by unit-fixture falsifiers driving a raw CancelGoal service that swallows or rejects requests (timeout -> 504 with tracked status untouched, stream-reconciled timeout -> 204/202, rejection -> 400 for both entry points) and an integration test that kills the action server mid-goal and expects 503. The handler-level stop test now asserts the mapped 504 instead of the never-produced ERR_VENDOR_ERROR.
clang-tidy (performance-unnecessary-value-param) flags the FaultEvent shared_ptr being copied on every subscription callback invocation while only read as const. Pass it by const reference.
declare_parameter<int>() takes the int64 a ROS parameter actually holds and narrows it on the way in, which -Wconversion flags and which lets an out-of-range severity_floor or max_tracked_nodes wrap before its own range check ever runs. Read both as int64, clamp in that domain, and narrow after.
The action status stream is the authority for a goal's state, but the RPC completions were still writing over it. An accepted CancelGoal wrote CANCELING unconditionally, so a stream frame carrying the terminal CANCELED that arrived first was moved backwards permanently: no further frame is ever published, GET kept reporting the goal as running, and it only left the tracking map through the stuck-goal path with a false "action server crashed" warning. update_goal_status now refuses to leave a terminal state - the guard belongs there rather than at the cancel call site because goal-accept and get_result can land late in exactly the same way, while the stream keeps writing directly under the lock. For the same reason a tracked goal must never lose its status stream: cleanup reads the goal count for a path and unsubscribes afterwards, so a goal sent in between kept a subscription that was about to be destroyed, and since the cancel-timeout path reconciles against that stream such a goal could only ever answer 504. unsubscribe_from_action_status now re-checks under the lock that guards the erase and lets a live goal veto it. The manager's three cancel guards never classified themselves and rode the kTransportError default into 500 "action server unavailable" - including for an execution the cleanup timer evicted between the handler's lookup and the manager's re-check, where the truthful answer is the 404 the same request gets a millisecond later. They now carry kNotTracked (404) and kInvalidRequest (400); only the missing-transport guard keeps 500. The mapper is declared in the header so those wire mappings are pinned directly: over HTTP they are reachable only through a race. Also on the response shapes: the reconciled 202 body rendered a hardcoded "running" although the reconcile set includes CANCELED, which an immediate GET of the same execution reports as "failed"; it now renders the tracked status. Its Location header was built from a hardcoded apps/components pair and sent areas and functions clients into the components collection; it now echoes the request path. A timed-out cancel on a goal the gateway already knows to be terminal no longer promises progress that cannot happen, and error parameters carry return_code only when a server actually returned one. The per-return_code wording for codes 1-3 now exists once, in the mapper - the transport's second copy meant deleting a mapper case silently changed the wire message with every test still green.
…budget With the 15s cancel floor gone this parameter is the whole cancel budget, yet it was declared with no range at all and appeared in no document. Zero and negatives were accepted silently and make the response wait expire immediately, so every cancel answers 504 unless the status stream wins the race; an unbounded value pins an HTTP worker for as long as it says. Clamp to [1, 3600] with a warning, like the sibling timeouts, and pin the whole config space - both degenerate directions, both endpoints and the default - against the budget the gateway actually applies rather than the value that was configured. Document the parameter next to its siblings in the server config table and define "cancel budget" where the DELETE/PUT route text uses it: the configured timeout plus up to 2s of cancel-service discovery, which a shorter budget does not shorten. Also correct the Common Error Codes table, where invalid-request was listed as 400-only thirty lines under the PUT block that documents it on 409.
…amped executor The sanitizer jobs multiply every declared CTest timeout, but a budget a test asserts on itself is invisible to that rewrite: an instrumented gateway can blow "must answer within 8s" long before ctest's clock runs out, and the failure reads as a starved response rather than as ASan/TSan overhead. Read the multiplier from MEDKIT_TEST_TIME_SCALE, which those two jobs now set to the same factor they apply to ctest. The unscaled value stays 8s so the falsifier keeps its edge where it was proven red - no scaling turns burning the full 10s service budget and answering 500 into a pass. The executor_threads clamp sweep asserted only startup log lines, so a regression that logged the clamped count and then failed to bring the executor up would have stayed green. Add one real request against the 256-thread gateway.
…st it Two of the three guards in cancel_action_goal cannot be tripped by a caller that honours the method's contract: both HTTP entry points resolve the execution through get_tracked_goal and answer 404 themselves, every tracked goal_id is produced by uuid_bytes_to_hex, and the action transport is a constructor dependency the gateway always supplies. Giving the malformed-id guard its own wire status was therefore covering an input no caller can build, so drop it and write the precondition down on the declaration instead. The tracked-goal re-check keeps its classification: that one a caller genuinely can trip, by racing the cleanup timer. Pin the surviving consequence end to end as well - after a cancel, the execution must reach a terminal ROS 2 status rather than sitting in "canceling" forever, which is the only place the stream-versus-RPC ordering is visible over HTTP.
rclcpp::Time was copied on every ERROR/FATAL log line only to be read, which clang-tidy flags as performance-unnecessary-value-param.
… copying req.path() already returns a const reference; the reconciled 202 branch was copy-constructing a std::string from it on every accepted stop.
…that was posted to The 202 on POST executions built its Location from a hardcoded apps-or-components pair, so on the areas and functions registrations of that route - it is registered for all four entity types - a client was handed a Location in the components collection, naming an entity that is not a component. It also bypassed api_path(). The created execution is a sub-resource of whatever collection the request targeted, so extend the request path instead, matching the bulk-data and lock handlers. Exercising this needs a genuine non-component entity that owns the action, because create_execution rejects a collection/entity-type mismatch before it gets that far, so the handler fixture now also seeds the area its component sits in.
…ubscribe Vetoing the unsubscribe while a goal is tracked only covered the decision, not the transport call that follows it with no lock held. A goal sent into that gap re-flagged the path, got a no-op from the transport because the subscription was still alive at that instant, and then had it destroyed by the unsubscribe already in flight - leaving the goal tracked, the path flagged as subscribed, and no status stream at all. Nothing repaired it, since the flag makes every later subscribe a no-op, so the goal reported "running" until the stuck-goal path evicted it with a warning blaming an action server that had done nothing wrong, and every timed-out cancel for it answered 504 instead of reconciling. Repair the pair after the transport call instead of holding the lock across it: taking our mutex into rclcpp's subscription create/destroy path would add a lock-order edge neither this method nor its subscribe sibling has today. The header now states the postcondition that is actually true and testable - on return, a path with a tracked goal has a live stream. The fake transport gains the idempotence the real one has, so it can no longer silently re-arm itself and hide the window, plus a one-shot re-entry hook that reproduces the production interleaving with no threads.
…l defines The status stream is now the gateway's only source of truth for a goal's terminal state, but it was subscribed KEEP_LAST(10) BEST_EFFORT VOLATILE while rcl_action publishes it KEEP_LAST(1) RELIABLE TRANSIENT_LOCAL. The profiles match, so the bug is not a mismatch - it is durability: the subscription is created only after the goal has been sent, and a VOLATILE reader is delivered nothing on match. An action that finishes inside that window published its terminal frame to a reader that did not exist yet, and no other code path ever re-reads a goal's status, so the execution reported "running" for the rest of its life. Reproduced 2 runs in 3 with an immediate goal. Use rcl_action's own profile, which is what it prescribes for the client side too: the fresh subscription is delivered the writer's last sample on match, which is exactly the frame it missed. KEEP_LAST(1) means the publisher overwrites rather than blocking, so a slow gateway cannot stall an action server; it can coalesce intermediate transitions, which is harmless because only the current status is tracked. The cancel fixture's stand-in publisher offered VOLATILE, which no action server does and which a TRANSIENT_LOCAL reader cannot match - it now makes the same offer as the server it stands in for.
The new 504 told clients to poll the execution status resource, but nothing
that resource documents can express a cancel outcome: it renders CANCELED and
ABORTED identically as "failed". The advice now names x-medkit.ros2_status,
which carries the ROS-level state verbatim, and rest.rst documents that field
and corrects the execution example, which named four keys the endpoint does not
emit and a status value that is not in its enum.
The terminal-status message quoted "succeeded"/"aborted" - raw ROS words the
resource it cites never produces. It now quotes that resource's own vocabulary,
and its test asserts the agreement rather than the string the handler happens
to print.
The 409 on re-executing a running execution carried invalid-request, which is
not in the SOVD standard code list, while the server already answers its other
409 with precondition-not-fulfilled ("prerequisites not met"). One status, one
code; the error table and the OpenAPI declaration follow, the latter having had
no 409 at all for a route that returns it.
If the goal is evicted between the mapper's read and the response body, the
202 no longer invents "running" for an execution whose own Location answers 404
- it says 404 too.
The parameter row claimed one budget for all four operation RPCs. Discovery
differs per RPC - bounded by the parameter for send goal, a fixed 2 s for get
result and cancel - so it now tabulates what each actually costs. The package
README documented cancel as 200 with a JSON body; it returns 204, and none of
the outcomes this branch added were there at all.
MEDKIT_TEST_TIME_SCALE covered a single assertion while three more Python budgets went unscaled and the C++ fixtures had no knob at all - and the sanitizer jobs run the unit suite too. A knob that covers some budgets reads as covering all of them, which is worse than not having one, because a flake in an unscaled budget gets diagnosed as a product regression. Cover the rest: the 256-thread gateway's /health deadline and per-request timeout, the terminal-status polls, and the cancel fixture's service-discovery, status-delivery and teardown budgets. Document the knob in CONTRIBUTING.md, where someone reproducing a sanitizer failure will look for it - it had no mention outside the workflow that sets it.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes two related ROS 2 gateway behaviors: (1) server.executor_threads now provides real RPC-response parallelism by moving service/action clients into a shared Reentrant callback group (with action status subscriptions in a separate MutuallyExclusive group), and (2) action cancel timeouts are now reported as timeouts (with reconciliation against the /_action/status stream), instead of being misreported as definitive rejections.
Changes:
- Introduces shared gateway callback groups and wires service/action transports to use them, making executor thread count effective for unblocking RPC futures.
- Adds explicit cancel outcome classification (timeout vs unavailable vs transport error vs rejection vs not-tracked) and centralizes HTTP mapping for DELETE-cancel and PUT-stop.
- Adds parameter clamping/documentation (
service_call_timeout_sec,executor_threads) plus extensive unit/integration tests and doc updates for the new behaviors and status codes.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_log_bridge/src/log_bridge_node.cpp | Clamp/narrow ROS int64 parameters safely; minor signature tweak |
| src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp | Match cooldown_allows signature change |
| src/ros2_medkit_integration_tests/test/scenarios/test_scenario_action_lifecycle.test.py | Add terminal-state polling to catch status-stream races |
| src/ros2_medkit_integration_tests/test/features/test_thread_pool_starvation.test.py | Expand coverage: clamp sweep + multi-gateway launch assertions |
| src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py | New starvation falsifier for callback-group split |
| src/ros2_medkit_integration_tests/test/features/test_executor_single_thread.test.py | New guard: executor_threads=1 must still work end-to-end |
| src/ros2_medkit_integration_tests/test/features/test_action_status_first_goal.test.py | New test for first-goal terminal status delivery (durability/QoS) |
| src/ros2_medkit_integration_tests/test/features/test_action_cancel_unavailable.test.py | New test for 503 mapping when cancel service disappears |
| src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py | Add get_time_scale() for sanitizer-aware in-test budgets |
| src/ros2_medkit_gateway/test/test_operation_manager.cpp | Wire transports with shared callback groups; add status-stream assertions |
| src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp | Extend routing tests for cancel timeout budget + unsubscribe race |
| src/ros2_medkit_gateway/test/test_operation_handlers.cpp | Add Location-header test for non-component collections; update cancel expectations |
| src/ros2_medkit_gateway/test/test_generic_client_compat.cpp | Ensure compat generic clients honor a provided callback group |
| src/ros2_medkit_gateway/test/test_gateway_node.cpp | Add clamp tests for service_call_timeout_sec |
| src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp | New comprehensive unit fixture pinning cancel outcome mapping |
| src/ros2_medkit_gateway/test/test_callback_groups.cpp | New unit tests pinning callback-group wiring contract |
| src/ros2_medkit_gateway/src/ros2/transports/ros2_service_transport.cpp | Accept/store callback group; create clients in RPC group |
| src/ros2_medkit_gateway/src/ros2/transports/ros2_action_transport.cpp | Use RPC group for service clients; status QoS/profile + status group; classify cancel outcomes |
| src/ros2_medkit_gateway/src/ros2_common/callback_groups.cpp | Factory for shared callback groups (startup-time creation) |
| src/ros2_medkit_gateway/src/main.cpp | Update executor_threads docs/comments to reflect new callback-group behavior |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Declare new 409/503/504 responses in OpenAPI for stop/cancel routes |
| src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp | Centralize cancel mapping; fix Location construction; adjust 409 error code |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Create shared callback groups; clamp service_call_timeout_sec; wire transports |
| src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp | Small callback signature change (const-ref shared_ptr) |
| src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp | Remove cancel budget floor; add cancel outcome classification; harden status updates + unsubscribe race repair |
| src/ros2_medkit_gateway/README.md | Update cancel/stop API docs and outcome table |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_service_transport.hpp | Document and require RPC callback group for service transport |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp | Document RPC/status callback groups for action transport |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2_common/callback_groups.hpp | New shared callback group contract + rationale/constraints |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/typed_router.hpp | Clarify TypedRequest::path() semantics for Location construction |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp | Store callback groups in node so they outlive transports/entities |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/operations/operation_types.hpp | Add CancelOutcome; extend ActionCancelResult |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/managers/operation_manager.hpp | Document cancel preconditions; add timeout getter; unsubscribe postcondition notes |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp | Expose cancel-mapper seam for unit testing |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/error_codes.hpp | Add standard not-responding error code constant |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/compat/generic_client_compat.hpp | Add callback-group parameter to both compat paths |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Update executor_threads documentation to match new dispatch model |
| src/ros2_medkit_gateway/CMakeLists.txt | Build callback_groups; add new gtests (cancel outcomes, callback groups) |
| docs/config/server.rst | Document service_call_timeout_sec range/semantics + executor threads behavior |
| docs/api/rest.rst | Update operation execution schema docs + stop/cancel outcome tables |
| CONTRIBUTING.md | Document MEDKIT_TEST_TIME_SCALE for sanitizer reproduction |
| .github/workflows/quality.yml | Export MEDKIT_TEST_TIME_SCALE for ASan/TSan jobs |
Suppressed comments (1)
src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp:266
- For CancelOutcome::kServiceUnavailable, the fallback message is hardcoded to "Cancel service not available". Since this mapping is shared by DELETE-cancel and PUT-stop, the stop endpoint can end up emitting a cancel-specific message even though the client requested a stop.
case CancelOutcome::kServiceUnavailable:
return CancelFailure{503, ERR_X_MEDKIT_ROS2_ACTION_UNAVAILABLE,
result.error_message.empty() ? "Cancel service not available" : result.error_message};
Comment on lines
+244
to
+256
| std::string message = | ||
| std::string(verb) + " outcome unknown: the action server did not answer the cancel request in time. "; | ||
| // CANCELED already reconciled above, so a terminal status here means | ||
| // the goal finished on its own. Telling the client to watch for | ||
| // progress would describe something that cannot happen - say what the | ||
| // gateway already knows instead, in the vocabulary the resource being | ||
| // named actually answers in (`sovd_status_from_ros2`, not the raw ROS | ||
| // word: the execution resource never emits "succeeded"/"aborted"). | ||
| if (tracked.has_value() && | ||
| (tracked->status == ActionGoalStatus::SUCCEEDED || tracked->status == ActionGoalStatus::ABORTED)) { | ||
| message += "The execution status resource already reports the goal as " + | ||
| sovd_status_from_ros2(tracked->status) + ", so there is nothing left to cancel."; | ||
| } else { |
The TSan job SIGKILLed the sweep's ceiling gateway. The log shows why, and it is not the OOM killer - launch_testing sent the signal itself after its own escalation ran out: "failed to terminate '30' seconds after receiving 'SIGINT', escalating to 'SIGTERM'", then "failed to terminate '45.0' seconds after receiving 'SIGTERM', escalating to 'SIGKILL'". All six functional assertions had already passed, including the /health request, so the gateway started, applied its bound and served correctly; only joining 256 instrumented executor threads outlived the grace period. Unsanitized the same teardown costs nothing measurable - the whole test runs in about 1.3 s - so this is instrumentation cost, not gateway behaviour, and the fix belongs in the harness rather than in the documented range. Resolve the upper endpoint per build: the documented 256 normally, 16 under a sanitizer. Detection reads the sanitizer runtimes' own *SAN_OPTIONS so a future sanitizer job cannot forget to opt in, with MEDKIT_TEST_SANITIZED as an explicit override for reproducing locally; both jobs now set it as well. The constant says plainly that a sanitized run does not pin the documented ceiling and that every normal build still does, so nobody reads coverage into a green sanitizer run that it does not have. The /health request stays, and the pinned "Main executor bounded to %zu threads" wording is untouched.
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.
Pull Request
Summary
Two related bugs on the action cancel path.
server.executor_threadshad no effect. Every ROS entity the gateway creates was placed in the node default callback group. That group is mutually exclusive, so callbacks run one at a time, no matter how many executor threads are configured. An HTTP thread waiting for a service or action response could only be unblocked by that one group. The discovery refresh runs in the same group, so a full refresh pass could delay a response that a client was waiting for.The generic service clients and the three action clients now use a shared reentrant group. The per-action status subscriptions use a shared mutually exclusive group of their own. Order inside one subscription is still kept, but status updates no longer wait behind the default group. Both groups are created once at startup, by a helper in
ros2_common/. The gate that forbids creating callback groups outside that directory still passes and its allowlist did not grow. Timers and the fault, trigger and/rosoutsubscriptions stay in the default group. Discovery should be serialized against itself, andrefresh_mutex_already does that.The second bug: a cancel that timed out was reported as
400 x-medkit-ros2-action-rejected. That code means the action server refused the cancellation. What really happened is that no answer arrived in time. The goal may have been accepted for cancellation.ActionCancelResultnow carries an explicit outcome. The transport sets it on every exit path. One shared mapping is used byDELETE .../executions/{id}and byPUTwith{"capability": "stop"}:504with the standard codenot-responding, and the message says the outcome is unknown.503 x-medkit-ros2-action-unavailable.500 x-medkit-ros2-action-unavailable.404. This is the same answer the same request gets one moment later.400 x-medkit-ros2-action-rejected, not changed.The 15 s minimum cancel budget was added earlier as a temporary fix. It is removed here. The budget is
service_call_timeout_secagain, like the other action RPCs. That parameter had no range check and no documentation. Both are added.Two more problems on the same path were found while fixing it.
The status subscription used volatile and best effort. The action server offers reliable and transient local. The subscription is also created only after the goal is sent. So a goal that finished quickly could lose its last status message, and nothing could recover it. The subscription now uses the profile that
rcl_actiondefines for action clients.A goal sent while the cleanup timer was removing the subscription for the same action path could end up tracked with no status stream. After this change that state also decides whether a timed out cancel answers 204 or 504.
Two smaller fixes are included, both in code this change already rewrites. The
Locationheader of a created execution was built from a fixed apps or components pair. On the areas and functions routes it pointed into the components collection, and it did not useapi_path(). It is now built from the request path. The409answer on the stop route usedinvalid-request. That code is not in the SOVD list for this case, so it now usesprecondition-not-fulfilled. The same document already uses that code for the same situation. This changes the response body, so it is written here directly.Issue
Type
The breaking part is small. A cancel that answered
400before can now answer404,500,503or504, depending on what happened. The409on the stop route uses a different error code. No response that existed before was removed.Testing
There is a test for each behaviour. Every new test was first run against the old code, to check that it fails, and that it fails for the right reason.
Executor: an integration test points the gateway at an aggregation peer that does not answer. Each discovery pass then stalls in the health check. During the stall the test calls a service backed operation. On
mainthe call uses the full 10 s budget and returns500. With this change the answer comes back well inside the budget. A second test runs the same operations withexecutor_threads: 1, to show that the reentrant group does not need a second thread. The clamp is tested at both ends of the documented range.Cancel: a test fixture serves the cancel service directly. It can block, reject, or answer. This covers the timeout, the rejection and the case where the status stream already knows the result. Killing the action server after the goal is accepted covers the unreachable case in an integration test. The QoS fix is covered end to end with an action that finishes quickly. That test failed 2 of 3 runs before the fix and passed 5 of 5 after it.
Build, linters, unit tests, integration tests, clang-tidy and the documentation build all pass. Two integration tests fail.
test_opcua_securedfails the same way on branches that contain none of this work. The other one is a rosbag test that belongs to a different change.The generated API description was compared with the merge base. Both gateways were started and
/api/v1/docsand the entity collections were compared. Nothing was removed. Three responses were added: the503and504above, and the409description. The entity responses are identical.Checklist
Updated: the executor section of the server configuration page, both operation routes in the REST API page, and the common error code table. That table had four rows with constant names that do not exist in the code. They are corrected here.
Known, not fixed here
test_auth_manager.CleanupExpiredTokensfails about one run in six on my machine. It fails onmaintoo. This change does not touch any authentication code. It needs its own issue.