Remediate the adversarial review: connectivity, conditions, resync, lifecycle, and passive quality - #8
Merged
Merged
Conversation
The formatting gate was failing repo-wide at baseline. Landing it as an isolated change keeps the behavioral diffs that follow readable.
…ness A cached probe model was treated as proof the agent had answered, so a device could report ONLINE against an agent that was never reachable -- and stay there, because AgentDown was broadcast only on the connected transition and a session attaching later never learned it. Connectivity now has one writer family. A cached model is never liveness: connect() and read_signals() both gate on the runtime's published state, and a newly attached queue is seeded with the current up/down fact so it cannot start out believing a dead agent is live. Streaming failures now participate. Every ladder-1 exit marks the agent down and backs off; the establish-failure counter resets only once a stream has delivered a liveness part, so an agent answering with valid headers and an immediate EOF degrades to polling instead of spinning an unbounded reconnect loop. Adds the shared interfaces the remaining phases build on: the two-lane instance queue, the injected clock seam, route-aware publish policy, StreamRun, and cancellation-aware spawn/shutdown.
One queue with one delivery class meant a lagging consumer could lose a Fault condition, an AgentDown, a DataLoss, a ModelDrift or a StreamDegraded exactly as easily as a scalar sample. Traffic now travels in two lanes. Scalar samples coalesce by data item under pressure -- a stale reading is superseded by the newer one for the same signal rather than either being lost -- while conditions and lifecycle facts take a reserved lane that waits for room, preempted by shutdown, and is counted if it ever gives up. Ordinary delivery is now one observation per event; a snapshot means a genuine re-baseline. That distinction also repairs the publish deadband, which could never suppress anything: every batch arrived labelled as a re-baseline and reset the deadband's entry state before it could apply. An attach snapshot now records its dedupe floors only once delivery is known, so a dropped snapshot republishes instead of being lost.
A document announcing a new instanceId was decoded against the probe model of the agent's previous incarnation and published immediately, with the re-probe following behind it. Any observation in that window was a hybrid: decoded with one generation of the model, routed with the next. Documents seen while a resync is pending now defer. They still count, still carry their header facts, and still prove the agent alive, but they dispatch nothing and set no dedupe floor -- so the post-resync snapshot, built against the freshly probed model, is the first thing an instance sees. A probe that fails during recovery fails the cycle rather than falling through to data it cannot trust. Routing joins publish-policy identity, so a reload or drift that moves a signal's channel, component path or name flushes its open batch window on the route those readings were collected under, instead of letting the window straddle both.
Device task handles were dropped on the floor at spawn, so nothing signalled or awaited them at teardown: the runtime could be torn down while shapers still held unflushed batch windows and sessions were still attached. Telling an agent to stop fared no better -- the request went into a queue that a blocked acquisition task was never going to read. Shutdown is now a cancellation-token tree with every handle retained. Devices are cancelled and joined first so their open windows flush and publish while the agents that feed them are still alive, then the agent tasks and their metric tickers, then a final metric flush. Each stage carries its own deadline and names whatever it has to abort. The budgets matter because SIGTERM is a process signal: it is precisely the input that still arrives when the broker or IPC channel is dead and nothing can drain. Against a dead transport the win is terminating cleanly rather than being killed; against a merely slow one, the batches actually land. Total worst case is 12s, inside the tightest stop window this component ships into. A clean stop no longer raises the device-unreachable alarm or counts as a reconnect. Stopping is not an outage, and the old path alarmed the whole fleet on every deployment.
…sting malformed input A data item held one condition slot, so the newest transition overwrote whatever was already there. Clearing one activation promoted the signal to GOOD while another Fault was still asserted, and a data item carrying both a Fault and a Warning settled on whichever the agent happened to serialize last. The standard permits concurrent activations and tracks them by conditionId across their transitions. Each data item now keeps a ledger keyed by activation. A signal's published state is the aggregate across everything still active, so it stays Fault until the last Fault clears, and the answer no longer depends on document order. The transition that triggered the sample still rides its own extras, now including conditionId and a count of what remains active. Three further places trusted input they should not have. An HTTP-200 MTConnectErrors answer to /current was reported as a parse failure rather than the agent's own error code; three consecutive such cycles now mark the agent unreachable. Observations missing a required sequence or timestamp were defaulted to 0 and the empty string, which let them publish as GOOD and then be suppressed as duplicates -- they are now refused and counted. A namespace declared with a prefix escaped the 1.3 version floor entirely. Element count is now bounded alongside the byte cap: a document of millions of tiny elements stayed under the byte limit while costing tens of times more heap.
…uires Staleness was counted into a metric and never published, so a consumer holding a GOOD value kept holding it -- indefinitely -- while the agent went silent or unreachable. Nothing on the data class ever said the value had stopped being true. A per-session watchdog now emits the transitions HLD section 6 defines. A held value whose agent has missed its liveness window degrades to UNCERTAIN with MTC_STALE and the age in milliseconds; past staleSignalSecs it becomes BAD; an unreachable agent takes every held signal to BAD directly. Recovery restores the quality the signal actually had, verbatim. The clock is the agent's liveness, not per-signal change age: MTConnect publishes on change, so an unchanged value under a live agent is current, not stale. The staleSignals metric keeps its own meaning -- value silence -- and is deliberately not merged with this. These samples are time-driven, so they are the first published data this adapter emits that no observation triggered. Each carries a passive extra naming the transition so consumers can tell them apart.
…nd close the remaining gates The connect/poll/publish loop had grown inside the seam that CI excludes from coverage, so the decisions it composes were never measured. Those decisions now live in driver.rs behind a Wire trait, leaving supervisor.rs as what it claims to be: construction, spawning, and shutdown invocation over a live runtime. 1648 lines join the denominator. That split also lets three tests land that earlier phases had to defer for want of a harness -- the mixed-generation flush, the flush-on-cancel path, and the passive-quality emission. Each was verified to fail when its call site is removed; the passive emission in particular was wired but entirely unguarded until now. receivedTs is stamped when the agent's document arrives rather than when a session drains its queue, so it measures the adapter's receive moment instead of queue backlog and poll cadence. A resume snapshot reads the live inventory, so signals added by a reload during a pause are included. The live suites keep their self-skip for ordinary runs but honour EC_REQUIRE_LIVE, so a run that is meant to have infrastructure can no longer pass without it. The crate moves to edition 2024, which the implementation design has specified all along.
Brings the Diataxis set, README, DESIGN.md and AGENTS.md to current state: condition aggregation and its extras, the passive quality ladder and its tokens, the rejection rules for required observation fields, the element cap, the two delivery lanes, the single connectivity authority, the bounded shutdown, and receivedTs meaning payload arrival. Several claims were simply false and are replaced rather than annotated: DESIGN.md described streaming as future work, the tutorial pinned edition 2021, the coverage exclusion list omitted a suite, the shaping metric family went unmentioned, run_device was placed in the wrong module, and AGENTS.md still described orchestration as living in the excluded supervisor seam. The schema's staleSignalSecs description now covers both of its jobs -- the staleSignals metric threshold and the passive BAD-expiry bound. User-facing pages state present behavior only; change history is confined to a revision appendix per document.
Its decision register (D-R1..D-R16) now lives in DESIGN.md alongside the rest of the component's design rationale; the phase plan itself was scaffolding for the work, not a description of the adapter.
This suite self-skips without EC_MTC_AGENT, so it never ran while the remediation was built: its imports and signatures were carried forward blind while its assertions quietly went stale. Standing infrastructure up revealed five of six failing -- none of them a product defect, all of them the suite still describing behavior two phases had replaced. Ordinary observation flow arrives per-observation now, so the waits that expected a batched snapshot never matched. Where a re-baseline genuinely is owed the tests still demand one, and say which rung owes it: the post-resync view after a restart, and the recovery view after a buffer overrun. The restart case is now a real proof rather than a value match -- it asserts the resync view rebuilds the whole device, which is only true if the restart document published nothing before the re-probe. The tiny agent case proves the connectivity gate from the other side: connect is refused against a live, answering agent until acquisition delivers, which is exactly what a cached probe model no longer buys. Recorded while doing it: cppagent answers an overrun cursor with HTTP 400 rather than an OUT_OF_RANGE document, so the live machine recovers down the degradation floor instead of ladder 2.
LLD section 12's wire row had never been run. This gate builds a genuine EdgeCommons against a live EMQX, drives the real acquisition and publish path against the canonical agent, and decodes what lands on the bus with prost directly against the generated schema rather than through the library's own reader. It pins the things this remediation added: the sequence and receivedTs extras, conditionId and activeConditions, the passive marker, and the quality tokens for conditions, staleness and unreachability -- including UNAVAILABLE as a BAD null rather than a zero or an empty string. Two behaviors can only be proven with a live peer and are proven here. Two conditions active at once on one data item keep the signal at FAULT until the last FAULT clears, with the count riding alongside; and a paused agent drives held values through stale, expired and unreachable onto the broker, each carrying the value and sequence it is standing in for. The device fixture gains a condition data item and moves to the 2.3 namespace, because cppagent emits conditionId only from 2.3 -- without it the aggregate could not be observed at all. Documents that staleSignalSecs above twice the heartbeat never takes effect while streaming, since the link is declared unreachable first.
Runs the real binary as a real Linux process against the live cppagent and broker, and evidences the leg from the bus rather than from unit tests: the UNS topic grammar and envelope, the sb/* surface including the write refusal, the metric families and events, the condition fault reaching both its own signal and the signal bound to it, and the passive ladder degrading and recovering. It exists mostly for the two things no in-process test can reach. SIGTERM is a real signal, so the structured shutdown is observed end to end: teardown narrated and complete in 0.11s, the open batch window flushed with all four buffered samples landing on the broker, no unreachable alarm raised for a clean stop, and the process exiting on its own. The same holds with the broker frozen, and with both broker and agent frozen while acquisition sits in an HTTP long poll. The harness is a separate crate outside the component workspace, so it does not enter the component's own build, test or coverage gates.
…ss stop paths Running the platform legs against real infrastructure found three shipped artifacts that do not do what they claim. The image could not be built at all. The base pinned rustc 1.85 while the committed lockfile needs 1.88 for time and 1.86 for the icu and idna chain, so docker build failed outright. The floor comes from the locked graph, not from this crate's rust-version. The Kubernetes path served no metrics. The deployment maps a metrics port and the platform profile selects the prometheus target, but the crate never enabled the feature behind it -- so the target fell back to a log file that the mandated read-only root filesystem then refused, and the port answered nothing. The image now builds with the feature: the endpoint serves 53 families where it previously refused connections. On Greengrass the lifecycle shell was never replaced, leaving the binary a grandchild of the process the nucleus signals. Stopping a component could cut teardown short instead of letting it flush and exit; exec hands the signal to the process that has to act on it. Also carries the release-leg harnesses that found these, one per platform, and a dockerignore so a build context is not the whole target tree.
…harness The harness recipe now matches the shipped one, and writes its log outside the work directory the nucleus removes with the component -- the teardown narration is the thing that has to survive a removal.
Describes the three release-leg harnesses as part of the validation surface, and states plainly what they could not establish: the nucleus revokes IPC before signalling a component being removed, so updates in flight at that moment are dropped rather than retried, and a removal carrying a full batch window remains unexercised.
mbreissi
marked this pull request as ready for review
August 4, 2026 19:09
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.
Remediates the adversarial review of the MTConnect adapter. All 8 release-blocking findings, all 6 correctness defects, 2 further defects found during design, and 3 packaging defects found by the release legs are fixed. Every finding was independently verified against the code before any work started.
Release-blocking
Connectivity could report
ONLINEagainst a dead agent — permanently. A cached probe model was accepted as proof the agent had answered, andAgentDownwas broadcast only on the connected→disconnected edge, so a session attaching afterwards never learned. The device stayed ONLINE, alarm cleared, forever.AgentRuntimeis now the sole authority; a cached model is never liveness.Streaming failures neither marked down nor backed off. The establish-failure counter reset on a successful open, before any data arrived, so an agent answering with valid headers and an immediate EOF span an unbounded reconnect loop that never degraded to polling.
Condition activations collapsed onto one slot per data item. Clearing one activation promoted the signal to GOOD while another Fault was still asserted, and a mixed Fault/Warning settled on whatever the agent serialized last. Each data item now keeps a ledger keyed by activation identity and publishes the aggregate.
Observations published before model resynchronization. A document announcing a new
instanceIdwas decoded against the previous incarnation's model and published before the re-probe. Documents seen under a pending resync now defer entirely.Passive quality transitions were absent.
MTC_STALEexisted nowhere; staleness was counted into a metric and never published, so consumers held obsolete GOOD values indefinitely.The queue dropped loss-intolerant traffic. One delivery class meant a Fault condition was as droppable as a scalar sample. Two lanes now: coalescing for data, a reserved bounded lane for conditions and lifecycle.
Shutdown was unstructured. Device task handles were discarded at spawn; the stop request went into a queue a blocked task would never read. Structured cancellation with every handle joined under staged budgets totalling 12s.
The shaper mixed routing generations. A reload changing only a signal's route left its window open. Routing is part of policy identity now.
Correctness
Resume inventory frozen at connect ·
/currentreporting an HTTP-200MTConnectErrorsas a parse failure instead ofMTC_AGENT_ERROR:<code>· requiredsequence/timestampsilently defaulted to0/""· prefixedxmlns:*bypassing the 1.3 version floor · element count unbounded under the byte cap ·receivedTsstamped at queue drain rather than payload arrival.Found during this work, not in the review
The
publish.deadbandwas inert in production. Every delivery shipped labelled as a re-baseline, resetting the deadband's entry state before it could ever suppress anything. A configured, documented feature that did nothing.Attach snapshots recorded dedupe floors before delivery was known, so a dropped snapshot lost those observations permanently.
Found by the release legs — shipped artifacts that did not work
The
Dockerfilecould not build theCargo.lock. Base pinned rustc 1.85; the locked graph needs 1.88 (time) and 1.86 (icu/idna).docker buildfailed outright.The Kubernetes path served no metrics. The Deployment maps a metrics port and the platform profile selects the Prometheus target, but the crate never enabled the feature behind it — so it fell back to a log file the mandated read-only root filesystem refused, and the port answered nothing. Now serves 53 families.
The Greengrass lifecycle never replaced its shell, leaving the binary a grandchild of the process the nucleus signals. Proven causally: same artifact, only the Run-script form differing, gave
exitCode=143×3 withoutexecandexitCode=0×3 with it.Wire-visible changes
Additive only, within existing free-form fields — no core-library change, so no four-way parity work and no cross-language interop matrix. New sample extras
conditionId,activeConditions,passive; newqualityRawtokensMTC_STALE:<ageMs>andMTC_AGENT_UNREACHABLE; newMtconnectParse.rejectedObservationsmeasure.Consumers should note the cadence change: passive quality emits time-driven samples that no observation triggered, bypassing batch windows. They carry the
passiveextra so they can be filtered.Verification
Offline: 531 tests · clippy
-D warningsclean ·cargo fmt --checkclean (it failed at baseline) · coverage 97.50%, up from 96.75%, withdriver.rsbringing 1,648 previously-excluded lines into the denominator.Live matrix, all green:
prostlab-5950xexitCode=0in 67msThe cppagent suite was found failing 5 of 6 when infrastructure came up — all stale assertions, no product defect. It self-skips without
EC_MTC_AGENT, so seven phases edited it blind while its expectations rotted.EC_REQUIRE_LIVEnow makes that impossible.Proven on a live bus: concurrent Fault+Warning on one data item keeps the signal at FAULT until the last Fault clears, with
activeConditionstracking0→1→2→1→0; the passive ladder reaching the broker asMTC_STALE:3014→ expired →MTC_AGENT_UNREACHABLE; and shutdown flushing an open batch window onto the broker before exit.Known limits
staleSignalSecsabove2 × heartbeatMsnever takes effect while streaming — the link is declared unreachable first. With shipped defaults the expiry step is unreachable. Both outcomes are BAD; documented in the config reference.On Greengrass, the nucleus revokes IPC authorization several hundred milliseconds before signalling a component being removed, so updates in flight at that moment are dropped and not retried — the channel is already gone. Stopping a component does not do this. A
--removeteardown carrying a genuinely full batch window has not been exercised.Separately: a core-library defect this surfaced, not fixed here.
edgecommons::config::source::configmapfires"configuration reloaded"every ~15-20s against an unchanged mount — same..datasymlink target, samesha25625s apart — republishing the entirecfgpayload to the UNS each time and rebuilding the metric target. That needs its own change in the core repo.Design docs are synced in a companion PR on the core repo (
docs/mtconnect-adapter-remediation).