chore(docker+register): refuse worktree deploys (relative bind mounts rehome rig data — register row 68); record row 67 (startup reconcile stores config but starts no InstanceActor) + row-48 postscript
This commit is contained in:
Joseph Doherty
@@ -3,6 +3,14 @@ set -euo pipefail
|
||||
|
||||
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
|
||||
|
||||
# Same guard as docker/deploy.sh: relative bind mounts mean a worktree deploy silently
|
||||
# rehomes every node's data directory to the worktree's copy, splitting rig state.
|
||||
if [[ "$SCRIPT_DIR" == *"/.claude/worktrees/"* && "${SCADABRIDGE_ALLOW_WORKTREE_DEPLOY:-}" != "1" ]]; then
|
||||
echo "ERROR: refusing to deploy from a git worktree ($SCRIPT_DIR)." >&2
|
||||
echo "Deploy from the main checkout, or set SCADABRIDGE_ALLOW_WORKTREE_DEPLOY=1 to override." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "=== ScadaBridge Env2 Docker Deploy ==="
|
||||
|
||||
# Reuse the primary build (same scadabridge:latest image, same network creation)
|
||||
|
||||
@@ -3,6 +3,17 @@ set -euo pipefail
|
||||
|
||||
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
|
||||
|
||||
# The compose file's bind mounts are RELATIVE (./site-a-node-a/data etc.), so running
|
||||
# this script from a git worktree silently rehomes every node's data directory to the
|
||||
# worktree's copy — the rig keeps running, but its SQLite stores split from the main
|
||||
# checkout's and all state written there is lost on the next deploy from main.
|
||||
if [[ "$SCRIPT_DIR" == *"/.claude/worktrees/"* && "${SCADABRIDGE_ALLOW_WORKTREE_DEPLOY:-}" != "1" ]]; then
|
||||
echo "ERROR: refusing to deploy from a git worktree ($SCRIPT_DIR)." >&2
|
||||
echo "Bind-mount data dirs would rehome to the worktree copy and split rig state." >&2
|
||||
echo "Deploy from the main checkout, or set SCADABRIDGE_ALLOW_WORKTREE_DEPLOY=1 to override." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "=== ScadaBridge Docker Deploy ==="
|
||||
|
||||
# Build image (creates network if needed)
|
||||
|
||||
@@ -41,7 +41,7 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round
|
||||
| 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**~~ **RESOLVED 2026-08-15 (`09108018`).** Not a CLI bug: `ManagementActor.HandleQueryAuditLog` returned the repository's ValueTuple `(Entries, TotalCount)` straight to `SerializeResult`; System.Text.Json writes properties only and a ValueTuple's `Item1`/`Item2` are **fields**, so the whole payload serialized to the literal `{}` — every filter combination looked empty with no error anywhere in the stack. Fixed with a named Commons record `AuditLogQueryResult` (mirroring `SecuredWriteListResult`) plus a class-wide guard: `SerializeResult` now throws on any `ITuple` result, so the next tuple-returning handler fails loudly instead of silently emitting `{}`. Fail-before verified (test asserted exactly `"{}"` pre-fix); live re-verified on the rig (`totalCount 2725` unfiltered, `712` for `--entity-type Template --action Delete`). The only other tuple-returning repo method in Commons (`INotificationOutboxRepository.QueryAsync`) is never serialized. | Found by R6 against the rig; underlying `AuditLogEntries` rows verified present | Closed 2026-08-15. | — |
|
||||
| 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**~~ **RESOLVED 2026-08-15 (`b5e6821c`).** Working set re-seeded on site-a entirely via the CLI (`soak-*` rows left alone): template `RigProbe` + instance `rig-probe-1`, bound to the OPC PLC sim's built-in moving tags (`SpikeData`, `RandomSignedInt32`) plus a 5s heartbeat script. Deploy Success; `debug snapshot` returns live Good-quality data; `health site` shows `OPC PLC Simulator: Connected`, `good:2 bad:0 uncertain:0`; a 60s `debug stream` produced 239 events; and **`scadabridge.site.stream.batch_size` recorded live for the first time** (count 70 / sum 238, batches to ~10 events) — the R2 batching histogram is now observable on the rig, which was this row's purpose. Reproducible via `docker/seed-live-probe.sh`. Two diagnostic corrections worth keeping: the custom tags in `infra/opcua/nodes.json` are declared but **not simulated** by opc-plc (a healthy-looking connection with zero traffic — bind to the built-ins: `SpikeData`, `RandomSignedInt32`, `StepUp`, `FastUInt1..5`, `SlowUInt1..5`); and `scadabridge_site_connection_up` counts open central→site gRPC subscriber streams, not data connections, so this row's original "0 on all six site nodes" was healthy steady state, not the fault (row 54). | Found during the R2 close-out live probe (2026-08-15) | Closed 2026-08-15. | — |
|
||||
| 48 | ~~**Docker rig has no running site instances — live stream probes have no traffic**~~ **RESOLVED 2026-08-15 (`b5e6821c`).** Working set re-seeded on site-a entirely via the CLI (`soak-*` rows left alone): template `RigProbe` + instance `rig-probe-1`, bound to the OPC PLC sim's built-in moving tags (`SpikeData`, `RandomSignedInt32`) plus a 5s heartbeat script. Deploy Success; `debug snapshot` returns live Good-quality data; `health site` shows `OPC PLC Simulator: Connected`, `good:2 bad:0 uncertain:0`; a 60s `debug stream` produced 239 events; and **`scadabridge.site.stream.batch_size` recorded live for the first time** (count 70 / sum 238, batches to ~10 events) — the R2 batching histogram is now observable on the rig, which was this row's purpose. Reproducible via `docker/seed-live-probe.sh`. Two diagnostic corrections worth keeping: the custom tags in `infra/opcua/nodes.json` are declared but **not simulated** by opc-plc (a healthy-looking connection with zero traffic — bind to the built-ins: `SpikeData`, `RandomSignedInt32`, `StepUp`, `FastUInt1..5`, `SlowUInt1..5`); and `scadabridge_site_connection_up` counts open central→site gRPC subscriber streams, not data connections, so this row's original "0 on all six site nodes" was healthy steady state, not the fault (row 54). Close-out postscript: the seeding agent's live verification ran against worktree-homed data dirs (row 68), so after the orchestrator's redeploy from main the config was absent at boot and re-fetched by the startup reconcile — which exposed row 67 — and `rig-probe-1` was re-verified live on the main checkout's data dirs (snapshot Good ×3, heartbeat ticking, sim values moving). | Found during the R2 close-out live probe (2026-08-15) | Closed 2026-08-15. | — |
|
||||
| 49 | ~~**Sweep code-review "Resolved" entries that cached or lifetime-widened a DI-scoped service**~~ **RESOLVED 2026-08-15 — swept, no further bug found (`75520bee`).** All 576 resolved findings across the 25 component findings files were re-read for the row-45 generalization (a resolution that caches, hoists, or widens the lifetime of a DI-resolved service, later invalidated by a concurrency/lifetime feature). Nine matched or nearly matched the shape; **NotificationOutbox-006 is the only one that ever became a bug** and was already fixed as row 45. The other eight verify STILL-SAFE or SUPERSEDED-BENIGN — the two with real concurrency nearby each anticipated it explicitly (`DeploymentService`'s `dbGate` serialises the one repository touch inside the bounded-parallel Send phase; `ArtifactDeploymentService` builds all per-site commands serially before fanning out, commenting "DbContext is not thread-safe"). An independent product-code cross-check confirms the pattern is absent: no `IServiceScope` is stored on a field anywhere in `src/` (every long-lived holder takes `IServiceScopeFactory` and opens a scope per operation), every `Task.WhenAll`/`Parallel.*` site either touches no scoped service or opens a per-operation scope, and no scoped-registered service is captured by a singleton. One stale archive corrected: NotificationOutbox-004 was marked Resolved with an `_Unresolved._` body; the real resolution is now recorded. Near-miss shapes recorded as rows 58–60. | Found while fixing row 45 | Closed 2026-08-15. | — |
|
||||
| 50 | ~~**No guard against capturing a scoped service on a singleton actor (captive dependency)**~~ **RESOLVED 2026-08-15 (`00a804e6`).** Both mitigations shipped, and they are complementary. (1) `ValidateScopes`/`ValidateOnBuild` in Development on **both** role branches via `Host/ServiceProviderValidation.cs` — written out rather than left to `CreateBuilder`'s silent default; production stays unvalidated deliberately (fail-closed at boot). Switching it on surfaced **two real Site-role defects**: `ReconcileService` and `AuditLogKpiSampleSource` were registered on sites but need central-only repositories, so neither could ever be constructed there — one unconstructible descriptor fails `ValidateOnBuild` for the entire site graph, which is exactly why `SiteHealthEndpointTests` had quietly pinned `UseEnvironment("Production")` to dodge validation. Both moved to central-only registrations (`AddCentralCommunication()` / `AddAuditLogCentralKpi()`); the workaround is gone and the site graph now boots validated. (2) `CaptiveDependencyGuard` reflects over all 28 shipped actor classes and fails on any instance field (backing fields included) typed `IServiceScope`, `DbContext`, or Scoped-in-the-real-containers — the scoped set is derived from the built Central and Site graphs, so a lifetime change auto-re-scopes the check; `IServiceProvider`/`IServiceScopeFactory` fields are allowed (the correct pattern). Proven non-vacuous via permanent positive-control dummy actors. No live violation; the two test-only `_injectedRepository` ctor fields allowlisted with written safety arguments plus a staleness self-test (row 61). Doc: REQ-HOST-11 in `Component-Host.md`. | Found while fixing row 45 | Closed 2026-08-15. | — |
|
||||
| 51 | ~~**`BundleLockedException` reaches the CLI as "An internal error occurred"**~~ **RESOLVED 2026-08-15 (`b449fe20`).** Relocated to `Commons/Types/Transport/` beside `BundleUnlockThrottledException` (row 44's pattern); both consumers already carried the namespace, so no call-site edits. `ManagementActor.HandlePreviewBundle`/`HandleImportBundle` now rethrow it as `ManagementCommandException`, so CLI and UI print the lockout reason instead of a correlation id. Fault-mapping tests mirrored from row 44 on both handlers. Follow-up in the same diff (`73f518d6`): deleted the dead `BundleUnlockRateLimitedException` (superseded by row 44, zero references — three similarly-named unlock exceptions with one inert was a live trap). | Found while fixing row 44 | Closed 2026-08-15. | — |
|
||||
@@ -60,6 +60,8 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round
|
||||
| 64 | **The fleet-wide unlock throttle's count-then-insert is not atomic across central nodes** *(Low, accepted by design)* | Row-52 implementation trade, documented in both Transport docs | Simultaneous attempts can overshoot `MaxUnlockAttemptsPerIpPerHour` by the number in flight; bounded and self-correcting within the window. A conditional single-statement insert would close it at the cost of provider-specific raw SQL. | A threat model needing a hard cap rather than a rate damper, or evidence of a real brute-force exploiting the overshoot |
|
||||
| 65 | **`BundleUnlockAttempt` rows are pruned only by nodes actively serving unlocks** *(Low)* | Row-52 implementation trade | The prune rides the unlock path, so a cluster that stops importing entirely leaves the last rows in place indefinitely (bounded and tiny: one row per permitted attempt, retained 2× the window). | The table appears in a size review, or Transport gains a central scheduler a prune tick could ride |
|
||||
| 66 | **No counter for import-session cap rejections** *(Low)* | Row-53 implementation note | Row 53 shipped the open-sessions gauge and a Warning log, so a rejection is diagnosable from logs, but there is no series to alert on. | Someone wants an alert rule on import-session exhaustion rather than a log search |
|
||||
| 67 | **Startup reconcile stores a fetched instance config but does not start the InstanceActor** *(Medium-Low, availability)* | Proven live during the rows-46–53 close-out (2026-08-15): site-a booted without `rig-probe-1`'s config, `SiteReconciliationActor`'s node-STARTUP fetch stored it ("Reconcile: fetched + stored config"), but no `InstanceActor started` followed — `debug snapshot` returned `instanceNotFound` and the enabled, successfully-deployed instance stayed dead until the NEXT restart, whose SQLite load finally started it | The reconcile heal path heals the *file*, not the *runtime*: an instance whose config is missing at boot (lost volume, first boot after restore, rehomed data dir) needs two restarts to run again. Narrow trigger, self-healing, but silent — nothing surfaces "config stored, actor not running". Fix shape: after a startup-reconcile fetch stores a config for an enabled instance with no running actor, tell the DeploymentManager to start it (or re-run the SQLite load path for the fetched set). | Next SiteRuntime/reconcile session |
|
||||
| 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)
|
||||
Rows removed from the Deferred table above once confirmed shipped. Kept here for traceability.
|
||||
|
||||
Reference in New Issue
Block a user