From 226a628f6fad8aba7a628e38f288bec3ebd1a837 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 18 Aug 2026 05:59:42 -0400 Subject: [PATCH] =?UTF-8?q?docs(register):=20row-101=20postscript=20?= =?UTF-8?q?=E2=80=94=20whole-snapshot=20generation=20guard=20shipped=20(al?= =?UTF-8?q?l=20three=20instance-config=20hazards=20proven=20real:=20backwa?= =?UTF-8?q?rd=20restart=20via=20old-ConfigJson=20redeploy,=20delete/disabl?= =?UTF-8?q?e=20resurrection,=20boot=20name-set=20skew;=20both-ends=20bump?= =?UTF-8?q?=20corrects=20the=20completion-only=20ordering=20argument);=20a?= =?UTF-8?q?dd=20row=20102=20(stream-gauge=20fixed-baseline=20flake)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/2026-07-08-deferred-work-register.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/plans/2026-07-08-deferred-work-register.md b/docs/plans/2026-07-08-deferred-work-register.md index 0c6099aa..cb110ebc 100644 --- a/docs/plans/2026-07-08-deferred-work-register.md +++ b/docs/plans/2026-07-08-deferred-work-register.md @@ -93,7 +93,8 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round | 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. | — | | 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. | Observed by the 2026-08-17 row-100 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 | | 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)