From bb83099300f76aa489440996bcd6880ad3b0b539 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:09:51 -0400 Subject: [PATCH] docs(review): close Driver.Sql-004/005/006/008/010 with resolutions and RED evidence --- code-reviews/Driver.Sql/findings.md | 20 ++++++------ .../SqlDriverTests.cs | 32 +++++++++++-------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/code-reviews/Driver.Sql/findings.md b/code-reviews/Driver.Sql/findings.md index a17eb89a..78d2e7a3 100644 --- a/code-reviews/Driver.Sql/findings.md +++ b/code-reviews/Driver.Sql/findings.md @@ -99,13 +99,13 @@ surface. | Severity | Low | | Category | Error handling & resilience | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:651` | -| Status | Open | +| Status | Resolved | **Description:** `HandlePollError` skips the connection class by testing `ex is DbException`, on the stated premise that "a subscribed-read outage surfaces as a DbException the reader threw, which ReadAsync has ALREADY classified". Not every unreachable-database throw is a `DbException`: `SqlPollReader.QueryAsync` raises `InvalidOperationException` when the provider factory returns no connection, and a provider may raise `ArgumentException`/`FormatException` from a malformed connection string on the same open path. For those, `ReadAsync` writes the good endpoint-bearing `LastError` and transitions the host to `Stopped`, rethrows, and the poll engine then routes the same exception here — where `Degrade` overwrites that message with the guidance-free "poll failed: TypeName" and the outage is logged a second time. The comment describes the intended behaviour; the type test does not implement it. **Recommendation:** Mark the exception as already-classified at the one place that knows (e.g. `ReadAsync` wrapping the throw, or a per-call flag) rather than inferring it from the exception type, so the reader's richer `LastError` survives. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed — `90f3016a`. `ReadAsync`'s catch now records the exception *instance* in a `_classifiedByRead` field before rethrowing, and `HandlePollError` defers only when `Interlocked.CompareExchange` matches that same object (consume-once); the `ex is DbException` type test is gone. A field rather than `Exception.Data` or a wrapper: `Data` is provider-owned and may be read-only, and wrapping would change the type `IReadable`'s callers and the poll engine see. TDD: `HandlePollError_forANonDbExceptionReadAsyncClassified_stillDefers_soTheGoodLastErrorSurvives` drives a `BreakableFactory` that hands back no connection (the reader's `InvalidOperationException` path) and was RED — the endpoint-bearing `LastError` was overwritten with "poll failed: InvalidOperationException". Its falsifiability control, `HandlePollError_forAnExceptionReadAsyncNeverSaw_degrades_evenWhenItIsADbException`, was RED against the type test too (a `DbException` the engine raised was silently swallowed), and pins that the marker is per-instance rather than per-type. The pre-existing DbException test was rewritten to drive the real ReadAsync→HandlePollError round trip instead of handing `HandlePollError` a bare exception. --- @@ -116,13 +116,13 @@ surface. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:641` | -| Status | Open | +| Status | Resolved | **Description:** `IsConnectionClass` treats `BadCommunicationError` as "the database did not answer", so a poll carrying it reports the endpoint `Stopped`. But that code is minted by `GroupResult.Failed`, which the reader raises only after the connection has opened — its own comment says "the connection opened, so the server is up — this query is what failed (an identifier that is not in the catalog, a lock, a permission)". A single group failing on a lock, a revoked `SELECT`, a schema change or an over-long `IN` list (see Driver.Sql-002) therefore reports the whole database endpoint as down. The two types disagree about what the same status code means. **Recommendation:** Either restrict the connection class to `BadTimeout` (a wedged socket is the case the host-state signal is for), or give a post-open query failure its own status so the two classifications cannot drift. Note the host state is currently only consumed through `IHostConnectivityProbe`, which is unwired (Gitea #521) — the operator-visible half today is the `LastError` wording and the Degraded verdict. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed — `ee1f1b28`. `IsConnectionClass` is now `BadTimeout` only, and `BadCommunicationError` moves to a new `IsQueryFailureClass` with its own branch in `ObservePollOutcome`: still Degraded (every tag in that group is Bad), but the host stays **Running** and the `LastError` points at the table/column, the grant and any lock rather than at the network. The published status code is unchanged — the drift was in what the driver *read into* it, not in what the reader emits, so the wire behaviour and the `GroupResult.Failed` contract stay put. The first recommendation was rejected on its own: restricting the class without adding the branch would have left an all-groups-failing driver reporting Healthy. TDD: `ReadAsync_whenTheQueryFailsAfterTheConnectionOpened_degradesButLeavesTheEndpointUp` drops the fixture's table out from under a driver whose catalog gate has already passed, and was RED (`LastError` blamed communication; the host went `Stopped`). `ReadAsync_whenEveryTagTimesOut_degradesHealthAndReportsTheHostStopped` is the standing control that a genuine no-answer outage still reports `Stopped`. --- @@ -133,13 +133,13 @@ surface. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:599` | -| Status | Open | +| Status | Resolved | **Description:** `ObservePollOutcome` advances `LastSuccessfulRead` only when at least one snapshot is Good. Several codes are proof the database answered without being Good: `BadNoData` (the query ran, the row is absent), `Uncertain`/`Bad` (a present row with a NULL cell) and `BadTypeMismatch` (a cell came back and would not coerce). A driver whose every tag reads one of those — a truncated staging table, a key set that was deleted — leaves `LastSuccessfulRead` frozen. That is benign on its own (health is correctly left unchanged, because these are authoring/data facts), but it interacts with the comms-loss watchdog: once anything has degraded the driver, the conjunction "non-Healthy AND `LastSuccessfulRead` frozen for 90 s" (CLAUDE.md, Gitea #527) trips against a database that is answering every poll on time, and the node force-reconnects a healthy connection. **Recommendation:** Advance `LastSuccessfulRead` on evidence-of-answer codes, not only on Good — the honest reading of "the database answered" the watchdog was designed around — while leaving the health state classification exactly as it is. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed — `a0a49d33`. A named `IsAnswerEvidence` predicate now decides the clock, separately from the health verdict: `Good`, `BadNoData`, `Uncertain`, `Bad`, `BadTypeMismatch` and `BadCommunicationError` (post-Driver.Sql-005 that one means the connection opened and the server refused the query — so the transport works and a forced reconnect could not fix it). Deliberately excluded, and each pinned by a control: `BadTimeout` (the database did not answer) and `BadNodeIdUnknown` / `BadConfigurationError` / `BadInternalError` (minted before any statement was executed — a blanket "not a timeout" rule would keep the clock fresh on a driver that never reached the database, disabling the watchdog rather than fixing it). The health *state* classification is untouched: an answered-but-not-Good poll advances the clock and reports the host Running, and changes nothing else. TDD: `ReadAsync_whenTheDatabaseAnswersWithNoRow_advancesTheEvidenceClock_withoutChangingHealth` (degrade first, then read a deleted key) was RED with a frozen clock, as were four of the six `ObservePollOutcome_everyEvidenceOfAnswerCode_advancesTheEvidenceClock` theory cases; the two negative theories and `ReadAsync_anUnresolvableRef_isNoEvidenceTheDatabaseAnswered_soTheEvidenceClockStands` were green throughout and are the falsifiability controls. --- @@ -167,13 +167,13 @@ surface. | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs:556` | -| Status | Open | +| Status | Resolved | **Description:** `SelectWideRow`'s remarks justify giving the `topByTimestamp` form the dialect's single-row limit on the grounds that "there the `ORDER BY` makes 'the first row' a defined answer". That holds only when the ordering column is unique. `ORDER BY ts DESC` plus a one-row limit over a column with ties returns an arbitrary row among the tied ones, and — unlike the where-pair form, whose whole argument for not taking the limit is that `Rows.Count > 1` is the evidence of ambiguity — the reader can never detect it, because the server has already discarded the other rows. The browse-commit default makes the tie shape reachable rather than theoretical: `SqlBrowseSession.PreferredTimestampColumn` will happily nominate a `date`-typed column (see Driver.Sql.Browser-001), which ties for every row written on the same day. **Recommendation:** State the tie caveat where the claim is made, and in `docs/drivers/Sql.md` alongside the `topByTimestamp` example: the model requires a selector that identifies one row, and a non-unique ordering column silently does not. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed — `9d4760fd`, **documentation only**. `SelectWideRow`'s over-claim ("the `ORDER BY` makes 'the first row' a defined answer") now reads that the `ORDER BY` fixes only *which* rows can be first, followed by an explicit caveat: with a non-unique ordering column the server returns an arbitrary row among the tied ones and this method can never detect it, because the discarded rows never reach it — there is no `Rows.Count > 1` left to warn on, unlike the where-pair form. `docs/drivers/Sql.md` carries the same warning beside the `topByTimestamp` example, naming the reachable shape (a `date`-typed column ties for every row written on the same day, and browse-commit will nominate one) and the fix (a column unique per row, or a latest-row view). Behaviour deliberately unchanged: the entry argues for the doc, and the only behavioural remedy — a tie-break column in the emitted `ORDER BY` — lives in `SqlGroupPlanner`, which is out of scope here and would need an authored notion of what breaks the tie. --- @@ -201,10 +201,10 @@ surface. | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:675` | -| Status | Open | +| Status | Resolved | **Description:** Every health transition is a read-modify-write over a `Volatile`-accessed field with no compare-and-swap: `Degrade`, `TouchLastSuccessfulRead` and `SetHealthUnlessFaulted` each `ReadHealth()`, build a new record and `WriteHealth()` it. Concurrent poll loops (one per subscription, plus an ad-hoc `ReadAsync`) can interleave between the read and the write, so a `LastSuccessfulRead` advance can be lost, or a `Faulted` verdict recorded by a concurrent reinitialize can be overwritten by a stale non-Faulted snapshot despite the guard — the guard reads the field before the write rather than atomically with it. Impact is limited to the health surface (a stale timestamp or state until the next poll), which is why this is Low. **Recommendation:** Use `Interlocked.CompareExchange` in a small retry loop for the read-modify-write helpers, or funnel health mutation through a lock — the `_probeLock` the host-state transition already uses would do. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed — `336e79ea`. Every poll-driven transition now goes through one `UpdateHealth(Func)` compare-and-swap retry loop: `Degrade` and the new `RecordHealthyRead` via `UpdateHealthUnlessFaulted` (so the Faulted guard is evaluated against the *same* snapshot the CAS swaps out, closing the "guard reads before the write" half), `TouchLastSuccessfulRead` directly. Two details are load-bearing. The transform is re-run per attempt and must derive its timestamp from the clock rather than from the call site — the Healthy write previously computed `DateTime.UtcNow` before the call, which a retry would republish *stale* and rewind the clock; that is why the Healthy path became `RecordHealthyRead` instead of passing a prebuilt record. And `WriteHealth` survives, unconditional, for the Initialize/Shutdown lifecycle writes that are last-writer-wins by definition — a Volatile write racing a CAS loop only makes the loop retry and re-read, which is strictly safer than before. TDD: `HealthTransitions_fromConcurrentPolls_neverRewindTheEvidenceClock` runs an advancing and a degrading `ObservePollOutcome` loop against one driver while an observer watches `LastSuccessfulRead`; monotonicity is the observable a lost update violates (a `Degrade` that read the clock at t0 republishes it after an advance to t1). RED 3/3 runs before the fix, green 3/3 after. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs index c059e476..492cc05d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs @@ -381,21 +381,26 @@ public sealed class SqlDriverTests using var done = new CancellationTokenSource(); var rewinds = 0; - var observer = Task.Run(() => - { - var highWater = driver.GetHealth().LastSuccessfulRead ?? DateTime.MinValue; - while (!done.IsCancellationRequested) + var token = TestContext.Current.CancellationToken; + var observer = Task.Run( + () => { - var seen = driver.GetHealth().LastSuccessfulRead ?? DateTime.MinValue; - if (seen < highWater) Interlocked.Increment(ref rewinds); - else highWater = seen; - } - }); + var highWater = driver.GetHealth().LastSuccessfulRead ?? DateTime.MinValue; + while (!done.IsCancellationRequested) + { + var seen = driver.GetHealth().LastSuccessfulRead ?? DateTime.MinValue; + if (seen < highWater) Interlocked.Increment(ref rewinds); + else highWater = seen; + } + }, + token); var writers = new[] { - Task.Run(() => { for (var i = 0; i < iterations; i++) driver.ObservePollOutcome([advancing]); }), - Task.Run(() => { for (var i = 0; i < iterations; i++) driver.ObservePollOutcome([degrading]); }), + Task.Run( + () => { for (var i = 0; i < iterations; i++) driver.ObservePollOutcome([advancing]); }, token), + Task.Run( + () => { for (var i = 0; i < iterations; i++) driver.ObservePollOutcome([degrading]); }, token), }; await Task.WhenAll(writers); @@ -581,7 +586,7 @@ public sealed class SqlDriverTests var health = driver.GetHealth(); // Untouched: HandlePollError left the exception to ReadAsync, so it neither re-degraded nor leaked… - health.LastError.ShouldBe(classified.LastError); + health.LastError.ShouldNotBeNull().ShouldBe(classified.LastError); health.LastError.ShouldNotContain(SecretToken); // …and it logged nothing (ReadAsync owns the single outage warning). logger.Entries.Count(e => e.Level == LogLevel.Warning).ShouldBe(warningsAfterRead); @@ -618,7 +623,8 @@ public sealed class SqlDriverTests driver.HandlePollError(thrown); var health = driver.GetHealth(); - health.LastError.ShouldBe(classified.LastError); // the richer message survived… + health.LastError.ShouldNotBeNull() + .ShouldBe(classified.LastError); // the richer message survived… health.LastError.ShouldNotContain("poll failed"); // …and was not overwritten by the poor one logger.Entries.Count(e => e.Level == LogLevel.Warning).ShouldBe(warningsAfterRead); }