From 4da5f7cd7a4a212b910785d6b84bb8df9c8b3b3c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 07:04:40 -0400 Subject: [PATCH] =?UTF-8?q?docs(register):=20close=20row=20101=20=E2=80=94?= =?UTF-8?q?=20stale-snapshot=20race=20verified=20real=20(worse=20than=20gu?= =?UTF-8?q?essed:=20spurious=20removal=20is=20NOT=20self-healing),=20defin?= =?UTF-8?q?itions-write=20generation=20guard=20shipped=20in=200d3a90a9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/2026-07-08-deferred-work-register.md | 2 +- 1 file changed, 1 insertion(+), 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 43770777..0c6099aa 100644 --- a/docs/plans/2026-07-08-deferred-work-register.md +++ b/docs/plans/2026-07-08-deferred-work-register.md @@ -93,7 +93,7 @@ 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** *(Low, flake + possible production race — UNVERIFIED hypothesis)* | Observed by the 2026-08-17 row-100 fix session (one failure in its first SiteRuntime.Tests run, on the pristine `dcfcf822` code path; passed in isolation and in 3 subsequent full runs + this wave's full suite) | The test received `RemoveConnectionCommand("PlcA")` where `"PlcGone"` was expected — and `RemoveConnectionCommand("PlcA")` can only come from `ApplyConnectionDefinitionOverlay`'s disappearance detection, i.e. the periodic sweep's `ReadStoreSnapshotAsync` racing the artifact-apply write and momentarily not seeing `PlcA`'s definitions row. If that mechanism is real it is not test-only: in production the same race would transiently remove a LIVE connection on a sweep tick that straddles an artifact apply (self-healing at the next tick via re-create, but a needless data gap). Verify the read/write ordering inside `DeploymentManagerActor` first — if the store read and the apply are on the same actor's timeline the hypothesis is wrong and this is a pure test-fixture race. | Next SiteRuntime/DeploymentManagerActor session, or a second occurrence of this fingerprint | +| 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. | — | | 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)