From d3be5f052784de7a2d8ad541340766d0d0324313 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 18 Aug 2026 09:18:43 -0400 Subject: [PATCH] =?UTF-8?q?test(communication):=20close=20register=20row?= =?UTF-8?q?=20102=20=E2=80=94=20serialize=20the=20two=20gauge-moving=20cla?= =?UTF-8?q?sses=20so=20the=20subscribers.active=20baseline=20holds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gauge test polls baseline+1, but the process-global gauge's baseline is captured once: a parallel class's streams closing after capture lowers the gauge below baseline+1 forever. Survey confirmed only SiteStreamGrpcServerTests and CleanupVerificationTests drive the real SubscribeInstance handler that moves the counter; both now share the SiteStreamSubscriberGauge collection (row-96 pattern). 4/4 full-suite runs green, durations unchanged. --- docs/plans/2026-07-08-deferred-work-register.md | 2 +- .../Grpc/CleanupVerificationTests.cs | 4 ++++ .../Grpc/SiteStreamGrpcServerTests.cs | 14 +++++++++++--- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/docs/plans/2026-07-08-deferred-work-register.md b/docs/plans/2026-07-08-deferred-work-register.md index 7a1c3cdd..aba0e55b 100644 --- a/docs/plans/2026-07-08-deferred-work-register.md +++ b/docs/plans/2026-07-08-deferred-work-register.md @@ -94,7 +94,7 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round | 99 | ~~**CLI `audit query --status` client-side validation is case-sensitive**~~ **RESOLVED 2026-08-17 (merge `152ec85e`, agent commit `f33f25a1`) — premise corrected: "the server would accept it" was FALSE, and removing the client gate alone would have been a regression.** The server-side parser (`AuditQueryParamParsers.ParseEnumList`) is documented-and-tested **lax** — it silently drops unparseable values and collapses to null (filter unconstrained), i.e. exactly the row-46/88 silent-wrong-answer trap. Fix shipped both halves: **seven** `AcceptOnlyFromAmong` gates removed across `audit query`/`audit export` (`--channel`/`--kind`/`--status` on both, plus `export --format`, which rejected `CSV` the server lowercases anyway), valid sets moved to help text, and a new **strict** `ParseEnumListStrict` (throws naming the valid values; `LooksNumeric` guard rejects ordinals) wired into both ManagementService audit endpoints. The lax parser deliberately survives for the CentralUI drill-in/export query strings (bookmarked fragments must render, not fault — documented as non-reconcilable). Left alone with reasons in code: `--direction up|down` (no server parse to defer to), `--format json|table` (CLI-local), NotificationCommands' literals (unsurveyed). Four pre-existing tests were passing *vacuously* (no-credentials exit codes) and now pin the real contract; fail-before 21/26. **Found-and-fixed follow-on (orchestrator, `16056f8e`):** row 88's own `ParseDeploymentStatus` accepted ordinals (`--status 2` → InProgress) — same `LooksNumeric` guard applied + 3 tests. **Live rig probes (post-redeploy):** `audit query --status delivered` returns rows; `--status Nonsense` and `--status 3` both fail loud naming the 8 valid values; `deploy status --status 2` fails loud naming the 4 valid values. | Found by the 2026-08-17 rows-37/86-88 fix session | Closed 2026-08-17. | — | | 100 | ~~**`SiteScriptCompileCache.Clear()` is process-global — test classes wipe each other's entries**~~ **RESOLVED 2026-08-17 (merge `8a9a8d29`, agent commit `84b405ff`) — premise corrected: there were ZERO production callers of `Clear()`** (the only production entry point is `ScriptCompilationService.CompileCore` → `GetOrAdd`; `Clear()` existed solely for tests), so neither register option was taken as written — `Clear()` was **deleted outright** and the cache became an instance-scoped `sealed class` owned by `ScriptCompilationService`. The N1 "one compile per body per process" property now rests on `AddSiteRuntime` registering that service as a **DI singleton** — a lifetime that became load-bearing, so new `SiteRuntimeServiceRegistrationTests` pins it (a Scoped/Transient downgrade would reintroduce WP3.1's recompile storms with no behavioral test failing). The 37 test sites doing `new ScriptCompilationService(logger)` each silently get their own cache — isolation for the whole assembly, not just the row-91 classes. Row-91 workarounds RETIRED, not preserved: serialized collection removed from all three classes, exact `Hits`/`Count` assertions restored, guid key token gone, `ForeignEntrySlack` 512→8; new `SeparateCaches_DoNotShareEntriesOrCounters` pins the scoping. SiteRuntime.Tests green three consecutive full-parallel runs. | Found by the 2026-08-17 row-91 fix session | Closed 2026-08-17. | — | | 101 | ~~**`ApplyingArtifacts_RemovedDataConnection_StopsLiveDclConnection` flaked once — suspected overlay-sweep read racing the artifact-apply write**~~ **RESOLVED 2026-08-17 (`0d3a90a9`) — hypothesis VERIFIED, and the production consequence was WORSE than the row guessed.** The race is real: the snapshot is read off the actor thread but compared against the overlay only when the piped message lands, so a read starting before an artifact apply's store writes can land after `HandleApplyArtifactDataConnectionsToDcl` populated the overlay — its stale (even empty) snapshot then "proves" the just-applied names disappeared. Overlay iteration order explains the exact fingerprint (`PlcA` removed first). Two corrections to the row's guesses: (1) it is NOT "self-healing at the next tick" — the removal evicts the hash slot the overlay's push is gated on, so the next fresh sweep clears the tombstone but re-creates nothing; the stopped connection stays dead **until an instance redeploy**; (2) the mid-write "momentarily not seeing the row" framing was the lesser half — the whole-snapshot staleness (read-before-write, land-after-apply) is the dominant window. Fix: `_definitionsWriteGeneration`, bumped on the actor thread by every artifact apply and captured at every snapshot-read dispatch (boot load + sweep); a result carrying a stale generation is discarded WHOLE — additions included, since a stale row pushed at a live connection would reconfigure it backward and flip-flop with the next fresh read. Deterministic repro fails before the fix (`StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections`); `FreshEmptySweepSnapshot_StillRemovesDisappearedConnections` pins that row-90's legitimate disappearance still fires. SiteRuntime.Tests 647/647; rig redeployed, zero spurious removals/discards at boot, all rig-probe-1 tags Good. Memo §10 records the mechanism. **Postscript 2026-08-18 (merge `505f3569`, agent commit `f02e1166`): extended to the WHOLE snapshot.** The instance-config half had the same TOCTOU, and all three hazards were PROVEN real pre-fix: a stale sweep restarts an instance **backward** (`TryRestartStaleInstance` feeds the snapshot's old `ConfigJson` to `HandleDeploy` — a literal old-config redeploy; pipeline ownership only covers in-flight deploys and the old revision was never claimed), a read-pre-delete/disable **resurrects** a just-stopped instance (delete even clears the sweep claim), and the boot path skews `_deployedInstanceNames`. Fix: one `_storeWriteGeneration` over both tables, bumped at BOTH ends of every actor-thread config mutation (deploy persist, enable, disable, delete, rollback, artifact apply) — both ends because `ApplyDeployment`/`HandleDisable`/`HandleDelete` advance actor state BEFORE the off-thread write, so a completion-only bump (the row-101 original argument, which holds only for the artifact apply) leaves a window. Stale sweep discarded whole; stale boot load re-dispatched, capped at 5 consecutive (then applied with a Warning). CDC/`SiteReconciliationActor` writes deliberately never bump — discovering them is the sweep's purpose. 3 new fail-before tests + 2 fresh-generation guards pinning that row-72/78 legitimate repairs survive; SiteRuntime.Tests 652/652 x3; full suite green (one unrelated Communication.Tests gauge flake → row 102); rig probe: disable/enable/redeploy churn over multiple sweep ticks produced zero self-heal events on either site-a node, tags Good. Memo §11. | Observed by the 2026-08-17 row-100 fix session | Closed 2026-08-17; whole-snapshot extension 2026-08-18. | — | -| 102 | **`SiteStreamSubscribersActiveGauge_GoesToOneOnConnect_AndBackToZeroOnCancel` is delta-hardened but still flakes — the baseline can't survive other contributors CLOSING** *(Low, test-infra)* | Observed 2026-08-18 (one full-suite failure, `Expected: 4, Actual: 1`; 3/3 green in isolation) | The test already polls `baseline + 1` (a prior hardening pass), but `scadabridge.site.stream.subscribers.active` is a process-global observable gauge and the baseline is captured ONCE: capture while 3 parallel classes' streams are up → they close → the gauge legitimately settles at this test's own 1 and never reaches `baseline+1=4`. A fixed-baseline delta only survives contributors that hold constant or increase — the row-91 pattern's blind side. Fix options, in order of preference: serialize the stream-opening test classes in Communication.Tests into one collection (row-96 pattern; needs a survey of which classes spin `SiteStreamGrpcServer`), or make the gauge observable per-server-instance so the test can read its own. Weakening to `>= 1` loses the balanced-close half of the assertion. | Next Communication.Tests touch, or a second occurrence of this fingerprint | +| 102 | ~~**`SiteStreamSubscribersActiveGauge_GoesToOneOnConnect_AndBackToZeroOnCancel` is delta-hardened but still flakes — the baseline can't survive other contributors CLOSING**~~ **RESOLVED 2026-08-18 via the preferred option (row-96 pattern).** The survey found the gauge moves ONLY inside the real `SiteStreamGrpcServer.SubscribeInstance` handler, and exactly two test classes drive it: `SiteStreamGrpcServerTests` (the gauge test's own class) and `CleanupVerificationTests` — `GrpcOptionsWiringTests` instantiates the server but never opens a stream, and the client/aggregator suites subscribe against fakes, so none of those can perturb the counter. Both drivers now share the `[Collection("SiteStreamSubscriberGauge")]` xunit collection (comments on both classes + at the baseline poll explain why the relative read alone was insufficient), so no other contributor can close a stream between the baseline capture and the assertions. Verified 4/4 consecutive full-suite runs green (717/717), durations unchanged (22–25 s vs the prior 24 s) — the serialization is free. Original evidence: the test polls `baseline + 1`, but the process-global gauge's baseline is captured ONCE; capture while parallel classes' streams are up → they close → the gauge settles at this test's own 1 and `baseline+1` is unreachable forever — a fixed-baseline delta only survives contributors that hold constant or increase. | Observed 2026-08-18 (one full-suite failure, `Expected: 4, Actual: 1`; 3/3 green in isolation); fixed same day | Closed 2026-08-18. | — | | 68 | ~~**`deploy.sh` run from a git worktree silently rehomes every node's bind-mount data dir**~~ **RESOLVED 2026-08-15 (guard added in the same close-out).** The compose files' bind mounts are relative (`./site-a-node-a/data`), so a deploy from a worktree checkout mounts the worktree's copies — the rig keeps running and looks healthy, but its SQLite stores split from the main checkout's and everything written there is orphaned by the next deploy from main. Bit the rows-46–53 close-out live: the seeding agent's `rig-probe-1` deploy persisted into worktree-homed dirs and "vanished" after the orchestrator's redeploy (no tombstone, no product bug — LWW/tombstone forensics ruled out deletion; the row sat in `.claude/worktrees/…/docker/site-a-node-a/data/site-localdb.db` the whole time). Both `docker/deploy.sh` and `docker-env2/deploy.sh` now refuse to run from a `.claude/worktrees/` path (override: `SCADABRIDGE_ALLOW_WORKTREE_DEPLOY=1`). | Found during the rows-46–53 close-out | Closed 2026-08-15. | — | ## Resolved (verified against the code 2026-07-10) diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/CleanupVerificationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/CleanupVerificationTests.cs index ab28f93a..3606eee7 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/CleanupVerificationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/CleanupVerificationTests.cs @@ -11,6 +11,10 @@ namespace ZB.MOM.WW.ScadaBridge.Communication.Tests.Grpc; /// Verifies that after a gRPC stream is cancelled, the SiteStreamManager /// subscription is properly cleaned up with no leaked subscriptions. /// +// Serialized with SiteStreamGrpcServerTests (register row 102) — this class's real +// SiteStreamGrpcServer streams move the process-global subscribers.active gauge that +// SiteStreamSubscribersActiveGauge_GoesToOneOnConnect_AndBackToZeroOnCancel baselines. +[Collection("SiteStreamSubscriberGauge")] public class CleanupVerificationTests : TestKit { [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/SiteStreamGrpcServerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/SiteStreamGrpcServerTests.cs index 84c870e1..949e6218 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/SiteStreamGrpcServerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/Grpc/SiteStreamGrpcServerTests.cs @@ -11,6 +11,12 @@ using ZB.MOM.WW.ScadaBridge.Communication.Grpc; namespace ZB.MOM.WW.ScadaBridge.Communication.Tests.Grpc; +// Serialized with CleanupVerificationTests (register row 102): these are the only two +// classes whose streams move the process-global scadabridge.site.stream.subscribers.active +// gauge, and the gauge test below captures its baseline ONCE — a parallel class's streams +// CLOSING after that capture makes baseline+1 unreachable (fixed-baseline deltas only +// survive contributors that hold or increase). Same collection ⇒ never concurrent. +[Collection("SiteStreamSubscriberGauge")] public class SiteStreamGrpcServerTests : TestKit { private readonly ISiteStreamSubscriber _subscriber; @@ -481,9 +487,11 @@ public class SiteStreamGrpcServerTests : TestKit await WaitForConditionAsync(() => server.ActiveStreamCount == 1); - // While the stream is up the gauge is one above whatever baseline other - // (possibly parallel) tests left behind — read relative so the assertion - // is robust to test interleaving on the process-wide static counter. + // While the stream is up the gauge is one above the captured baseline. The + // relative read alone is NOT enough on a process-wide counter — a parallel + // class's streams closing after the capture lowers the gauge below + // baseline+1 forever (register row 102) — so the classes that move this + // gauge are serialized into the SiteStreamSubscriberGauge collection. // // SiteStreamSubscriberOpened() runs AFTER the _activeStreams insertion that // WaitForConditionAsync(ActiveStreamCount == 1) keys off (see