opcua: rescan while disconnected, heal PLC_COMMS_LOST on connect, and name a plugin freeze-frame's source - #660
Draft
bburda wants to merge 8 commits into
Draft
opcua: rescan while disconnected, heal PLC_COMMS_LOST on connect, and name a plugin freeze-frame's source#660bburda wants to merge 8 commits into
bburda wants to merge 8 commits into
Conversation
Two failures share the same shape: the plugin decides something once, at startup, and can never revise it while it runs. Re-scan while no session is up. Config-less discovery (#544, #509) ran a single scan a couple of seconds after start. A gateway that boots alongside its PLC scans while the PLC is still coming up, finds nothing, falls back to opc.tcp://localhost:4840 and retries that endpoint for as long as it runs. Only a restart found the PLC. The poller's reconnect arm now asks the plugin for a fresh scan, rate limited by discovery.interval_s (default 30 s), and adopts a newly found server for its next connect attempt, resetting the backoff so the new endpoint is tried at once rather than after the dead one's accumulated wait. The rules that made discovery safe are unchanged: an explicitly configured endpoint_url still wins and is never rescanned, the scan stays a bounded read-only TCP sweep plus GetEndpoints, and nothing is scanned while a session is up. interval_s stops being an accepted-but-ignored knob. Clear PLC_COMMS_LOST on every successful connect. The fault raised for a sustained outage (#496) was cleared only when the running process still remembered raising it. The fault manager keys faults by fault code and persists them, so a fault raised before a gateway restart is standing in the store with nothing in memory to remember it, and the arm that would clear it is never entered when the first connect succeeds. The fault then stayed CONFIRMED for good. Both connect paths, the initial one and every reconnect, now send the clear regardless of what this process raised. The clear is fire and forget, so a clear for a fault that is not there costs nothing. The debounce that governs raising is untouched. Tests: the discovery pass and the endpoint adoption rule are exercised with injected probes, including the positive control that a configured endpoint is refused the very server an unconfigured one accepts. The comms-lost heal runs against the live test server, because only a connect that actually succeeds reaches that arm.
A freeze-frame captured for a plugin-backed entity (#564) reaches a client with an empty topic and an empty message_type. That is correct, the values are the plugin's live entity data and not a ROS message, but it leaves the snapshot with no field at all naming its origin. Two of these frames from different bridges are indistinguishable, and a frame is indistinguishable from a topic capture whose metadata went missing. Frame now carries the capture path that read it, and it is served as x-medkit.source: plugin_data_provider for a read through the owning plugin's DataProvider, plugin_x_plc_data_route for the in-process dispatch of the plugin's own x-plc-data route (bridges that export no DataProvider). topic and message_type are left empty rather than overloaded, since neither names a ROS topic here. The field is omitted, not emptied, when the capture named no path, so a fault-manager freeze-frame taken from a real topic is unaffected and carries its topic and message_type as before.
The gateway runs four nodes inside its own process, all named after itself: the gateway node, "<node>_sub" for the subscription executor, "<node>_fault_clients" for the fault-service transport, and "<node>_lifecycle_state_reader" for the lifecycle reader. None of them starts with an underscore, so the ROS 2 hidden-node convention does not cover them, and runtime introspection returned all four as ordinary Apps. The gateway advertised its own plumbing as diagnosable entities, and an operator browsing /api/v1/apps saw four entries that answer nothing useful. There were two half-answers to the same question. count_peer_nodes knew the gateway's own FQN plus "_sub" and "_fault_clients" but not the lifecycle reader, and the app filter knew only the underscore rule, so it dropped none of the four. Both now go through one predicate, is_own_gateway_node, so a fifth helper is declared in one place instead of two. The match is exact per suffix, never a prefix test: a genuine peer named "<node>_monitor" or "<node>2" must stay visible, and hiding a real node is the worse error. A fault_manager sharing the process is not ours either and stays listed. Remote entities are left alone: a peer's helper nodes carry the same fully qualified names and are the peer's own filter's business.
Every existing opcua docker scenario pins OPCUA_ENDPOINT_URL, which short-circuits discovery, so none of them can reach the failure this covers: the gateway and the PLC power on together, the start-up scan runs while the PLC is still booting, and the plugin is left retrying its fallback endpoint. The scenario starts the gateway first with discovery on and no endpoint configured, asserts it settled on the fallback endpoint with no session, then brings an OPC-UA server up on the same subnet and asserts the endpoint is adopted within two re-scan intervals. It also asserts the container never restarted, since a restart would satisfy the endpoint check while proving nothing: a restart is exactly what used to be needed. The network is created with an explicit /24 so the read-only sweep stays 254 hosts and finishes in seconds.
…cted A gateway that starts before its PLC runs the discovery rescan for the life of the outage, and several things it does in that state were wrong. Component identity. With no node map the SOVD component is named from the device. When the start-up connect fails there is no device to ask, so the name comes from the fallback endpoint and an empty DeviceInfo, and it was then pinned forever: after discovery adopted the real PLC the component still served opcua-<fallback host> while introspect() reported the adopted endpoint. The identity is now re-derived on the first poll of a new session (config-less mode only, an explicit node map still owns the name), the derived alarms entity follows the rename, and the change is logged at INFO. The docker race scenario gained a config-less pass that asserts the rename against a real server. Rescan cadence. The cadence was stamped when a sweep STARTED, so a sweep of a legal /16 (minutes at the defaults) made the next one due the moment it returned: the poll thread swept back to back and the reconnect attempt dropped to one per sweep. It is now stamped when the sweep ends. NetworkDiscovery::run() also takes a cancel predicate, bound to the shutdown flag and checked before each probe and between the sweep and identify phases, so shutdown() no longer has to wait out a sweep. Reconnect backoff. The rescan is consulted once per reconnect attempt and attempts are spaced by the exponential backoff, so the real cadence was max(interval_s, backoff) while the README, the header and the start-up log all said "every interval_s". The backoff ceiling is now capped at the rescan cadence while discovery is rescanning. interval_s. An unset interval and an explicit 0 both mapped to the 30 s default, so there was no way to keep discovery on and stop rescanning. They are now distinct: unset takes the default, an explicit 0 leaves the start-up scan one-shot, and a negative value is refused with a warning that no longer claims the kept default is one-shot. The start-up line no longer promises a loop that rescans "every 0s" either. Discovery report. A pass re-emitted its whole report every rescan, so a site with a secured-only server logged the same WARN every 30 s for the life of the process. A pass whose outcome matches the previous one now reports at DEBUG. The first pass, and every changed outcome, still reports at INFO/WARN. Connect-time clear. It is a link-state clear, not an operator resolving a root cause, so it now sets skip_correlation_auto_clear and cannot cascade-clear the symptom faults a rule attributes to PLC_COMMS_LOST. The poller's own clear on a successful reconnect is the same event and does the same. Those clears were also buffered unconditionally while the fault manager was unmatched, so a flapping link pushed real alarm reports out of the bounded buffer: the buffer now keeps at most one pending clear per fault code, evicts a clear before a report, and refuses a clear rather than dropping a report.
The two Frame::source constants had no assertion from a capture: only a hand-built frame in the merge helper's test named one, so swapping the DataProvider and route values left the whole suite green. The route and DataProvider loss-of-comms tests now each assert the constant their own path must produce, plus the literal wire value - symbol against symbol stays equal when the two constants are swapped, and that string is what every x-medkit.source consumer reads. The merge test that omits the key keeps its place as a helper contract for a frame a caller built without naming a path - the capture paths always name one - and says so instead of standing in as a control for them. The peer-node count test listed only two of the three helper nodes the gateway creates in its own process, so a lone gateway with a lifecycle reader would have counted a peer and skipped the empty-graph warning. Document the entity-frame source field in the REST fault snapshot reference: an entity frame carries no topic or message type, so source is the only provenance a consumer gets.
…y what is re-derivable
The per-entity SOVD route DELETE /{entity}/faults/{code} lands on
FaultProvider::clear_fault for a plugin-owned entity, which is the branch
the gateway takes instead of its own. The gateway sets
skip_correlation_auto_clear there so an operator scoped to one entity
cannot cascade-clear correlated symptoms reported by apps in other
entities, and the ClearFault contract documents that guarantee, but this
plugin sent the flag off and reopened the hole wherever a PLC is
involved. It now sets the flag, and the reason each call site sets or
clears it travels with the call: a ClearOrigin says whether the device
reported the condition inactive (a real resolution, cascade kept), the
link came back, or an operator cleared through the scoped route.
The pending-dispatch buffer used that same distinction too bluntly. It
gave up any clear before any report, but only the link-state clear is
re-derivable - the next reconnect sends it again. A device alarm's
inactive edge is as one-shot as its raise, so evicting it left the flush
replaying the raise with nothing behind it and the fault standing while
the device said inactive. Ranking is now by re-derivability: the
link-state clear is what a full buffer gives up first, everything else
ages out oldest-first as it did before.
The start-up discovery sweep claimed to be cancellable through the
shutdown flag, but nothing can set that flag while it runs: it happens
inside set_context(), during node construction, before the executor the
gateway shuts down from ever spins. A SIGTERM during a wide sweep
therefore waited the sweep out. Both sweeps now ask one predicate that
also reads rclcpp::ok(), which rclcpp's own signal handler turns false,
so the start-up sweep ends on the signal and the rescan keeps ending on
shutdown() as well.
Also: a rescan sweep that throws now stamps the cadence on its way out,
or the next poll iteration would immediately start another one; the
start-up log line no longer states the wrong reason for reporting in
full; and the package changelog records this branch.
Measuring that start-up sweep also showed the gateway logging nothing at
all while it ran: the discovery report is buffered so a repeated rescan
can be reported at DEBUG, and the "scanning [subnets]" announcement had
been swept up with it. A minutes-long sweep with no output reads as a
hung process, so the announcement is sent before the sweep again, with
the same first-pass INFO / rescan DEBUG levelling on its own.
…e wire Four call sites decide whether a ClearFault may cascade, and only the scoped SOVD DELETE was pinned: swapping the origin at any of the other three left the suite green. The two that need a live session are now driven against the test_alarm_server fixture with a real fault manager on the other end, so the flag is read off the wire. A successful connect must clear PLC_COMMS_LOST without cascading, and the same test drives the fixture's own CLI to fire and clear a condition, because a clear the device itself reported is a resolution at the source and must keep the cascade. Having both cases in one test is what makes each flag a decision rather than a constant. The fixture harness gained a stdin pipe to send those commands, the way the docker scenario already drives it through a FIFO. The poller's own clear travels the same callback as every device alarm, so the rule that tells them apart moved into clear_origin_for_signal and is tested on both branches, exact code match included. The sweep's cancel predicate had the same shape of hole: it is private and no test reached it, so reverting it to the shutdown flag alone left everything green while a SIGTERM during the start-up sweep would again have to wait the sweep out. The rule is now the static discovery_cancelled_for, tested on both inputs, with the member reduced to reading the two values off the process. A second test shows the input is real by shutting a private rclcpp context down and reading rclcpp::ok() back. The comment on the cancellation test no longer claims it exercises the plugin's own predicate, which it never did. Also: the remaining prose semicolons on this branch (two comments and five operator-visible log strings) are periods and commas now, and the Refused outcome's doc comment says what it means.
bburda
marked this pull request as draft
September 6, 2026 19:13
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
1 participant
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
In config-less discovery mode (
OPCUA_DISCOVERY_ENABLED=1, subnets set, noOPCUA_ENDPOINT_URL, no node map), a gateway that boots before its controller neverfinds it. The start-up scan runs while the controller is still coming up, finds nothing,
falls back to
opc.tcp://localhost:4840and retries that endpoint until someonerestarts it. Three more things read wrong beside it: the gateway lists the four nodes in
its own process as apps, a plugin entity's freeze-frame names no origin, and a
PLC_COMMS_LOSTraised before a restart stays CONFIRMED, because only the process thatraised it would clear it.
What changes
Discovery and reconnect
endpoint_urlconfigured, the reconnect arm rescans ondiscovery.interval_s, adopts a server that appeared after start-up and resets thebackoff. The cadence is measured from the end of a sweep, the backoff is capped at it
so the documented interval is the real one, and an unchanged outcome reports at DEBUG.
rclcpp::ok(). A rescan thatthrows still stamps the cadence.
session after an adoption, so the provisional
opcua-<host>name goes. The alarmsentity follows.
Comms-lost clears and the pending buffer
PLC_COMMS_LOSTis cleared on every successful connect, the first included, so apersisted fault does not stand against a healthy link.
skip_correlation_auto_clear. Aclear the device itself reports still cascades.
fault_manageris unreachable ranks by what is re-derivable.The link-state clear goes first, one clear per code is pending at most, and a device
alarm's inactive edge ages out like a report.
Gateway entity listing
peer named
<node>_monitoror<node>2stays visible. Remote entities are untouched.Freeze-frame source
x-medkit.source,plugin_data_providerorplugin_x_plc_data_route.topicandmessage_typestay empty, and the field isomitted, not emptied, when the capture named no path.
Docs and changelog
docs/api/rest.rst,docs/tutorials/snapshots.rst, pluginCHANGELOG.rst.Behaviour changes to know about
interval_sunset means the 30 s default, an explicit0means discovery on with norescan. The old README line was
interval_s: 0, so a config copied from it gains norescanning on upgrade. Drop the key for the default. A negative value is refused.
The plugin
CHANGELOG.rstmarks this one Breaking.DELETE /{entity}/faults/{code}served by this plugin now setsskip_correlation_auto_clear, as theClearFaultcontract already promised./api/v1/appsentries disappear with the plugin loaded, the gateway's own nodeamong them.
fault_managerstill logsAttempted to clear non-existent faultat WARN on everyconnect where nothing was standing.
Testing
11 packages finished,4974 tests, 0 errors, 0 failures, 539 skipped. Linters on the built packages:1140 tests, 0 failures.run_integration_tests.shTotal: 18 / Passed: 18,run_alarm_tests.shpassed, new
run_discovery_race_test.shpassed.(
endpoint still 'opc.tcp://localhost:4840' (connected='False') after 50s), andpasses on the branch through step 9 of 9, the config-less rename.
PLC_TANK_HIGHstill CONFIRMED afterPLC_COMMS_LOSTwas clearedthrough the per-entity route. The control with the flag off cascades.
uncancelled.
Not in this PR
fault_managerWARN for a clear of a fault that is not there.PLC_COMMS_LOST. Thathalf has unit coverage only.
source_idonPLC_COMMS_LOSTin the/api/v1/faultslist view while thefault manager logged a source. Unverified, filed on its own if it holds.
gateway_nodeexiting 139 on shutdown with the plugin loaded. It reproduces onmain,see [BUG] gateway_node segfaults on shutdown when the OPC UA plugin is loaded #659.
Issue
Type
Checklist