docs(localdb): phase 2 truth pass across both repos
Normative first: Component-StoreAndForward.md:83 specified the whole chunked, ack-confirmed SfBufferSnapshotChunk resync protocol, which Phase 2 deleted. It is rewritten rather than removed — the failure modes it reasoned about still exist, they are just bounded differently — and it now states the duplicate-delivery bound explicitly: a message can be delivered twice only when the OLD primary delivered it and the status change had not yet replicated when the gate flipped. One flush interval plus the in-flight ack, and unlike the old model it does NOT grow with backlog depth or with how long a node was absent. The N1 directional-authority and N5 orphan-row hazards are recorded as structurally gone, not merely unguarded. Frame-size known-issue amended: its 2026-06-26 resolution replaced the intra-site hop with notify-and-fetch; Phase 2 then deleted notify-and-fetch itself, so the 128 KB Akka frame constraint no longer applies to that hop in any form. Successor ceiling recorded (4 MB gRPC cap via MaxBatchSize, which batches by ROW COUNT), including that the failure mode differs — an oversized gRPC message is rejected, not silently dropped. Deployment docs gain the two operational constraints that have no home in code: a site pair must be stopped and started TOGETHER (the SfBufferSnapshot compat handler that made a mixed-version pair converge went with the replicator, and a mixed pair now diverges silently), and a node offline beyond TombstoneRetention can resurrect deleted rows on rejoin. Both CLAUDE.md files corrected — each still said Phase 2 was NOT started. Definition of done closed: build 0 warnings, all 10 suites green (3509 tests, 0 failures, 0 skips). Two DoD items needed amending rather than ticking: the stale-symbol grep still matches 4 lines, all deliberate comment prose recording what was deleted (a literal zero would delete the explanations that stop the old design coming back), and the live gate has 10 evidence items, not the 9 the checklist claimed. Deletes the phase2 resume-state scratch doc, which said to delete it on landing. Claude-Session: https://claude.ai/code/session_01BL2Vu1ESDQ9SCN4gVKkdts
This commit is contained in:
Joseph Doherty
@@ -1186,10 +1186,22 @@ Specific corrections:
|
||||
|
||||
## Definition of done
|
||||
|
||||
- [ ] Solution builds with **0 warnings** (`TreatWarningsAsErrors` is on)
|
||||
- [ ] Every suite green: Host, CentralUI, Commons, SiteRuntime, AuditLog, Communication,
|
||||
StoreAndForward, HealthMonitoring, IntegrationTests, SiteEventLogging
|
||||
- [ ] `grep -rn "SiteReplicationActor\|ReplicationService\|SfBufferSnapshot" src/` → no matches
|
||||
- [ ] Live gate PASS with all 9 evidence items captured
|
||||
- [ ] Convergence suite verified non-vacuous
|
||||
- [ ] Both `CLAUDE.md` files updated
|
||||
All met, 2026-07-20.
|
||||
|
||||
- [x] Solution builds with **0 warnings** (`TreatWarningsAsErrors` is on)
|
||||
- [x] Every suite green — Host 330, CentralUI 925, Commons 684, SiteRuntime 512, AuditLog 355,
|
||||
Communication 312, StoreAndForward 130, HealthMonitoring 97, SiteEventLogging 70,
|
||||
IntegrationTests 94. 0 failures, 0 skips.
|
||||
- [x] `grep -rn "SiteReplicationActor\|ReplicationService\|SfBufferSnapshot" src/` → **no code
|
||||
references.** Four matches remain and are all deliberate: prose inside `///` and `//` comments
|
||||
in `ActiveNodeEvaluator`, `SiteLocalDbSetup` and `AkkaHostedService` that records what was
|
||||
deleted and why. **The check as written is too blunt** — a literal "no matches" would force
|
||||
deleting the very explanations that stop someone re-introducing the old design. Read it as
|
||||
"no references from code".
|
||||
- [x] Live gate PASS — **10** evidence items, not the 9 this checklist says (Task 20 itself lists
|
||||
ten). All captured in `docs/plans/2026-07-19-localdb-phase2-live-gate.md`, with two scope
|
||||
caveats recorded there rather than glossed.
|
||||
- [x] Convergence suite verified non-vacuous — with the eight `RegisterReplicated` calls commented
|
||||
out, all four Task 18 scenarios go red (4 failed / 0 passed); restored, 20/20 across the three
|
||||
LocalDb suites.
|
||||
- [x] Both `CLAUDE.md` files updated
|
||||
|
||||
@@ -88,8 +88,8 @@
|
||||
{"id": 17, "subject": "Task 17: Config-key cleanup (incl. ConfigFetchRetryCount; 10 appsettings files)", "status": "completed", "classification": "standard", "blockedBy": [16], "note": "As planned. DEVIATION: deploy/wonder-app-vd03/appsettings.Site.json sits under a GITIGNORED deploy/ tree, so its edit is local-only and must be repeated on the box at deploy time. Comment style is // (JSONC) rather than \"_comment_\" keys: every one of these files already contains // comments and .NET's json config reader accepts them, and a _comment_ key inside a bound section is a phantom config entry. Both relaxations pinned by the INVERSE of the test they replace (Site_MissingSiteDbPath_IsAccepted..., EmptySqliteDbPath_IsAccepted...), each verified red with the old rule restored. DatabaseOptionsValidator needed no change - it was already null-tolerant/blank-rejecting, which is exactly migration-only semantics.", "commit": "605e5682"},
|
||||
{"id": 18, "subject": "Task 18: Two-node convergence suite for the Phase 2 tables", "status": "completed", "classification": "high-risk", "blockedBy": [17], "note": "DEVIATION: landed as a NEW file LocalDbPhase2ConvergenceTests.cs rather than extending Phase 1's LocalDbSitePairConvergenceTests.cs, and drives the REAL SiteStorageService instead of hand-written SQL - possible only post-cutover, and what makes the cascade scenario test the shipped transaction rather than a re-creation of it. Scenario 4 was retargeted onto shared_scripts/external_systems/static_attribute_overrides because the plan's version overlapped LocalDbConfigConvergenceTests' N1 scenario almost exactly; the union-survives property is per-table, so re-proving it on untouched tables is the non-redundant half. Cascade test carries a never-removed control instance (absence assertions otherwise cannot distinguish 'cascade converged' from 'node B lost these tables'). Non-vacuity PROVEN as mandated: with the 8 RegisterReplicated calls commented out, 4 failed / 0 passed; restored, 20/20 across the three LocalDb suites.", "commit": "15013156"},
|
||||
{"id": 19, "subject": "Task 19: Rig configuration (MaxOplogRows/MaxOplogAge + MaxBatchSize per D6)", "status": "completed", "classification": "small", "blockedBy": [17], "note": "MaxBatchSize 500->16 (D6: row-count batching x ~70 KB production config_json vs the 4 MB gRPC cap; 16 => ~1.1 MB worst case). MaxOplogRows 1M->250,000 and MaxOplogAge 7d->2d from the soak's 0.80 rows/sec (~69k/day). Tighter-than-default is SAFE because a cap breach prunes + sets needs_snapshot (graceful snapshot resync), not data loss - the Task 1 finding that the plan's stop condition was weaker than written. site-b/site-c left unreplicated so default-OFF stays proven side by side.", "commit": "921edab4"},
|
||||
{"id": 20, "subject": "Task 20: Live gate on the docker rig (10 evidence items)", "status": "pending", "classification": "high-risk", "blockedBy": [18, 19], "note": "Includes the post-cutover oplog drain-under-churn check (evidence 10) that Task 1 cannot measure pre-cutover. Sample DBs via throwaway copies (snap() helper), never host-side sqlite3 on live files; metrics on :8084 in-container."},
|
||||
{"id": 21, "subject": "Task 21: Documentation truth pass", "status": "pending", "classification": "standard", "blockedBy": [20]}
|
||||
{"id": 20, "subject": "Task 20: Live gate on the docker rig", "status": "completed", "classification": "high-risk", "blockedBy": [18, 19], "note": "ALL 10 CHECKS PASS. Evidence: docs/plans/2026-07-19-localdb-phase2-live-gate.md. Key blocker found and fixed mid-run: external systems reach a site ONLY via ArtifactDeploymentService, which `instance deploy` never invokes - `deploy artifacts` was needed both to deliver the probe harness and to propagate the owed ExternalSystemDefinitions restore. THREE METHOD CORRECTIONS to the plan: (1) its instruction to run DB checks host-side against the bind mounts is UNSAFE - host sqlite3 poisons the container WAL; copy the db/-wal/-shm triplet and query the copy. (2) `docker exec ... curl` cannot scrape metrics (no curl in aspnet:10.0) and with 2>/dev/null the failure is silent - it nearly became a false 'metrics missing' finding; use a network-sharing curl sidecar. (3) checks needing S&F load need CachedCall, not Call. CAVEATS recorded not glossed: check 2's zero-count is vacuous alone (legacy source was also empty) and rests on the ABSENCE of CDC triggers; check 7's native_alarm_state leg was empty live and is covered only offline; check 10's sampling is coarse and the real rise/drain evidence comes from check 6's 0->4->0.", "commit": "158e79bb"},
|
||||
{"id": 21, "subject": "Task 21: Documentation truth pass", "status": "completed", "classification": "standard", "blockedBy": [20], "note": "Both CLAUDE.md files + Component-StoreAndForward.md:83 (normative resync paragraph rewritten for CDC, stating the duplicate-delivery bound explicitly: limited to messages the OLD primary delivered whose status change had not yet replicated when the gate flipped - one flush interval plus in-flight ack, and it does NOT grow with backlog depth or absence duration) + components/{StoreAndForward,SiteRuntime,Host}.md + the frame-size known-issue (amended: Phase 2 deleted notify-and-fetch itself, so the 128KB Akka frame constraint is gone from the intra-site hop entirely; successor ceiling is the 4MB gRPC cap via MaxBatchSize, and note the failure mode differs - oversized gRPC is REJECTED, not silently dropped) + deployment topology-guide.md and installation-guide.md (D5 stop-both-together, D2 TombstoneRetention resurrection bound). DoD closed: build 0 warnings, all 10 suites green (3509 tests, 0 failures). DoD grep nuance recorded in the plan: 4 matches remain in src/ and are all deliberate COMMENT prose explaining what was deleted - a literal 'no matches' would delete the explanations that stop someone re-introducing the old design.", "commit": null}
|
||||
],
|
||||
"knownFlakes": [
|
||||
{
|
||||
|
||||
@@ -1,198 +0,0 @@
|
||||
# LocalDb Phase 2 — resume state (updated 2026-07-20, pre-compaction #2)
|
||||
|
||||
Scratch handoff for picking the plan back up. Delete once Phase 2 lands.
|
||||
|
||||
**Plan:** [`2026-07-19-localdb-adoption-phase2.md`](2026-07-19-localdb-adoption-phase2.md)
|
||||
(execute with `superpowers-extended-cc:executing-plans`; task state in
|
||||
`…-phase2.md.tasks.json`, which carries a per-task deviation note)
|
||||
**Branch:** `feat/localdb-phase2` (from `feat/localdb-phase1`, which is **not** merged)
|
||||
**Tree:** clean @ `0ad11d6b`. Build 0 warnings. 25 commits ahead of `origin/feat/localdb-phase2`.
|
||||
|
||||
---
|
||||
|
||||
## Where we are
|
||||
|
||||
**Tasks 1–16 are DONE. The cutover has landed. Next up: Task 17.**
|
||||
|
||||
| Task | State |
|
||||
|---|---|
|
||||
| 1–6 | done (waves 0–2) — see the git log |
|
||||
| 7 `SiteLocalDbSetup` DDL | done |
|
||||
| 8 `sf_messages` migrator | done |
|
||||
| 9 config-table migrator | done |
|
||||
| 10 S&F CDC convergence specs | done |
|
||||
| 11 resync + config convergence specs | done |
|
||||
| 12 active-node purge pin | done |
|
||||
| 13 guarded-write scope check | done |
|
||||
| **14 + 15 + 16 the CUTOVER** | **done — landed as ONE commit `037798b3`** |
|
||||
| **17–21** | **not started** |
|
||||
|
||||
**Verification at the cutover boundary:** build 0 warnings; SiteRuntime 512, StoreAndForward 130,
|
||||
Host 330, AuditLog 355, ExternalSystemGateway 142, HealthMonitoring 97, LocalDb integration 16 —
|
||||
**all pass, 0 failures.** (SiteRuntime/StoreAndForward fell from 533/153 as 6 obsolete test files
|
||||
were deleted.)
|
||||
|
||||
### The bespoke replicators are gone
|
||||
|
||||
`SiteReplicationActor`, `ReplicationMessages.cs`, `ReplicationService` and
|
||||
`StoreAndForwardStorage.ReplaceAllAsync` are deleted. Ten tables now replicate via LocalDb CDC:
|
||||
the Phase 1 pair plus `sf_messages` and the 7 site config tables.
|
||||
|
||||
`notification_lists` and `smtp_configurations` are created but **never registered** — permanently
|
||||
empty by design, and registering them would open a standing replication channel whose only
|
||||
historical payload was plaintext SMTP passwords. That decision is pinned by its own
|
||||
security-named test and stated in `OnReady`.
|
||||
|
||||
---
|
||||
|
||||
## Next: Task 17 (config-key cleanup)
|
||||
|
||||
Partly **required**, not optional: `SiteRuntimeOptions.cs:66` still documents
|
||||
`ConfigFetchRetryCount` as "Consumed by `SiteReplicationActor`", which no longer exists.
|
||||
|
||||
Per the plan: relax `StartupValidator.cs:113` (`SiteDbPath`) and
|
||||
`StoreAndForwardOptionsValidator.cs:19-21` (`SqliteDbPath`) to migration-only; delete
|
||||
`ReplicationEnabled` and `ConfigFetchRetryCount` outright with their validator rules and config
|
||||
entries; **leave the two path keys present in configs** with a comment — the legacy migrator reads
|
||||
them, and removing them would strand un-migrated data on a node that has not yet started once.
|
||||
**10** `appsettings.Site.json` files, including `deploy/wonder-app-vd03/`.
|
||||
|
||||
Then 18 (two-node convergence suite), 19 (rig config — `MaxBatchSize = 16`), 20 (live gate),
|
||||
21 (docs truth pass).
|
||||
|
||||
---
|
||||
|
||||
## Findings that change the plan (carry these forward)
|
||||
|
||||
### From Task 1 (unchanged)
|
||||
|
||||
- **D6's premise is wrong, its risk is real.** Largest known production `config_json` is ~60–70 KB,
|
||||
not >128 KB → no stop condition. But `MaxBatchSize=500` × 70 KB ≈ 35 MB vs a 4 MB gRPC cap →
|
||||
**Task 19 must set `LocalDb:Replication:MaxBatchSize = 16`.** The one firmly evidence-backed number.
|
||||
- **D4 is wrong.** `native_alarm_state` is bounded by per-`SourceReference` coalescing on a 100 ms
|
||||
flush (`NativeAlarmActor.cs:473-502`).
|
||||
- **`sf_messages` has a hard 50 rows/sec ceiling** — `SweepBatchLimit` (500) ÷ `RetryTimerInterval` (10 s).
|
||||
- **Task 1 step 7's stop condition is weaker than written.** Exceeding the oplog caps prunes and
|
||||
flags `needs_snapshot` → graceful resync, not data loss.
|
||||
|
||||
Measured (six 60 s intervals, copy-based `snap()`): `sf_messages` **0.80 rows/sec** insert, **0/sec**
|
||||
retry-UPDATE, oplog 0, max `payload_json` 76 B, max `config_json` 721 B (rig — *not* representative),
|
||||
zero SQLite errors in 30 min.
|
||||
|
||||
### From waves 1–2
|
||||
|
||||
- **LATENT PHASE-1 DEFECT, fixed in-app only.** The LocalDb library never creates the parent
|
||||
directory of `LocalDb:Path`, and `SqliteLocalDb` opens the file **eagerly in its constructor**, so
|
||||
a missing directory is a **hard boot failure** (`SQLite Error 14`). The default site config uses
|
||||
the **relative** `./data/site-localdb.db`; the docker rig escapes only because its volume mount
|
||||
creates `/app/data`. Fixed via `SiteLocalDbDirectory.Ensure(config)` before `AddZbLocalDb`, pinned
|
||||
by `Host.Tests/SiteLocalDbDirectoryTests`.
|
||||
**OPEN DECISION FOR THE USER:** the real fix arguably belongs in the `ZB.MOM.WW.LocalDb` library
|
||||
— **every other consumer still has this gap.** Not done here: a library change means a version
|
||||
bump and is outside this plan's scope.
|
||||
- **`SiteStorageService.CreateConnection()` contract flipped** — returns an **already-open**
|
||||
connection; calling `Open`/`OpenAsync` on one **throws**.
|
||||
- **LocalDb has NO in-memory mode.** Every `Mode=Memory;Cache=Shared` fixture became a real temp file.
|
||||
- **A store constructor change's test blast radius is ~7× the plan's estimate** (Task 5: 40 files,
|
||||
7 projects).
|
||||
- **`tests/ZB.MOM.WW.ScadaBridge.TestSupport`** holds `TestLocalDb` (dispose **before** deleting —
|
||||
the master connection anchors the WAL). Use it; don't copy fixtures.
|
||||
- **Where behaviour moved owner, retarget tests — don't delete them with the code.** WAL is genuinely
|
||||
LocalDb's; directory creation is **not**. Both directions occurred.
|
||||
|
||||
### From waves 3–5 (new)
|
||||
|
||||
- **PLAN DEFECT — Tasks 14/15/16 cannot compile separately.** `SiteReplicationActor` takes a
|
||||
`ReplicationService` and calls `ReplaceAllAsync`; `DeploymentManagerActor` Tells types from
|
||||
`ReplicationMessages.cs`; `AkkaHostedService` constructs the actor. Landed as one commit, which
|
||||
also strengthens Task 14's own invariant (never both mechanisms, never neither).
|
||||
- **The legacy-copy column intersection.** The plan said "migrate all 16 columns"; that would be a
|
||||
data-loss bug. An older legacy file lacks `execution_id`/`parent_execution_id`/
|
||||
`last_attempt_at_ms`, naming a missing column throws `no such column`, and the existing reader
|
||||
treats that as an unrecognised shape and **silently discards every row**. The copy intersects the
|
||||
legacy and current column sets, with a required-column (PK) guard so the tolerance cannot degrade
|
||||
into copying NULL-keyed rows.
|
||||
- **`MigratorColumnLists_MatchTheLiveSchema`** (added beyond the plan). A column-list mismatch is
|
||||
invisible at runtime in BOTH directions — a typo is silently dropped by the intersection, a
|
||||
missing column silently leaves data behind. `SiteStorageTables`/`LegacyTable` are `internal` for it.
|
||||
- **Absence assertions need a positive control.** `MessageAddedThenRemoved_NeverConvergesToPresent`
|
||||
PASSED with `sf_messages` unregistered entirely — an absent row is also what a pair replicating
|
||||
nothing looks like. Fixed with a control row that must converge in the same window.
|
||||
- **The positional-argument hazard was REAL.** Removing `DeploymentManagerActor`'s optional
|
||||
`IActorRef? replicationActor` shifted 6 trailing optionals and 4 test call sites bound the wrong
|
||||
arguments, some with no compile error. **`Props.Create` builds an expression tree, which rejects
|
||||
OUT-OF-POSITION named arguments** — so named args work only if in position, and the rest must be
|
||||
padded positionally.
|
||||
- **`ReplaceAllAsync` was unsafe to keep, not merely unused.** A mass DELETE on a now-replicated
|
||||
table would be captured and shipped to the peer.
|
||||
- **`LocalDbSitePairHarness`** is the shared two-node fixture (Phase 1 + both Phase 2 suites; Task 18
|
||||
makes four). Its `Phase2ReplicatedTables` list is **literal, not derived from production code**, so
|
||||
a registration that drifts fails the tests instead of agreeing with itself.
|
||||
- **Ordinal sort gotcha:** `_` (0x5F) sorts before `b` (0x62), so `data_connection_definitions`
|
||||
precedes `database_connections`.
|
||||
- **Never `git checkout <file>` to undo a non-vacuity experiment** — it reverts to HEAD and takes
|
||||
in-progress work with it (cost one redo of Task 8). Use `cp` backups.
|
||||
|
||||
---
|
||||
|
||||
## Rig operational gotchas (hard-won; not obvious from the repo)
|
||||
|
||||
- **`ExternalSystem.Call` does NOT buffer to store-and-forward — only `ExternalSystem.CachedCall`
|
||||
does.** The single most important detail for generating S&F load.
|
||||
- **Never** query the bind-mounted SQLite files from the macOS host while containers run. Use the
|
||||
plan's `snap` helper, or `docker run --rm -v <dir>:/d alpine/sqlite3 …`.
|
||||
- **No `curl` in the `aspnet:10.0` image**, and metrics port 8084 is not published. Use a sidecar:
|
||||
`docker run --rm --network container:scadabridge-site-a-a curlimages/curl:latest -s localhost:8084/metrics`
|
||||
- **Seeded template 4 (`Motor Controller`) is not deployable** — 34 validation errors. The soak used
|
||||
a purpose-built `SoakGenerator` template.
|
||||
- CLI: `template script update` requires `--name` and `--trigger-type` even to change only `--code`.
|
||||
In zsh, don't put auth flags in an unquoted variable.
|
||||
- Rig auth: `--url http://localhost:9000 --username multi-role --password password`.
|
||||
`LdapGroupMappings` roles must be canonical `Designer`/`Deployer`; central caches them at startup.
|
||||
|
||||
## Rig state as left
|
||||
|
||||
Reseeded and healthy; both site-a nodes restarted after the poisoning incident.
|
||||
|
||||
**Needs cleanup before the Task 20 live gate:**
|
||||
- `ExternalSystemDefinitions` id 1 is still repointed to `http://127.0.0.1:9` — restore to
|
||||
`http://scadabridge-restapi:5200`.
|
||||
- Template `SoakGenerator` (id 2021) and instances `soakgen-1..4` (ids 5–8) are still deployed on
|
||||
site-a and **still generating load**.
|
||||
|
||||
---
|
||||
|
||||
## Commits (this branch, newest last)
|
||||
|
||||
| Commit | What |
|
||||
|---|---|
|
||||
| `25463d52` … `d512572d` | plan review, soak, root-cause + hardening, first resume doc |
|
||||
| `12eb97e0` | **Task 2** — gate CLOSED |
|
||||
| `9e3239c5` | **Task 3** — `StoreAndForwardSchema.Apply` |
|
||||
| `ac5eb12c` | **Task 4** — `SiteStorageSchema.Apply` |
|
||||
| `f2efeb37` | **Tasks 5+6** — both stores take `ILocalDb`; latent directory defect fixed; `TestSupport` added |
|
||||
| `fefbbb31` | resume state through wave 2 |
|
||||
| `f8aa02e2` | **Task 7** — Phase 2 DDL in the consolidated DB (not yet registered) |
|
||||
| `bdc0dffe` | **Task 8** — migrate legacy `store-and-forward.db` |
|
||||
| `5ddc7eed` | **Task 9** — migrate legacy `scadabridge.db` config tables |
|
||||
| `2bbe6631` | **Task 10** — S&F CDC convergence specs (+ `LocalDbSitePairHarness` extraction) |
|
||||
| `c56bf4ae` | **Task 11** — resync + config convergence specs |
|
||||
| `79ce5161` | **Tasks 12+13** — purge pin; guarded-write scope |
|
||||
| `037798b3` | **Tasks 14+15+16 — THE CUTOVER** |
|
||||
| `0ad11d6b` | task state for the cutover |
|
||||
|
||||
Two known flakes, do not chase: `InstanceActorChildAttributeRaceTests` (intermittent under
|
||||
full-suite load) and `ParentExecutionIdCorrelationTests` (~91 s / times out on a cold MSSQL
|
||||
fixture, ~1 s warm). Neither reproduced during waves 1–5.
|
||||
|
||||
## Execution notes
|
||||
|
||||
- Waves ran **sequentially in the main worktree**, not as the plan's parallel worktree agents — the
|
||||
tasks were small enough that worktree setup + merge cost more than it saved, and sequential
|
||||
execution removes the git race the isolation rule exists to prevent.
|
||||
- Subagents were used only in waves 1–2, for bulk mechanical fixture rewiring. **Their reported
|
||||
results were re-verified independently** — one agent's partial run had missed 2 real failures that
|
||||
a full-suite run caught. Re-run the suites yourself.
|
||||
- Every new test was checked for **non-vacuity** by breaking the thing it claims to pin (removing
|
||||
DDL, removing the `Migrate` call, unregistering a table, commenting out the purge). This caught one
|
||||
genuinely vacuous test. Keep doing it.
|
||||
Reference in New Issue
Block a user