docs(plans): close residual #8 (R6 merged); register rows 43-46 (import-session leak, dead rate limiter, outbox DbContext sharing, CLI audit-config query)

This commit is contained in:
Joseph Doherty
2026-08-15 04:06:27 -04:00
parent 568c18833d
commit 2c6cfdebe7
2 changed files with 17 additions and 2 deletions
@@ -36,6 +36,10 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round
| 40 | **Wall-clock sleeps used as synchronization in tests (absence-assertion class)** | Found by the R5 sweep (`9fb52153`) — a different class from the one swept, deliberately left unfixed | Tests that sleep, then assert an *absence*: `ScriptDeadlineAtEnqueueTests.cs:105,128` (two `Thread.Sleep(2s)`, with `Assert.Equal(0, scheduler.DetachedThreadCount)` depending on the sleep out-racing the watchdog — worst offender), `DeploymentWarmThenGateTests.cs:117`, `SiteAlarmLiveCacheServiceTests.cs:195,216`, `DataConnectionActorTests.cs:435` (`Task.Delay(200)` to "settle" after an `AwaitCondition`). These fail toward false-pass rather than flake, so they don't trip CI — but each is a timing pin that weakens what the test proves. Fix pattern: replace the sleep with an explicit observable edge (or assert on the positive event with a bounded wait plus a negative control). | Next test-infra session, or the first time one of these areas regresses without its test catching it |
| 41 | **LocalDb (scadaproj) sync-session tests flake under parallel load** (scadaproj repo, not this one) | Found by R1; confirmed pre-existing at base `f627cee` (1 failure in 3 full runs) | `SyncSessionTests.Handshake_DigestMismatch_FailsClosed` and `BatchBudgetTests.Pump_RowLargerThanTheBudget_IsSentAloneRatherThanStallingTheStream` intermittently time out. Root cause looks structural, not a timing pin: on a handshake fault, `RunAsync`'s catch cancels the writer loop, which can kill the session's OWN Handshake before it transmits — the healthy peer then waits out its full 15 s cap for a message that never comes. Product-adjacent (the same early-cancel could delay a real peer's fail-closed verdict), so worth a look beyond the tests. | Next LocalDb session |
| 42 | **`build/push.sh`-class publish bug across the family repos** | Found by R1 the hard way (incident: 25 unrelated packages briefly published to the feed, deleted + verified same session; script fixed in scadaproj `0038d46`) | The bug shape: `dotnet pack -c Release -o ./artifacts` with no project argument and no `cd`, so the script packs the CALLER'S CWD. Fixed in LocalDb's script (cd to own repo root, name the solution, absolute output path). Peer repos' publish scripts (MxAccessGateway, HistorianGateway, OtOpcUa, others in scadaproj) should be checked for the same shape — the recovery only worked because the token happened to carry delete rights. | Before anyone runs a family publish script from outside its repo, or next umbrella-repo session |
| 43 | **Central UI import wizard leaks a `BundleSession` slot on every abandoned import — imports lock out for 30 min** *(High — availability + secret lifetime)* | Found live by R6 (reproduced: 3 back-to-back suite runs → 6 of 8 slots held, rotating pair of `TransportImportTests` failures; central-b restart cleared it) | `TransportImport.razor.cs`: `ResetSessionState()` nulls the local field and `Dispose()` deletes only the staged temp file — neither calls `IBundleSessionStore.Remove(sessionId)`; only `BundleImporter.ApplyAsync` removes sessions. An operator who unlocks a bundle then navigates away burns one of `MaxConcurrentImportSessions` (8) for the full `BundleSessionTtlMinutes` (30); eight abandonments lock out ALL imports on the active node. The leaked session also holds the **decrypted** bundle content that T-007 deliberately zeroes on the Apply paths — so this is a secret-lifetime hole too. The class doc's "an abandoned wizard does not leak" claim is true of the plaintext file only. Fix: remove the session (and zero its content) from `Dispose`/`ResetSessionState`/back-navigation. | Fix-now candidate — next session touching Transport or Central UI |
| 44 | **`BundleUnlockRateLimiter` is dead code — the documented per-IP unlock throttle is not enforced** *(Medium — security)* | Found by R6 | Registered, injected into `BundleImporter`, unit-tested, documented on `TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10`, pinned in `appsettings.Central.json` — but `_unlockRateLimiter` is never invoked anywhere in `src/`. A brute-force passphrase attacker is bounded only by the per-bundle 3-strike lockout, not the advertised 10/hour/IP cap. Fix: call it on the unlock path (or delete it and the config surface honestly). | Fix-now candidate — next Transport/security session |
| 45 | **Notification Outbox dispatcher shares one `DbContext` across up to 4 parallel deliveries** *(Medium — correctness under load)* | Found by R6; observed continuously on rig central-b: `InvalidOperationException: A second operation was started on this context instance` on dispatch | `NotificationOutboxActor.ResolveAdapters()` caches adapter instances in an actor-lifetime `IServiceScope` while the Email/Sms adapters are `AddScoped` and hold a scoped `INotificationRepository` (`ScadaBridgeDbContext`); `MaxParallelDeliveries` (4) then shares one context across concurrent deliveries. The per-delivery scope added in `DeliverGatedAsync` for exactly this hazard covers only `INotificationOutboxRepository` — the adapter escapes it. Effect today: failed attempts + retries + error noise; sustained contention could burn a row to `Parked`. Fix: resolve adapters inside the per-delivery scope. | Fix-now candidate — next Notification Outbox session, or first `Parked` row attributed to it |
| 46 | **CLI `audit-config query` returns `{}` for every filter combination** *(Low)* | Found by R6 against the rig; underlying `AuditLogEntries` rows verified present | Possibly a CLI output-shape/serialization issue; not investigated (out of R6 scope). | Next CLI session |
## Resolved (verified against the code 2026-07-10)
Rows removed from the Deferred table above once confirmed shipped. Kept here for traceability.
@@ -158,8 +158,19 @@ Deliberately not fixed in this program — each has a stated reason, not an over
design: `docs/plans/2026-08-15-target-scale-load-test-design.md`; harness:
`tests/ZB.MOM.WW.ScadaBridge.LoadHarness/`. Residual: WP-4's `[xc-3]`/`[xc-4]`/`[xc-5]`/`[xc-8]`
are central-cluster/browser concerns outside this harness and remain open scope.
8. **Playwright 14 pre-existing env failures.** Present on `main` too, rig-state related, not
introduced by this branch.
8. ~~**Playwright 14 pre-existing env failures.**~~ **RESOLVED 2026-08-15 (`014038fb`, R6).**
All 14 triaged to root cause: every one was **selector drift** from two merged Central UI
refactors (`9e243493` density sweep moved `.btn-sm` off buttons onto `btn-group-sm` groups and
re-cast SiteForm node sections as cards; `a506b19d` moved TemplateEdit modals to the global
DialogService host) — no stale rig data, no timing bug, no app defect among the 14. Suite now
**0 failed / 172 passed / 1 honestly-skipped** (SmtpEdit test self-skips: SMTP config rows
cannot be created or deleted from the management surface, UI-only). Runtime halved (10m → 4.5m
— each failure had been burning a 30 s locator timeout). All 49 `.btn-sm` selectors audited
individually rather than swept; the 35 still-correct ones left alone with comments. One stray
test area deleted from the rig via CLI. Three genuine app bugs found live during triage —
recorded as register rows 4345 (import-session slot leak, dead unlock rate limiter, shared
DbContext in outbox dispatch), plus a CLI query oddity as row 46 — found, not fixed, per the
program's scope rules.
9. ~~**`site_events` retention purge still oplog-visible.**~~ **RESOLVED 2026-08-15 (documented
by design, `9d2834e3`, R7).** The CDC capture of retention/cap deletes is correct and unchanged
— there is deliberately no purge-exemption path. The operator-facing gap is closed three ways: