Driver health is published AFTER teardown, so GetHealth() can report Healthy with a disposed client #517

New Issue

2026-07-27T13:52:03-04:00

dohertj2 commented

2026-07-27 13:52:03 -04:00

Found in the MTConnect Task 9 code review (PR #506), deliberately not fixed there because MTConnect follows the existing fleet convention — fixing one driver alone would create inconsistency.

Symptom

GetHealth() can report Healthy while the driver's client is already disposed.

Cause

ShutdownAsync and the InitializeAsync failure path both await TeardownCoreAsync() — disposing the client and clearing state — before calling WriteHealth(...). A GetHealth() landing in that window observes the stale, pre-teardown health value.

This is a fleet-wide ordering pattern, not an MTConnect regression: ModbusDriver.cs:247-249 has the identical teardown-then-write sequence.

Suggested fix

A fleet-wide pass: write a transitional / Degraded health value before disposing, so the race window leans conservative rather than optimistic. Audit every driver for the same ordering while doing it.

Found in the MTConnect Task 9 code review (PR #506), deliberately not fixed there because MTConnect follows the existing fleet convention — fixing one driver alone would create inconsistency. ## Symptom `GetHealth()` can report **Healthy** while the driver's client is already disposed. ## Cause `ShutdownAsync` and the `InitializeAsync` failure path both `await TeardownCoreAsync()` — disposing the client and clearing state — **before** calling `WriteHealth(...)`. A `GetHealth()` landing in that window observes the stale, pre-teardown health value. This is a fleet-wide ordering pattern, not an MTConnect regression: `ModbusDriver.cs:247-249` has the identical teardown-then-write sequence. ## Suggested fix A fleet-wide pass: write a transitional / `Degraded` health value **before** disposing, so the race window leans conservative rather than optimistic. Audit every driver for the same ordering while doing it.

dohertj2 referenced this issue

2026-07-27 13:52:24 -04:00

feat(mtconnect): read-only MTConnect Agent driver (P1) — 23 tasks + 5-leg live gate #506

dohertj2 referenced this issue from a commit

2026-07-31 05:25:41 -04:00

fix(opcua,drivers): carry driver status sub-codes to clients (#519) + health-write ordering (#517)

dohertj2 closed this issue

2026-07-31 05:25:41 -04:00

dohertj2 referenced this issue from a commit

2026-07-31 05:25:41 -04:00

merge: close #517 (health-write ordering) and #519 (status sub-codes reach clients)

dohertj2 commented

2026-07-31 05:26:12 -04:00

Resolved — master 3207089f (commit 2d20aeb7).

The symptom was real; the mechanism in the issue was not

ShutdownAsync cannot race. DriverInstanceActor calls it from PostStop — actor thread, after the mailbox has drained — so the health poll is serialised against it. The one class with a lock-protected concurrent GetHealth, Core/Hosting/DriverHost, has no production call site at all (it is a "Phase 1 scaffold" for the Tier C isolation deleted in #522), and it removes the driver from its registry before calling shutdown anyway.

InitializeAsync can. It runs on a fire-and-forget Task.Run (DriverInstanceActor.cs:567), so the health-poll tick keeps calling GetHealth() throughout. MTConnectDriver writes Healthy, then starts its sample stream and probe loop — either can throw into a catch that disposes the client and only then writes Faulted. That is this issue's claim, made concrete, and it is the only reachable window in the fleet.

The suggested fix would have made it worse

write a transitional / Degraded health value before disposing

Applied as a move, that inverts the terminal state. Teardown drains in-flight poll/probe loops, and those loops write health as they unwind — so a late loop can overwrite the shutdown value with Healthy and leave that at rest, with every connection closed. The trailing write is load-bearing, not an oversight.

The correct shape is a bracket: conservative value before, terminal value after. Applied to MTConnect's init-failure path. The shutdown paths keep their single trailing write, since the window is not reachable there — and the rule plus the reachability table are now documented on IDriver.ShutdownAsync so the ordering is not "fixed" on the wrong premise later.

One unambiguous defect, deterministic rather than racy

CalculationDriver.GetHealth() returned a hard-coded new(DriverState.Healthy, …) that consulted nothing — Healthy before InitializeAsync, after ShutdownAsync, and after Dispose. No race required. Its existing test was named Health_is_always_Connected and asserted exactly that; removed and replaced with a lifecycle test.

Verification

The ordering test samples GetHealth() from inside the client's Dispose — every other test in that file asserts state once InitializeAsync has returned, by which time the window has closed, which is why the defect survived: the terminal value was always right and only the ordering was wrong. Falsified by deleting the pre-teardown write and confirming red.

Audited and not changed

The fleet-wide pass found the teardown-then-write shape in ~9 drivers' ShutdownAsync and in exactly one InitializeAsync catch block (MTConnect). The nine were left alone deliberately, per the reachability table above.

Resolved — master `3207089f` (commit `2d20aeb7`). ## The symptom was real; the mechanism in the issue was not **`ShutdownAsync` cannot race.** `DriverInstanceActor` calls it from `PostStop` — actor thread, after the mailbox has drained — so the health poll is serialised against it. The one class with a lock-protected concurrent `GetHealth`, `Core/Hosting/DriverHost`, has **no production call site at all** (it is a "Phase 1 scaffold" for the Tier C isolation deleted in #522), and it removes the driver from its registry *before* calling shutdown anyway. **`InitializeAsync` can.** It runs on a fire-and-forget `Task.Run` (`DriverInstanceActor.cs:567`), so the health-poll tick keeps calling `GetHealth()` throughout. `MTConnectDriver` writes `Healthy`, then starts its sample stream and probe loop — either can throw into a `catch` that disposes the client and only then writes `Faulted`. That is this issue's claim, made concrete, and it is the only reachable window in the fleet. ## The suggested fix would have made it worse > write a transitional / `Degraded` health value **before** disposing Applied as a *move*, that inverts the terminal state. Teardown drains in-flight poll/probe loops, and those loops write health as they unwind — so a late loop can overwrite the shutdown value with `Healthy` and leave **that** at rest, with every connection closed. The trailing write is load-bearing, not an oversight. The correct shape is a **bracket**: conservative value before, terminal value after. Applied to MTConnect's init-failure path. The shutdown paths keep their single trailing write, since the window is not reachable there — and the rule plus the reachability table are now documented on `IDriver.ShutdownAsync` so the ordering is not "fixed" on the wrong premise later. ## One unambiguous defect, deterministic rather than racy `CalculationDriver.GetHealth()` returned a hard-coded `new(DriverState.Healthy, …)` that consulted nothing — Healthy before `InitializeAsync`, after `ShutdownAsync`, and after `Dispose`. No race required. Its existing test was named `Health_is_always_Connected` and asserted exactly that; removed and replaced with a lifecycle test. ## Verification The ordering test samples `GetHealth()` from **inside the client's `Dispose`** — every other test in that file asserts state once `InitializeAsync` has returned, by which time the window has closed, which is why the defect survived: the terminal value was always right and only the ordering was wrong. Falsified by deleting the pre-teardown write and confirming red. ## Audited and not changed The fleet-wide pass found the teardown-then-write shape in ~9 drivers' `ShutdownAsync` and in exactly one `InitializeAsync` catch block (MTConnect). The nine were left alone deliberately, per the reachability table above.

dohertj2 referenced this issue

2026-07-31 09:54:31 -04:00

DriverInstanceActor.DisconnectObserved has no production sender — a driver whose device dies mid-run is never detected, never reconnects, and never reports comms loss #527

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#517