From e59fa5ba4c6cf532a1e7b4dfdb2551fba3dae961 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 06:06:11 -0400 Subject: [PATCH] docs(plans): close register rows 43-44 (transport fixes merged); rows 51-53 (lockout fault mapping, per-node throttle scope, session-cap observability) --- docs/plans/2026-07-08-deferred-work-register.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/plans/2026-07-08-deferred-work-register.md b/docs/plans/2026-07-08-deferred-work-register.md index 1234e066..80c8a51b 100644 --- a/docs/plans/2026-07-08-deferred-work-register.md +++ b/docs/plans/2026-07-08-deferred-work-register.md @@ -36,14 +36,17 @@ 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 | +| 43 | ~~**Central UI import wizard leaks a `BundleSession` slot on every abandoned import — imports lock out for 30 min**~~ **RESOLVED 2026-08-15 (`70838982`).** New `IBundleImporter.Abandon(sessionId)` does the same zero-then-remove release `ApplyAsync` already did (T-007 discipline), idempotent (double/unknown/expired ids are no-ops) and only ever given the caller's own random-GUID id, so it cannot evict a concurrent circuit's session. Wired into EVERY abandonment path: wizard new-upload, `TryLoadAsync` pre-overwrite, Back-to-Upload/`ResetSessionState`, 3-strike lockout reset, `Dispose` — plus two CLI leaks the original row didn't know about: `transport preview` released nothing at all (every preview burned a slot for 30 min) and `transport import` stranded the session on pre-apply aborts; both now release in a `finally`. `BundleSessionEvictionService` is documented as the TTL backstop, not an abandonment path. Fail-before-fix stash-verified (3 of 4 new bUnit tests fail pre-fix). 10 new tests. | Found live by R6 | Closed 2026-08-15. | — | +| 44 | ~~**`BundleUnlockRateLimiter` is dead code — the documented per-IP unlock throttle is not enforced**~~ **RESOLVED 2026-08-15 (`5ab26a42`).** `BundleImporter.LoadAsync` now registers every unlock attempt against `MaxUnlockAttemptsPerIpPerHour` (10). Ordering is load-bearing: AFTER the per-bundle 3-strike check (hammering a locked bundle spends no hourly budget) and BEFORE the decrypt (a throttle denial burns no per-bundle strike, so the throttle can't be abused to lock out a legitimate import); successful unlocks count too. Denial = `BundleUnlockThrottledException` (placed in Commons so the management surface returns a curated message). Client key: Central UI via a scoped `CircuitClientAddressProvider` snapshotting the IP at circuit establishment; CLI via additive `ManagementEnvelope.ClientIp`. No attributable IP falls into one shared `` bucket — stricter, never a bypass; per-client isolation is only real behind a ForwardedHeaders-trusted proxy (same caveat as the login throttle, documented). Fail-before-fix verified (4 of 6 new tests fail with the limiter neutralised; 2 negative controls). | Found by R6 | Closed 2026-08-15. | — | | 45 | ~~**Notification Outbox dispatcher shares one `DbContext` across up to 4 parallel deliveries**~~ **RESOLVED 2026-08-15 (`9e769735`).** `_adaptersCache`/`_adaptersScope`/`ResolveAdapters()` deleted; the typed adapter is resolved inside the per-delivery DI scope `DeliverGatedAsync` already opens (static `ResolveAdapter` preserving DI last-wins + the null-adapter park path), so each concurrent delivery gets its own adapter and `DbContext`. Nothing shared was lost: the Email adapter builds a fresh SMTP client per attempt from an injected factory, the SMS adapter draws from the singleton `IHttpClientFactory`, and CentralUI's adapter-gated Type selector (`NotificationChannelCatalog`) is an independent scoped consumer, untouched. Fail-before-fix stash-verified (simulated DbContext throwing EF's exact second-operation exception under a rendezvous-guaranteed overlap: 1 of 2 rows persisted pre-fix; 1-vs-4 distinct adapter instances). Root cause of note: the bug was INTRODUCED by code-review finding NotificationOutbox-006's caching "fix" (2026-05-28), whose own regression test pinned the broken behavior and predates bounded parallel delivery — that finding is marked SUPERSEDED. NotificationOutbox 153, AuditLog 383 green. | Found by R6; observed live on rig central-b | Closed 2026-08-15. | — | | 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 | | 47 | ~~**`SiteStreamGrpcServer` duplicate-stream replacement disposed another handler's `CancellationTokenSource`**~~ **RESOLVED 2026-08-15 (found and fixed in-flight by R2, `e6842c10`).** `SubscribeInstance`'s duplicate-prevention path called `Cancel()` AND `Dispose()` on the replaced stream's CTS, which belongs to that still-running handler's `using var` and is still read — escaping as an unhandled `ObjectDisposedException` under scheduling pressure (confirmed pre-existing on `2b74851f`; load-only, passed 10/10 isolated). Fixed to cancel-only (the owner's `using` disposes exactly once), `Cancel` wrapped for the converse race, mirroring `CancelAllStreams()`. Deterministic regression test via a gate inside the setup window, fail-before/pass-after verified by reinstating the `Dispose()` as a negative control. Recorded here (though closed) because it is a product-bug instance of the disposal-race class, useful precedent for future stream-lifecycle work. | Closed 2026-08-15. | | 48 | **Docker rig has no running site instances — live stream probes have no traffic** | Found during the R2 close-out live probe (2026-08-15) | All 7 seeded instances are `soak-*` rows whose deploy fails pre-deployment validation (34 errors: ScriptCompilation ×4, ConnectionBinding ×30 — their bound data connections no longer exist on the sites), and `debug snapshot` returns `instanceNotFound`; `scadabridge_site_connection_up` is 0 on all six site nodes. Consequence: the R2 batching histogram (`scadabridge.site.stream.batch_size`) could not be observed live — batching verification rests on its integration/wire-skew/latency tests, which are thorough. Fix: re-seed a small working instance + data connection set (OPC UA sim is still up) so future live probes have traffic. | Next rig session, or before any live probe that needs event traffic | | 49 | **Sweep code-review "Resolved" entries that cached or lifetime-widened a DI-scoped service** *(Low, process)* | Found while fixing row 45: NotificationOutbox-006, a Low perf nit, introduced the row-45 Medium correctness bug, and its regression test actively pinned the broken behavior — written before `MaxParallelDeliveries` existed and never revisited when parallel delivery landed | The failure shape generalizes: a review resolution that widens a scoped service's lifetime is correct at the time and silently invalidated by a later concurrency feature. Sweep `code-reviews/*/findings.md` resolved entries for caching/lifetime-widening of scoped services and re-check each against features added after its date. | Next code-review or test-infra session | | 50 | **No guard against capturing a scoped service on a singleton actor (captive dependency)** *(Low)* | Found while fixing row 45 | The actors take root `IServiceProvider` rather than typed services, which defeats .NET's built-in scope validation — the row-45 class of bug is invisible at build time. Cheap mitigations: `ValidateScopes`/`ValidateOnBuild` on the central host's provider in Development, and/or a test asserting no `ReceiveActor` field holds an `IServiceScope`. | Next DI/host-hardening session | +| 51 | **`BundleLockedException` reaches the CLI as "An internal error occurred"** *(Low, pre-existing)* | Found while fixing row 44 | `ManagementService` doesn't reference `Transport`, so `MapFault` can't curate the 3-strike lockout — a CLI operator gets a correlation id, not the reason. Row 44 solved this shape for the throttle exception by placing it in Commons; `BundleLockedException` needs the same move (public type relocation, low risk, deliberately kept out of the row-44 diff). | Next Transport/CLI session | +| 52 | **The unlock throttle's counters are per-node and per-process** *(Low, by design but recorded)* | Row 44 implementation note; documented in `docs/components/Transport.md` | Each central node has its own limiter, so the effective fleet cap is 10/hour/IP per node, and a restart clears the window. Acceptable for the threat model today; a shared store is the hardening if it ever matters. | A hardening pass that wants a fleet-wide cap | +| 53 | **`MaxConcurrentImportSessions` has no observability** *(Low)* | Found while fixing row 43 — R6 could only infer the leak from rotating test failures | Nothing counts open sessions or logs when `Open` rejects at the cap. A gauge (mind the `ObservedMeters` allowlist) or a Warning log on rejection would make a recurrence diagnosable in seconds. | Next Transport or observability session | ## Resolved (verified against the code 2026-07-10) Rows removed from the Deferred table above once confirmed shipped. Kept here for traceability.