docs(plans)+docs(register): 2026-08-19 Medium-findings remediation plan (all 17 tasks executed); reopen+re-close register rows 95/99 with the P8-6/P6-3 defect proofs; add row 103 (deferred P6-6 lesser instances); CLAUDE.md — bulk deploy skips Disabled, LocalDb 0.3.2
This commit is contained in:
Joseph Doherty
@@ -92,6 +92,7 @@ spec for each is `docs/requirements/Component-<Name>.md`, and `README.md` carrie
|
||||
- **Batching is by BYTE BUDGET as of LocalDb 0.2.0** — `LocalDb:Replication:MaxBatchBytes` (default **2 MB**, sized under the 4 MB gRPC cap) bounds a delta/snapshot message by summed serialized size via a per-message split in `SyncSession.PumpLoopAsync`, with `MaxBatchSize` demoted to a secondary row cap; a single row over budget is sent alone rather than stalling the stream. **The rig's old `MaxBatchSize = 16` pin (a hand-computed byte-budget proxy: ~70 KB worst-case `config_json` x the 500 default is ~35 MB) is retired, but `MaxBatchSize` is NOT fully redundant with `MaxBatchBytes`** — it also bounds the separate DB READ page in `OplogStore.ReadBatchAboveAsync`/`SnapshotStreamer`, which materializes the whole page into memory *before* the byte-budget split runs, so an unset (500-default) `MaxBatchSize` still lets a reconnect drain transiently allocate ~35 MB per read even though every wire message stays under `MaxBatchBytes` (arch-review adversarial finding F2). Both site-a nodes on `docker/` therefore pin an explicit `"MaxBatchSize": 64` to bound that transient allocation, while `MaxBatchBytes` stays unset (its 2 MB default) to bound the wire message; site-b/site-c stay unreplicated so the key doesn't apply there.
|
||||
- **CDC registration is conditional, and both directions self-heal at boot** (arch-review WP1.3 + WP3.3, `Host/SiteLocalDbSetup.cs`). Capture triggers are installed only when this node has replication configured — `PeerAddress` **or** `ApiKey`, an OR because only the dialling half sets `PeerAddress` while the passive half carries the key alone. An unreplicated node calls **`DeregisterReplicated`** on all ten tables at boot, dropping triggers an earlier build left behind and pruning their oplog/row-version rows (idempotent; logs once at Information when something was actually cleaned). A replicating node registers with **`baselineExistingRows: true`**, which seeds `__localdb_row_version` for pre-existing rows at the LWW floor (HLC `0`, this node's id) and flags a snapshot resync — so turning replication ON for a site that has been running without it now converges on the rows already in the file instead of only on writes made after the restart. Deregistration must be symmetric (the handshake compares registered-table digests fail-closed), so replication is a both-nodes-together change in either direction. LocalDb 0.2.0 also makes backlog depth O(1) and drops the unused `__localdb_oplog_hlc` index; the on-disk bookkeeping schema goes to v2, upgraded in place on open, with no wire change (0.1.x peers still sync).
|
||||
- **LocalDb 0.3.0 (2026-08-15) makes that snapshot flag PER-TABLE** — the residual 0.2.1 explicitly deferred. On-disk bookkeeping schema goes to **v3** (a new `__localdb_snapshot_state` table, upgraded in place on open); still wire-compatible, and `lib_schema_version` deliberately stays `1` because the handshake compares it fail-closed for *equality*. The extension is negotiated by **capability** (`Handshake.supports_partial_snapshot`, `SnapshotBegin.tables` where **empty == all tables**), so a peer that does not advertise it is served a full snapshot — a scoped one would make it advance `last_applied_remote_seq` past deltas it never received. For the same reason **a partial snapshot moves no watermark on either side**; the overlapping deltas are still pumped and absorbed by LWW. Snapshot debt now clears on the receiver's new `SnapshotAck` rather than on enqueue, so a crash mid-snapshot re-streams the same scope. **`SiteLocalDbSetup.cs` needed no restructuring** — it already calls `RegisterReplicated` once per table in a loop, which is exactly what per-table flagging keys off. The consequence to know: **adding an eleventh table to `ReplicatedTables` on an already-replicating site now snapshots that one table**, where through 0.2.x the same edit re-streamed all eleven in full in both directions. First-boot behaviour is unchanged (all ten seed at once ⇒ the flagged set is every registered table ⇒ an ordinary full snapshot), and so is upgrading the rig in place (the ten tables are already ledgered, nothing seeds, nothing is flagged). A future "register the whole list in one call" API would silently give the coarse behaviour back.
|
||||
- **LocalDb 0.3.2 (2026-08-19, ScadaBridge arch-review P4-1 + P4-3; wire-compatible, no schema change):** (1) the sync pump now **faults the live session when a cap-prune invalidates its position** (horizon check on the oplog's min seq before each read batch + on the batch head after it; throws the new public `LocalDbOplogGapException` through the 0.3.1 fault-teardown) — previously a slow-but-alive peer silently skipped the pruned gap and the pair diverged until the next reconnect; (2) **transport keepalives both sides** — client `SocketsHttpHandler` pings via new validated `LocalDb:Replication:KeepAlivePingDelay`/`KeepAlivePingTimeout` (defaults 60s/20s, `Always` policy), and ScadaBridge's site gRPC listener sets Kestrel h2 keepalive (`Host/SiteKestrelSetup.cs` — server-wide, also governs the metrics port); (3) the server's single-stream guard is now **newest-wins with an awaited handover** — a fresh authenticated `Sync` cancels a dead/wedged incumbent and *waits* for it to finish (`TakeoverTimeout` 10s) before starting, so a half-open link can no longer wedge the pair behind `AlreadyExists` while the zombie side reports `LocalDbReplicationConnected=true`.
|
||||
- **LocalDb 0.2.1** (two adversarial-review findings against 0.2.0, wire-compatible, no schema change) fixes: (1) `DeregisterReplicated` was deleting the HLC clock's only durable crash anchor along with the table rows — an ungraceful exit (crash, SIGKILL, power loss) on a fully-deregistered file left nothing to recover from, so the reopened clock could restart from wall-clock time and re-issue stamps below HLCs peers already hold (silently discarded by LWW); the current clock is now flushed inside the deregistration transaction, before the prune. (2) The sync inbox was an **unbounded** channel, so inbound memory was a function of what the peer sends — a mutual full snapshot (both sides of a pair baseline unconditionally when replication is enabled late) buffered whole, in memory, on both nodes at once; it is now **bounded at 64 messages** with wait-mode backpressure (unrelated to, and not to be confused with, the rig's separate `MaxBatchSize: 64` DB-read-page pin above), which required moving the snapshot to run alongside the receive loop (not inside the pre-loop handshake) so the inbox is drained for the whole session and can't deadlock itself.
|
||||
- All timestamps are UTC throughout the system.
|
||||
- Inter-cluster communication uses **three** transports, not two — **all cross-cluster command/control and data now rides gRPC** after the ClusterClient→gRPC migration's Phase 4 (`docs/plans/2026-07-22-clusterclient-to-grpc-plan.md`) deleted Akka `ClusterClient`/`ClusterClientReceptionist`: (1) **gRPC command/control** — site→central over the central-hosted `CentralControlService` (`GrpcCentralTransport`, sticky central-a→central-b channel pair; deployments/notifications/health/heartbeat/audit-ingest/reconcile), and central→site over the site-hosted `SiteCommandService` (`GrpcSiteTransport`, per-site NodeA→NodeB channel pair; the 28 lifecycle/OPC-UA/query/parked/route/failover commands); (2) **gRPC** server-streaming for real-time data (attribute values, alarm states, `SiteStreamService`); and (3) **plain token-gated HTTP** for the deployment config itself — notify-and-fetch, the site pulls the config from `DeploymentConfigEndpoints` (`ManagementService/DeploymentConfigEndpoints.cs`) with an `X-Deployment-Token` header, `AllowAnonymous` with the per-deployment token as the entire security boundary. The gRPC boundary is per-site PSK-authenticated (`ControlPlaneAuthInterceptor`, unchanged). There is **no receptionist registration** — discovery is by dialling configured endpoints; central builds one `SitePairChannelProvider` per site (addresses from `Site.GrpcNodeAAddress`/`GrpcNodeBAddress`, refreshed from the DB every 60s and on admin changes), sites dial `ScadaBridge:Communication:CentralGrpcEndpoints` (both central nodes, h2c on `CentralGrpcPort` 8083, **NOT** via Traefik). **Discovery is asymmetric by design:** central discovers site gRPC addresses from the *database* (refreshable at runtime), sites discover central from *appsettings* (`CentralGrpcEndpoints`, static — restart required; `StartupValidator` requires a Site node to list at least one). `Akka.Cluster.Tools` stays for ClusterSingleton; only the ClusterClient part is gone. **Central never buffers for an unreachable site** — the send fails with the caller's Ask/deadline timing out; a `ConnectionStateChanged` mechanism built for this was deleted as dead code.
|
||||
@@ -124,7 +125,7 @@ spec for each is `docs/requirements/Component-<Name>.md`, and `README.md` carrie
|
||||
- Flattened configs include a revision hash for staleness detection.
|
||||
- Deployment identity: unique deployment ID + revision hash for idempotency.
|
||||
- Per-instance operation lock covers all mutating commands (deploy, disable, enable, delete).
|
||||
- **Bulk site deployment (`DeploymentService.DeploySiteAsync`, arch-review WP2.5)** redeploys every out-of-date instance at a site through one flatten session (a template chain shared by N instances is walked once) in three phases — serial Prepare, bounded-parallel Send (`SiteDeploymentMaxParallelism`, default 4), serial Finalize — still **not all-or-nothing**: any one instance's failure doesn't block the rest. The `PendingDeployment` staging row (5-minute TTL) is written per-instance immediately before that instance's round-trip in the Send phase, not upfront in Prepare, so a large batch's tail instances don't have their TTL clock already half-spent by the time they're reached.
|
||||
- **Bulk site deployment (`DeploymentService.DeploySiteAsync`, arch-review WP2.5)** redeploys every out-of-date instance at a site through one flatten session (a template chain shared by N instances is walked once) in three phases — serial Prepare, bounded-parallel Send (`SiteDeploymentMaxParallelism`, default 4), serial Finalize — still **not all-or-nothing**: any one instance's failure doesn't block the rest. **Disabled instances are SKIPPED by bulk deploy as of 2026-08-19 (arch-review P7-2)** — they are reported in the summary's additive `SkippedInstances` list, never flattened/sent/re-enabled (single-instance deploy remains the explicit operator path that deploys+enables a Disabled instance); Prepare's per-instance reconcile also runs under a short `SiteDeploymentReconcileTimeout` (5s) with a once-per-batch unreachable-site circuit-breaker, so a down site no longer costs 30s × N serial before the first send (P7-3). The `PendingDeployment` staging row (5-minute TTL) is written per-instance immediately before that instance's round-trip in the Send phase, not upfront in Prepare, so a large batch's tail instances don't have their TTL clock already half-spent by the time they're reached.
|
||||
- Site-side apply is all-or-nothing per instance.
|
||||
- System-wide artifact version skew across sites is supported.
|
||||
- Last-write-wins for concurrent template editing (no optimistic concurrency on templates).
|
||||
|
||||
@@ -87,14 +87,15 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round
|
||||
| 92 | **The event-log cap's size model is sample-based and blind to mid-history width shifts** *(Low, accuracy limitation — documented in memo §3.1)* | Row-81 fix session (2026-08-17) | Row width is sampled 500 rows from each END of `site_events`; a stream whose row width changes sharply mid-history is mis-sized until the trim reaches that region. Self-correcting per tick (each trim exposes new tail rows to the sample), and the model's deliberate +8–13% high bias absorbs modest shifts, but a pathological width profile could over- or under-trim for several ticks. **Reviewed 2026-08-18: accepted by design** — inherent to any sampled model, not closable by code short of the row-94 `dbstat` replacement; the row-93 schema pin does not (and cannot) cover it. | First field report of the cap trimming visibly wide of its target, or the row-94 `dbstat` replacement (which retires this row with the model) |
|
||||
| 93 | ~~**The cap model's page-layout constants are empirical for the CURRENT `site_events` schema**~~ **RESOLVED 2026-08-18 — the silent trap is now loud.** `SiteEventsSchemaCalibrationTests` (SiteEventLogging.Tests) pins the exact shape the calibration assumed: the eight-column list the sampler's `octet_length` sum covers, the index set as exactly the four secondary indexes `IndexedColumnCount = 4` charges plus the PK autoindex (absorbed into the measured fill factors), and each index's single covered column. Any `site_events` schema migration — new column, fifth index, composite widening — now fails the suite with a message directing to the memo §4 recalibration and the constants + sampling-SQL update, instead of silently mis-sizing the cap. Suite green 110/110. | Row-81 fix session (2026-08-17); pinned by test 2026-08-18 | Closed 2026-08-18. | — |
|
||||
| 94 | **`SqliteCapabilityTests` is a deliberate tripwire — do not "fix" it by weakening the assertion** *(Low, standing note)* | Row-81 fix session (2026-08-17) | The test FAILS BY DESIGN if a future SQLite bump ships `dbstat`/`sqlite_dbpage` support. The correct response at that point is to REPLACE the sampled page-layout model with exact `dbstat` measurement (retiring row 92 — and the row-93 schema pin `SiteEventsSchemaCalibrationTests`, added 2026-08-18, goes with the model too), not to relax the test. The test's own comment says the same. | `SqliteCapabilityTests` fails after a SQLite/provider bump |
|
||||
| 95 | ~~**The leaked 100 ms-tick test-actor pattern is systemic — three more classes carry it**~~ **RESOLVED 2026-08-17 (merge `106e467d`, agent commit `34a8079d`).** All three classes live in AuditLog.Tests (premise correction: none in SiteCallAudit.Tests). `Watch`/`Stop`/`ExpectTerminated` replicated from `da23f2dc` at every creation site: 11 in `AuditLogPurgeActorTests`, 10 in `SiteAuditReconciliationActorTests`, 2 in `OutageReconciliationTests`. Second premise correction: `OutageReconciliationTests` DID call `Sys.Stop` — but fire-and-forget AND from the last line of the `await using` block, so an in-flight tick could still be writing to MSSQL during read-back and any earlier assertion failure skipped the stop entirely; stops moved before the read-backs, plus the `AuditLogIngestActor` that test 2 was discarding outright (holding a disposed provider) is now stopped too. Known lesser sibling habit left in place: EventStream probes are never unsubscribed (bounded by the per-test ActorSystem — noted, not a row). AuditLog.Tests 383/383. | Row-82 fix session (2026-08-17) | Closed 2026-08-17. | — |
|
||||
| 95 | ~~**The leaked 100 ms-tick test-actor pattern is systemic — three more classes carry it**~~ **RESOLVED 2026-08-17 (merge `106e467d`, agent commit `34a8079d`).** All three classes live in AuditLog.Tests (premise correction: none in SiteCallAudit.Tests). `Watch`/`Stop`/`ExpectTerminated` replicated from `da23f2dc` at every creation site: 11 in `AuditLogPurgeActorTests`, 10 in `SiteAuditReconciliationActorTests`, 2 in `OutageReconciliationTests`. Second premise correction: `OutageReconciliationTests` DID call `Sys.Stop` — but fire-and-forget AND from the last line of the `await using` block, so an in-flight tick could still be writing to MSSQL during read-back and any earlier assertion failure skipped the stop entirely; stops moved before the read-backs, plus the `AuditLogIngestActor` that test 2 was discarding outright (holding a disposed provider) is now stopped too. Known lesser sibling habit left in place: EventStream probes are never unsubscribed (bounded by the per-test ActorSystem — noted, not a row). AuditLog.Tests 383/383. **Postscript 2026-08-19 (arch-review P8-6): the premise correction was itself WRONG — SiteCallAudit.Tests DOES leak 100 ms-tick actors.** `SiteCallAuditPurgeTests` (4 creation sites incl. a discarded `Sys.ActorOf`) and `SiteCallAuditReconciliationTests` (11 sites, two in one test) had ZERO `Watch`/`Stop`/`ExpectTerminated`. Reopened and re-closed 2026-08-19 (agent commit `102e7a2b`): row-82 treatment via a per-file `StopTicker` helper (`try/finally`, blocked test doubles released first) at every site; the same sweep also fixed `PendingDeploymentPurgeActorTests.cs:90` in Communication.Tests (same class), capped Communication.Tests with the row-96 `xunit.runner.json` (`maxParallelThreads: 4` — it was the last uncapped TestKit+Kestrel suite), collection-serialized the two `aggregators.active` gauge-moving classes (row-102 shape), and reworked 11 sleep-then-assert-absence sites in `SiteAlarmAggregatorActorTests` per row-40 (`AssertHoldsFor` polling helper; settle-sleeps before *actions* left alone). Both suites 3× green (717/717, 66/66), zero flakes observed. | Row-82 fix session (2026-08-17) | Closed 2026-08-17; reopened + re-closed 2026-08-19 (arch-review P8-6). | — |
|
||||
| 96 | ~~**xunit parallelism is uncapped almost everywhere**~~ **RESOLVED 2026-08-17 (merge `106e467d`, agent commit `d2d4db49`) — policy made and shipped: per-suite caps for the resource-heavy suites, pure unit suites stay uncapped.** The four MSSQL-fixture users (`ConfigurationDatabase.Tests`, `AuditLog.Tests`, `SiteCallAudit.Tests`, `Host.Tests` — verified before capping) get `xunit.runner.json` with `maxParallelThreads: 4`, **deliberately matching row-82's `sp_getapplock` slot default** so the in-process and cross-process limits agree; no collections serialized. `IntegrationTests` (already fully serialized) and `CentralUI.PlaywrightTests` (already `maxParallelThreads: 1`, stricter than needed) left alone. Cap verified live (216% CPU uncapped vs 145% capped on ConfigurationDatabase.Tests); cost is nil — these suites are latency-bound on MSSQL/sockets (14–25% CPU), durations unchanged within noise. Policy documented in `docs/test_infra/test_infra.md` → *Test Parallelism Policy* (two-axis model, per-suite table, default for new projects), cross-linked from `test_infra_db.md`. Found-not-fixed: `StoreAndForward.Tests/xunit.runner.json` is a no-op (pins the default `parallelizeTestCollections: true`) — harmless, tidy on next touch. | Row-82 fix session (2026-08-17) | Closed 2026-08-17. | — |
|
||||
| 97 | ~~**A live DCL connection actor is never reconfigured in place — on ANY node**~~ **RESOLVED 2026-08-17 (merge `9dbda4a2`, agent commit `7c1268d8`) — the memo's §6 sketch shipped, plus two things the sketch missed.** `EnsureDclConnection` now has three outcomes off its one hash slot: unchanged → nothing; new → create; **changed → `ReconfigureConnectionCommand`** (arbitration/tombstones byte-for-byte untouched). The manager forwards to the owning actor (unknown name falls back to create); `DataConnectionActor` handles it in **all three** lifecycle states and never stashes it, via a new shared `SwapAdapter` helper now also used by both failover sites. The sketch's two gaps, found in implementation: **endpoint selection must be re-armed to Primary** (a connection sitting on its old *backup* would otherwise dial the new backup first), and the deliberate link drop must **suppress the unstable-disconnect accounting** (else the swap itself could trip a failover). Also a premise-sharpening: "reconfigure while mid-connect" is the *primary* case, not an edge — a wrong endpoint loops in `Connecting` forever and never reaches `Connected`. Fail-before: 5/7 new `DataConnectionReconfigureTests` + 3 SiteRuntime ensure-path tests fail on the reverted behavior. **Live rig probe (post-redeploy, round trip):** connection 1's config changed (operationTimeoutMs 15000→16000→back) fired exactly TWO in-place reconfigures on the active node — `Reconfiguring DataConnectionActor for OPC PLC Simulator in place` → `Reconnected successfully on Primary endpoint` in 45 ms → `Re-subscribing 2 tags` — with all rig-probe-1 tags Good after each, the standby silent (0 reconfigure lines, topology as designed), and the old dead-end "has a new configuration" Warning never appearing. Rig config restored to original. | Found by the 2026-08-17 rows-89/90 fix session; memo `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md` (§9 postscript appended) | Closed 2026-08-17. | — |
|
||||
| 98 | ~~**`EnforceSiteScopeForInstance` is fail-open for nonexistent instance ids**~~ **RESOLVED 2026-08-17 (merge `152ec85e`, agent commit `f33f25a1`) — premise corrected: the defect had FIVE instances, not two, and the worst three were worse than "harmless".** Beyond the two named helpers, the same `if (target != null) EnforceSiteScope(...)` shape sat **inline** in `HandleGetInstance` (:879), `HandleGetSite` (:1702) and `HandleGetDataConnection` (:1844) — and those returned `ManagementSuccess{JsonData=null}` for a nonexistent id vs `ManagementUnauthorized` for an out-of-scope one, i.e. a working **id-enumeration oracle across the whole fleet** for a site-scoped caller. All five now route through one shared `DenyUnresolvedTargetIfScoped`: a scoped caller's unresolvable target is DENIED, indistinguishable from out-of-scope (no existence leak); system-wide callers (Administrator / system-wide Deployer) are untouched. Also checked: `Instance.SiteId` is non-nullable `int`, so no secondary fail-open path through `EnforceSiteScope(null)`. Fail-before: 5 of the 16 new ManagementActorTests fail on the old code. | Found by the 2026-08-17 rows-37/86-88 fix session (Agent A survey) | Closed 2026-08-17. | — |
|
||||
| 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. | — |
|
||||
| 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. **Postscript 2026-08-19 (arch-review P6-3): the strict parser this row shipped was itself fail-open — `Enum.TryParse` splits and ORs comma-joined names inside ONE token for any enum (not just `[Flags]`), and dense 0-based `AuditStatus` lands composites on defined members:** `--status Forwarded,Attempted` = 1|2 = 3 = **Delivered** (silently wrong data), `Submitted,Parked` = 0|5 drops Submitted; the server passes raw query values whole (ASP.NET Core does not split commas), so the trap was live on `/api/audit/query` + export. Reopened and re-closed 2026-08-19 (agent commit `db174633`): any token containing `,` is rejected before `TryParse` with the same valid-values error; proof-of-failure verified on the two name+name composites. Nuance pinned by probe: the `"Failed,7"` name+ordinal variant does NOT reproduce — `TryParse` rejects mixed name/numeric comma lists outright — the test is kept as a regression guard. Repeated-param lists of valid names unaffected. | Found by the 2026-08-17 rows-37/86-88 fix session | Closed 2026-08-17; reopened + re-closed 2026-08-19 (arch-review P6-3). | — |
|
||||
| 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**~~ **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. | — |
|
||||
| 103 | **Two lesser TTL-bounded circuit-teardown-vs-async-setup races remain in CentralUI** (arch-review 2026-08-19 P6-6's optional tail, deliberately deferred by the fix). The permanent debug-stream leak was closed at `DebugView.razor` + `DebugStreamHub` (dispose-vs-connect take-once pattern, commit `11e623c2`), but two same-family instances with a *different shape* were left: (1) `TransportImport.razor.cs TryLoadAsync` (~:244) — a circuit torn down mid-load strands 1 of the 8 `MaxConcurrentImportSessions` slots until the 30-min `BundleSession` TTL reaps it (partially defeats row 43); (2) `Topology.razor` (~:201-214) — timer created before a disposal re-check. Both are TTL/lifetime-bounded (no permanent leak), which is why they missed the Medium bar. Fix is the same post-await `_disposed` re-check + take-once teardown, but routed through `BundleSession.ReleaseSession`/the wizard-step lifecycle rather than a session-stop call. | Arch-review 2026-08-19 P6-6 remediation (T10 explicitly scoped them out) | Open. | Next CentralUI hardening pass, or opportunistically when either file is next touched. |
|
||||
| 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)
|
||||
|
||||
@@ -0,0 +1,374 @@
|
||||
# Medium-Findings Remediation Plan — 2026-08-19 Arch Review
|
||||
|
||||
Scope: all **Medium** and **Medium-High** findings from the 2026-08-19 updated architectural
|
||||
review (report artifact `43c7fa20-9626-4d59-a9c9-061524794660`). The four Highs (P2-1, P2-2,
|
||||
P6-1, P6-2) were fixed in the preceding wave (`c4d3c8b8..d18f344e`, deployed to the rig).
|
||||
Baseline: `main @ d18f344e`.
|
||||
|
||||
Two borderline findings are rounded **in** because they are cheap and adjacent to in-scope
|
||||
work: P3-3 (Low-Med, Debug View dual-address requirement) and P4-4 (Med-Low, standby
|
||||
event-log retention). All plain-Low findings stay out of scope (P1-4, P2-5, P2-6, P3-4,
|
||||
P3-5, P4-5..P4-7, P5-3..P5-6, P6-10..P6-12, P7-5, P7-6, P8-7..P8-10).
|
||||
|
||||
Register consequences (handled at integration, NOT by task agents, to avoid file
|
||||
contention): row 99 reopened+closed by T8 (P6-3 shows the closure was defective);
|
||||
row 95 reopened+closed by T17 (P8-6 shows its premise correction was wrong).
|
||||
|
||||
Execution model (same as the High wave): file-disjoint tasks, one dedicated manual git
|
||||
worktree + model-matched subagent each, pathspec commits, cherry-pick onto main,
|
||||
combined build + affected suites, `git diff` review, single merge session. Two waves.
|
||||
|
||||
Decision made in this plan (owner may override later): **P7-2 takes the conservative
|
||||
option** — bulk `DeploySiteAsync` skips `Disabled` instances and surfaces them in the
|
||||
result as skipped, matching the reconcile path's deliberate preservation of Disabled.
|
||||
Rationale: the alternative (silently force-enabling deliberately-stopped equipment on a
|
||||
bulk push) is the hazard the finding names.
|
||||
|
||||
---
|
||||
|
||||
## Wave 1
|
||||
|
||||
### T1 (Opus) — P1-1 + P1-2: DeploymentManager races (heal resurrection; redeploy override reset)
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs`,
|
||||
`InstanceActor.cs`, the `EnsureInstanceStartedResult` message, `InstanceActorTests.cs`.
|
||||
|
||||
- **P1-1 (Med):** `HandleEnsureInstanceStarted` (:1571) re-reads the store off-thread but
|
||||
captures no `_storeWriteGeneration`; the result handler never calls
|
||||
`IsStoreSnapshotStale` (unlike boot :521 and sweep :1726). A Disable/Delete processed
|
||||
between read and result resurrects the instance permanently (sweep has no runtime→store
|
||||
stop leg; disabled rows `continue`, deleted rows are absent). Verifier note: trigger is
|
||||
the ONE-SHOT startup reconcile (`SiteReconciliationActor.cs:267`), so the window is
|
||||
boot-adjacent; the stale comment at :1567-1570 must go. **Fix:** thread the generation
|
||||
captured at read through `EnsureInstanceStartedResult`; discard on mismatch (boot/sweep
|
||||
pattern). Test: heal result arriving after a Disable/Delete is dropped.
|
||||
- **P1-2 (Med, arguably Med-High):** ApplyDeployment creates the new InstanceActor (:1202)
|
||||
BEFORE the Task.Run that ends in `ClearStaticOverridesAsync` (:1222-1234); the new
|
||||
actor's PreStart override load typically beats the clear (verifier: likely interleaving)
|
||||
→ pre-redeploy overrides applied for the actor's lifetime, violating "reset on
|
||||
redeployment". Leg 2: old actor's PostStop fire-and-forget `FlushDirtyStaticOverrides`
|
||||
(InstanceActor.cs:363-364) can durably commit AFTER the clear. **Fix:** on redeploy,
|
||||
clear overrides in the persistence task BEFORE `CreateInstanceActor` (or gate the
|
||||
override load behind the persistence result), and order the old actor's final flush
|
||||
before the clear (e.g. await/flush before actor creation — Terminated is already awaited
|
||||
:1004-1066, so flushing synchronously in PostStop's stop path or draining before clear
|
||||
are both acceptable; pick the minimal ordering that closes both legs).
|
||||
`InstanceActorTests.cs:355-356` pins the WRONG contract — fix the test to pin the right
|
||||
one. Tests: redeploy never applies pre-redeploy overrides; late flush cannot resurrect.
|
||||
|
||||
### T2 (Sonnet) — P1-3: Alarm on-trigger runs bypass the stuck-script watchdog
|
||||
|
||||
**File:** `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs`
|
||||
(`RunAlarmScriptAsync` :454-537 / `LaunchAlarmScript` :149-186).
|
||||
|
||||
Script path arms watchdog + slot capture (:244-253); alarm path does none on the same
|
||||
bounded pool — a wedged alarm body silently costs a pool thread forever (bounded 4/alarm
|
||||
by the in-flight cap), with no site event, no detach, no healthCollector error increment,
|
||||
no `ScriptRunSummaryRecorder`. **Fix:** mirror the script path exactly — slot/stamp/
|
||||
watchdog + health/summary threading (timeout catch today is LogWarning-only :518-524).
|
||||
Tests: first tests to drive `LaunchAlarmScript` — wedged alarm body triggers watchdog
|
||||
detach + site event + `DetachedScriptThreads`; timeout increments health error.
|
||||
|
||||
### T3 (Opus) — P2-3 + P2-4: DCL ConnectResult generation; OPC UA batch rollback
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs`,
|
||||
`Adapters/RealOpcUaClient.cs`.
|
||||
|
||||
- **P2-3 (Med):** `ConnectResult` piped untagged (:777-787). Reconfigure mid-connect: old
|
||||
adapter's `ConnectResult(true)` drives ReSubscribeAll+BecomeConnected against the NEW
|
||||
never-connected adapter; new adapter's result dropped in Connected; idempotent-
|
||||
reconfigure guard (:644-652) then blocks recovery until restart/redeploy. **Fix:** tag
|
||||
`ConnectResult` with the adapter generation (stamped at connect launch, exactly the
|
||||
P2-1 pattern now on `SubscribeCompleted` — reuse it); drop mismatches. Tests mirror
|
||||
`DataConnectionStaleCompletionTests` for the connect leg.
|
||||
- **P2-4 (Med):** `CreateSubscriptionsCoreAsync` (RealOpcUaClient.cs:663): mid-batch
|
||||
`ApplyChangesAsync` throw leaves all `created` items registered (_callbacks/_itemShard/
|
||||
ItemCount/SDK items :628-661) with no rollback, and `IsConnectionLevelFailure`
|
||||
(DataConnectionActor.cs:1464-1471) misses `ServiceResultException` → chunk treated
|
||||
per-tag unresolved, each backoff probe RE-ADDS items → unbounded shard growth. **Fix:**
|
||||
unwind `created` via `ReleaseFailedItem` on throw; classify SDK bad-session codes
|
||||
connection-level. Tests: fake-SDK mid-batch throw leaves no residue; bad-session code
|
||||
drives the connection-level path.
|
||||
|
||||
### T4 (Opus) — P3-1 + P3-2 + P3-3: gRPC client lifecycle/disposal races
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/SiteStreamGrpcClient.cs`,
|
||||
`Grpc/SitePairChannelProvider.cs`, `DebugStreamService.cs`.
|
||||
|
||||
- **P3-1 (Med):** `RegisterSubscription` (:147-153) disposes the prior handler's CTS while
|
||||
its `ConsumeStreamAsync` may still read the token (ODE at :219/:294/:359/:364);
|
||||
`Unsubscribe` (:455-462) unguarded Cancel; `ReleaseResources` (:609-619) unguarded
|
||||
iteration whose ODE aborts the factory's DisposeAsync loop leaving later clients
|
||||
undisposed; an ODE from `prior.Cancel()` escapes the NEW SubscribeAsync with the CURRENT
|
||||
generation → phantom fault. **Fix:** cancel-only on replacement (dispose only from the
|
||||
owning path), ODE-guard every Cancel, keep factory disposal loop fault-isolated.
|
||||
`SiteStreamGrpcClientTests.cs:680` pins current behavior — re-pin.
|
||||
- **P3-2 (Med):** `UpdateSite`/`RemoveSite` (:123-131/:155-159) Dispose channels under
|
||||
`pair.Gate` while `ExecuteAsync` invokes outside the lock (:203-215) → 60s DB refresh
|
||||
with an edited address kills in-flight RPCs (multi-minute DeployArtifactsCommand) with
|
||||
ODE/Cancelled (not Unavailable → no cross-node retry). **Fix:** deferred disposal — park
|
||||
replaced channels until quiescent or time-delayed; mirror the stream factory's redesign
|
||||
rationale (SiteStreamGrpcClientFactory.cs:12-22).
|
||||
- **P3-3 (Low-Med, rounded in):** `DebugStreamService` (:111-114) throws unless BOTH gRPC
|
||||
node addresses are set, though single-endpoint sites are supported elsewhere. **Fix:**
|
||||
apply the N9 degeneration (`SiteAlarmLiveCacheService.cs:324-330`): reuse the sole
|
||||
endpoint.
|
||||
|
||||
### T5 (Opus) — P4-1 + P4-3: LocalDb sync divergence + liveness (cross-repo)
|
||||
|
||||
**Files (scadaproj):** `~/Desktop/scadaproj/ZB.MOM.WW.LocalDb/src/.../Internal/SyncSession.cs`,
|
||||
`OplogStore.cs`, `SyncBackgroundService.cs`, tests. **Files (here):**
|
||||
`src/ZB.MOM.WW.ScadaBridge.Host/Program.cs` (site Kestrel h2 keepalive),
|
||||
`Directory.Packages.props` (0.3.1 → 0.3.2 when published).
|
||||
|
||||
- **P4-1 (Med):** `PumpLoopAsync` never checks `needs_snapshot` after a cap-prune though
|
||||
`OplogStore.ReadBatchAboveAsync` documents the obligation (OplogStore.cs:27-29) —
|
||||
maintenance prunes above `_sentThruSeq` on a slow-but-alive peer → pump silently skips
|
||||
the gap, pair diverges for the session's lifetime (heals only on reconnect). **Fix:**
|
||||
fault the live session when `EnforceCapsAsync` prunes into the in-flight range (the
|
||||
0.3.1 fault-teardown control-lane flush already exists — reuse it). Test: prune during
|
||||
an active session faults it; reconnect serves the snapshot.
|
||||
- **P4-3 (Med):** no keepalive anywhere on the sync stream: bare `GrpcChannel.ForAddress`
|
||||
client-side, no Kestrel `Http2.KeepAlivePingDelay` on site 8083 → hard crash/partition
|
||||
of the dialing node leaves the passive node's handler hung on MoveNext holding
|
||||
`_active=1`, rejecting the rebooted initiator's reconnects `AlreadyExists`, possibly
|
||||
forever; zombie side reports `LocalDbReplicationConnected=true`. **Fix:** client
|
||||
`SocketsHttpHandler` KeepAlivePing (delay/timeout, always-with-active-requests) in
|
||||
`SyncBackgroundService`; Kestrel h2 keepalive on the site gRPC listener in Host
|
||||
`Program.cs`; AND newest-wins eviction (or a bounded read deadline) on the server's
|
||||
single-stream guard so a fresh authenticated Sync can displace a dead one.
|
||||
- **Version/publish:** bump LocalDb to 0.3.2, commit in scadaproj (do NOT push);
|
||||
`build/push.sh` needs the user-held `GITEA_NUGET_KEY` — if absent, leave the
|
||||
ScadaBridge `Directory.Packages.props` bump as a flagged follow-up and land only the
|
||||
Host keepalive here. The ScadaBridge-side Kestrel change is independent and lands
|
||||
regardless.
|
||||
|
||||
### T6 (Sonnet) — P4-2 + P4-4: audit-writer durability; standby event-log retention
|
||||
|
||||
- **P4-2 (Med, arguably Med-High):** `AuditLog/Site/SqliteAuditWriter.cs` — per-row
|
||||
`TrySetResult` (:487/:499) BEFORE `transaction.Commit()` (:503); commit failure
|
||||
(SQLITE_FULL/IOERR) no-ops `TrySetException` → `FallbackAuditWriter` sees success, no
|
||||
ring fallback, whole batch silently lost in exactly the outage the ring exists for;
|
||||
`SiteAuditWriteFailures` stays 0. Bonus (verifier): a mid-batch insert failure on row k
|
||||
loses rows 0..k-1 the same way. **Fix:** complete TCSes only after commit (the pattern
|
||||
`SiteEventLogger.FlushBatch` already uses :319/:357-361); on commit/insert failure fail
|
||||
EVERY row's TCS so the ring fallback engages. Tests: failed commit → all TCSes faulted +
|
||||
ring receives batch + failure counter increments.
|
||||
- **P4-4 (Med-Low, rounded in):** `SiteEventLogging/EventLogPurgeService.cs:215-219`
|
||||
early-exits on the standby on the stale premise "standby receives no writes" — DCL
|
||||
actors run on both nodes since rows 89/90/97 and write site events. On an unreplicated
|
||||
site (site-b/c posture) the standby's `site_events` grows forever. **Fix:** let the
|
||||
standby run retention/cap against its own file (no-op-safe on a replicated pair —
|
||||
deletes are CDC-captured and LWW-converge; say so in a comment). Test: purge runs when
|
||||
not primary.
|
||||
|
||||
### T7 (Sonnet) — P5-1 + P5-2: central ingest fallback budget; SiteCalls query shape
|
||||
|
||||
- **P5-1 (Med):** `AuditLog/Central/AuditLogIngestActor.cs:372-405` — per-entry fallback
|
||||
loop passes NO token/timeout (contrast `OnIngestAsync`'s 5s `IngestFallbackBudget`
|
||||
:268); each entry sits inside `SqlServerRetryingExecutionStrategy` 5×30s → minutes
|
||||
inside `ReceiveAsync`, singleton mailbox frozen, site re-sends amplify. **Fix:** mirror
|
||||
`OnIngestAsync` — short linked CTS + `IngestSqlCommandTimeout` per entry, pass the token
|
||||
to `InsertIfNotExistsAsync`/`UpsertAsync`. Test: fallback loop honors budget.
|
||||
- **P5-2 (Med):** `ConfigurationDatabase/Repositories/SiteCallAuditRepository.cs:301-331`
|
||||
orders `(CreatedAtUtc DESC, TrackedOperationId DESC)` with no index leading on
|
||||
`CreatedAtUtc` (full scan+sort per unfiltered page, row-35 class) and binds
|
||||
nvarchar(4000) params against `IsUnicode(false)` columns (may block seeks, incl. the
|
||||
keyset predicate :316; write path types VarChar deliberately :230). **Fix:** add the
|
||||
`(CreatedAtUtc DESC, TrackedOperationId DESC)` index via EF migration (BUILD FIRST —
|
||||
`--no-build` scaffolds empty) + typed varchar params on the read path. Update the
|
||||
handwritten production SQL script if one exists for this table (migration-parity rule).
|
||||
|
||||
### T8 (Sonnet) — P6-3: ParseEnumListStrict comma smuggling (register row 99 defective)
|
||||
|
||||
**File:** `src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs:117-119`.
|
||||
|
||||
`Enum.TryParse` ORs comma-joined names for any enum; `LooksNumeric` checks first char
|
||||
only; dense 0-based `AuditStatus` makes composites land on defined members —
|
||||
`"Forwarded,Attempted"` = 1|2 = 3 = `Delivered` (silent wrong data); comma also smuggles
|
||||
ordinals past the row-88/99 guard (`"Failed,7"` → `Skipped`). Server path confirmed:
|
||||
`AuditEndpoints.cs:656-658` passes the raw query value whole. **Fix:** reject any value
|
||||
containing ',' before TryParse (per-token error, same UX as existing invalid-name error).
|
||||
Tests: comma-joined names rejected; `"Failed,7"` rejected; single names/known-good paths
|
||||
unchanged. Do NOT edit the register (integration does).
|
||||
|
||||
## Wave 2
|
||||
|
||||
### T9 (Opus) — P6-4 + P6-5: ManagementActor scoping gaps (single-writer lane)
|
||||
|
||||
**File:** `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs` (sole owner
|
||||
this wave).
|
||||
|
||||
- **P6-4 (Med):** `HandleListDataConnections` (:1875-1881) takes no user, null role gate →
|
||||
lists every site's connections (scrubbed but topology-revealing existence oracle) while
|
||||
`HandleGetDataConnection` fail-closes (:1888-1900) — Get denies what List hands over.
|
||||
**Fix:** filter by `PermittedSiteIds` exactly as `HandleListSites` (:1719-1729) /
|
||||
`HandleListSecuredWrites` (:1665-1695) do. Check the Design UI page's separate
|
||||
`SiteRepository.GetAllDataConnectionsAsync` bypass (DataConnections.razor:309) — if it
|
||||
serves scoped users unfiltered, route it through scope too (same one-line filter).
|
||||
- **P6-5 (Med):** resolve-before-scope oracles outside row 98's five shapes: approve
|
||||
(:1465-1474) and reject (:1627-1635) check not-found + status (leaking "is Executed,
|
||||
not Pending") BEFORE `EnforceSiteScopeForIdentifier`; same shape `HandleDebugSnapshot`
|
||||
(:3604-3608) + `HandleSubmitSecuredWrite` (:1417-1423). **Fix:** hoist scope checks
|
||||
above resolution/status checks at all four sites (row-98 `DenyUnresolvedTargetIfScoped`
|
||||
semantics). Tests: out-of-scope caller gets the scope denial, not the status oracle,
|
||||
at each site.
|
||||
|
||||
### T10 (Opus) — P6-6: Debug-stream session abandoned mid-start leaks permanently
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/DebugView.razor(.cs)`
|
||||
(:493), `DebugStreamHub.cs` (:195).
|
||||
|
||||
`Connect()` awaits `StartStreamAsync` (≤30s) then assigns `_session` with no `_disposed`
|
||||
re-check; if Dispose already ran, StopStream never fires and the singleton
|
||||
`DebugStreamService` keepalive renews the registered session forever (:157/:213-219; the
|
||||
5-min orphan net only reaps UNregistered sessions) — holds the site gRPC debug sub +
|
||||
bridge actor + dead circuit graph until restart. Identical shape in `DebugStreamHub.cs:195`.
|
||||
**Fix:** post-await `_disposed` re-check that stops the just-created session (both sites);
|
||||
make Dispose vs late-assign race-safe (interlocked/lock — Blazor Sync context helps but
|
||||
Dispose can run on a different flow). Tests: dispose-during-start stops the session and
|
||||
leaves nothing registered. The lesser TTL-bounded instances (TransportImport slot,
|
||||
Topology timer) are OPTIONAL — take them only if trivial with the same pattern.
|
||||
|
||||
### T11 (Sonnet) — P6-7 + P6-8: audit export injection + CLI deadline
|
||||
|
||||
- **P6-7 (Med):** `ManagementService/AuditEndpoints.cs:810-840` CSV quotes only on
|
||||
`, " \r \n`; leading `= + - @ \t` pass through → formula/DDE execution on operator open
|
||||
(CWE-1236); exported Target/ErrorMessage/Request/ResponseSummary/Extra carry inbound-
|
||||
API-caller text verbatim. **Fix:** prefix-escape cells starting with those chars
|
||||
(`'` prefix, the standard neutralization). Secondary: CSV omits ExecutionId/
|
||||
ParentExecutionId/SourceNode vs JSONL and the `AuditExportRow.cs:11-13` "24-field"
|
||||
claim — add the columns and align the doc comment.
|
||||
- **P6-8 (Med):** `CLI/Commands/AuditExportHelpers.cs:129` —
|
||||
`SendGetStreamAsync(..., CancellationToken.None)` after `HttpClient.Timeout = Infinite`
|
||||
(residue of the e0e4b246 timeout-honesty fix): a hung server hangs the CLI forever, and
|
||||
body reads (:141/:152)/`CopyToAsync` (:171) are tokenless (mid-stream fault → raw stack
|
||||
+ truncated file). **Fix:** headers-received deadline + overall/inactivity deadline +
|
||||
Ctrl-C token wiring through all reads; friendly error on mid-stream fault. Tests:
|
||||
deadline fires; cancellation propagates.
|
||||
|
||||
### T12 (Sonnet) — P6-9: LoginThrottle never throttles single-IP password spray
|
||||
|
||||
**File:** `src/ZB.MOM.WW.ScadaBridge.CentralUI/Security/LoginThrottle.cs:189-190`.
|
||||
|
||||
Key is `{username}|{ip}` → independent 5-failure budget per username per IP; a spray
|
||||
(many usernames, one IP) is never throttled though the class doc claims spray protection.
|
||||
**Fix:** add a secondary per-IP counter with a higher ceiling (e.g. 20-30 failures/window
|
||||
→ per-IP lockout), layered on the existing per-pair key; keep the doc honest. Tests:
|
||||
spray locks the IP; normal per-user behavior unchanged; distinct IPs unaffected.
|
||||
|
||||
### T13 (Sonnet) — P7-1: ExternalSystem TimeoutSeconds silently dropped by bundles
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs:248`,
|
||||
`EntitySerializer.cs:127-141`, `BundleImporter.cs:3087-3128`, `ArtifactDiff.cs:166-179`,
|
||||
`RoundTripEquivalenceTests.cs:445-452`.
|
||||
|
||||
Row-27 SHAPE recurrence: DTO lacks `TimeoutSeconds` (operative at runtime and deployed as
|
||||
proto field 6); export never writes it, Add resets to 0, Overwrite keeps old, diff never
|
||||
compares → a tuned-timeout-only bundle previews Identical and the target env silently runs
|
||||
the gateway default. The round-trip guard is vacuous because the fixture never sets the
|
||||
field. **Fix:** add the DTO field + both serializer directions + both import branches +
|
||||
`AddIfDifferent` in the diff + a NON-DEFAULT fixture value so the reflection round-trip
|
||||
guard actually covers it. Schema stays additive (schemaVersion unchanged — additive field).
|
||||
|
||||
### T14 (Opus) — P7-2 + P7-3 + P7-4: bulk-deploy semantics, reconcile deadline, probe TOCTOU
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs`,
|
||||
`StaleInstanceProbe.cs`, `FlattenSession.cs`.
|
||||
|
||||
- **P7-2 (Med, decided above):** `DeploySiteAsync` deploys ALL instances (:804) and every
|
||||
success runs `forceEnabledState:true` (:610-612) — bulk push re-enables deliberately-
|
||||
Disabled equipment while the reconcile path deliberately preserves Disabled (:1638-1649,
|
||||
pinned). **Fix:** skip Disabled instances in bulk deploy; surface them in the result as
|
||||
a skipped list with reason; pin with tests (zero Disabled cases exist in
|
||||
DeploySiteAsyncTests today). Update `Component-DeploymentManager.md` (one sentence).
|
||||
- **P7-3 (Med):** phase-1 `TryReconcileWithSiteAsync` per instance is bounded only by the
|
||||
30s QueryTimeout → unreachable site × N instances ≈ hours serial before a single send;
|
||||
CLI's 5-min BulkDeployTimeout cancels and re-arms the identical cost. **Fix:** in bulk
|
||||
mode, short linked per-reconcile deadline AND a once-per-batch site reachability probe —
|
||||
first reconcile timeout marks the site unreachable for the rest of Prepare (instances
|
||||
proceed on DB state, which is what happens on timeout today anyway).
|
||||
- **P7-4 (Med):** `StaleInstanceProbe.cs:106-109` re-reads per-template versions AFTER
|
||||
flatten (comment claims before) → a non-structural template edit during flatten pairs
|
||||
pre-edit hash with post-edit version; memo permanently serves not-stale. FlattenSession
|
||||
captures correctly (FlattenSession.cs:114) but doesn't expose it. **Fix:** expose the
|
||||
session's captured `MemberVersions`; probe stores those instead of re-reading. Test:
|
||||
template edit mid-flatten still reports stale.
|
||||
|
||||
### T15 (Sonnet) — P8-1: three SiteHealthReport fields never cross the wire
|
||||
|
||||
**Files:** `src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/CentralControlDtoMapper.cs:452`,
|
||||
the vendored centralcontrol proto (regen via `bash docker/regen-proto.sh centralcontrol`),
|
||||
`SiteRuntime SiteHealthCollector.cs:243/:317-327`, `HealthReportSender.cs:180`.
|
||||
|
||||
`DetachedScriptThreads`, `ScriptRunShedCount`, `SiteStreamAlarmDropCount` are populated
|
||||
site-side but `ToDto` omits all three (proto ends at field 29) — a watchdog-detaching/
|
||||
shedding/alarm-dropping site looks healthy centrally. Aggravator: `AddIntervalCounters`
|
||||
takes only the 5 original counters and `HealthReportSender` never passes the shed count →
|
||||
a failed send permanently loses the interval's shed count (drained by
|
||||
`Interlocked.Exchange`). **Fix:** proto fields **30-32** (additive, never reuse numbers),
|
||||
mapper both directions, extend `AddIntervalCounters` so failed sends restore all drained
|
||||
counters. Follow the vendored-proto workflow exactly (regen script restores the csproj; an
|
||||
active `<Protobuf>` item must never be committed). Tests: mapper round-trips the three
|
||||
fields; failed send restores shed count.
|
||||
|
||||
### T16 (Sonnet) — P8-2 + P8-3 + P8-4: env2 topology drift + regen-proto trap
|
||||
|
||||
- **P8-2 (Med):** `docker-env2/docker-compose.yml` — add `init: true` to all 4 services
|
||||
(docker/ has it ×8 with the issue-#34 rationale; env2's PID-1 boot failure wedges at
|
||||
100% CPU instead of restart-looping).
|
||||
- **P8-3 (Med):** env2 centrals still carry the SEC-36-rotated LDAP bind cred in plaintext
|
||||
(`central-node-a/appsettings.Central.json:28,34` + node-b), plus inline SQL passwords +
|
||||
JWT key. **Fix:** port the docker/ posture — compose env-var overrides for secrets, and
|
||||
the same LDAP fallback/DisableLogin arrangement docker/ uses. Do NOT invent new secret
|
||||
values; reference the same env vars docker/ consumes.
|
||||
- **P8-4 (Med):** `docker/regen-proto.sh:58` — `trap ... ERR` doesn't fire on Ctrl-C
|
||||
during the long build → active `<Protobuf>` left in the csproj (breaks Docker build).
|
||||
**Fix:** idempotent restore trap on `EXIT` (covers ERR/INT/TERM/success; guard so a
|
||||
successful run doesn't double-restore).
|
||||
|
||||
### T17 (Sonnet) — P8-5 + P8-6: test-health (register rows 95/96/40/102 classes)
|
||||
|
||||
- **P8-5 (Med):** Communication.Tests: (a) add `xunit.runner.json` `maxParallelThreads: 4`
|
||||
(row-96 policy; ~14 TestKit classes + real Kestrel, last uncapped resource-heavy suite);
|
||||
(b) `SiteAlarmLiveCacheTelemetryTests.cs:55-93` asserts fixed baselines on the
|
||||
process-global `aggregators.active` gauge with no `[Collection]` while two sibling
|
||||
suites move the instruments (row-102 shape) — apply the row-102 treatment (serialize via
|
||||
collection or delta-based assertions); (c) rework the 8 sleep-then-assert-absence sites
|
||||
in `SiteAlarmAggregatorActorTests` (:385-386, :624-625 et al) per row-40 (await the
|
||||
observable effect / use TestKit expectations, never wall-clock sleeps), and fix the
|
||||
discarded 100ms-tick actor in `PendingDeploymentPurgeActorTests.cs:90` (row-95 class:
|
||||
Watch + Stop + ExpectTerminated).
|
||||
- **P8-6 (Med):** SiteCallAudit.Tests DOES leak 100ms-tick actors (row 95's premise
|
||||
correction was wrong): `SiteCallAuditPurgeTests` actors at :127/:152/:174 + discarded
|
||||
`Sys.ActorOf` :209-212, `SiteCallAuditReconciliationTests` :58/:248/:372/:592 — zero
|
||||
Watch/Stop/ExpectTerminated in either file. **Fix:** row-82 treatment on every leaked
|
||||
actor. Do NOT edit the register (integration reopens/closes row 95 with a citation).
|
||||
- Test-only task: verify by running both suites (Communication.Tests 3× for flake
|
||||
confidence).
|
||||
|
||||
---
|
||||
|
||||
## Shared constraints (all tasks)
|
||||
|
||||
- Fix + tests only; no drive-by refactors. Match surrounding style/comment density.
|
||||
- Targeted verification per task: build touched project(s) + test project(s); run affected
|
||||
suites filtered. Full solution sweep once, at integration.
|
||||
- Each agent works ONLY inside its own worktree (absolute paths, `git -C`), commits with a
|
||||
pathspec limited to its own files, reports commit SHA + test results. Agents do NOT
|
||||
touch `docs/plans/2026-07-08-deferred-work-register.md` or `CLAUDE.md` (integration
|
||||
does).
|
||||
- Component-doc updates only where the doc states the now-contradicted behavior; one
|
||||
sentence each.
|
||||
- Wire/proto changes additive only; never reuse field numbers.
|
||||
|
||||
## Integration (per wave, then final)
|
||||
|
||||
1. Cherry-pick wave commits onto `main` (verify file-disjointness first; T4/T15/T17 share
|
||||
the Communication project — files must stay disjoint).
|
||||
2. `dotnet build ZB.MOM.WW.ScadaBridge.slnx`; run the touched suites; full
|
||||
`dotnet test` sweep after wave 2.
|
||||
3. Register edits: reopen+close rows 95 and 99 citing P8-6/P6-3; add rows for anything an
|
||||
agent flags as deliberately deferred.
|
||||
4. `git diff` review; commit plan doc + register/docs touch-ups; remove worktrees.
|
||||
5. No push, no rig redeploy without the user.
|
||||
Reference in New Issue
Block a user