Files
ScadaBridge/docs/plans/2026-08-15-arch-review-remediation-execution-log.md
T
Joseph Doherty 491df111ea fix(dcl): derive tag-resolution health counts from per-tag authoritative state
Closes arch-review remediation residual #1 (DCL unsubscribe-during-reconnect
count staleness).

DataConnectionActor tracked TotalSubscribedTags/ResolvedTags as two int fields
incremented and decremented at five independent sites. ReSubscribeAll clears the
very maps those decrements key off (_subscriptionIds, _unresolvedTags) while
deliberately preserving _subscriptionsByInstance, so an unsubscribe landing
inside a reconnect window matched NEITHER decrement branch: the total leaked +1
per subscribe/reconnect/unsubscribe churn cycle, permanently and cumulatively.
The 37f13e2e discard gate stopped the orphan-handle half of that race; it could
not stop the counters drifting, because they were state of their own.

Both counts are now DERIVED at report time from the authoritative per-tag
collections, which makes the drift unrepresentable rather than merely guarded:

  total    = _instancesByTag.Count   (the per-tag counted set the residual
                                      called for — distinct tags with at least
                                      one subscribing instance)
  resolved = _subscriptionIds.Count  (tags for which the adapter holds a handle)

Two semantic corrections fall out of the derivation:

- A tag whose subscribe failed at CONNECTION level now counts toward the total.
  It was excluded before, yet the reconnect re-subscribe re-issued it from
  _subscriptionsByInstance and booked it as resolved — resolved above total, and
  a total driven negative by the eventual unsubscribe.
- _tagSubscriberCount is deleted. It duplicated _instancesByTag exactly, so
  HandleUnsubscribe's last-subscriber test is now "did UnindexTag drop the key?"
  — still O(1), with no parallel count that can disagree about when a handle is
  released. The subscribe-success promotion split (fresh vs. unresolved→resolved)
  also goes: it existed only to pick which scalar to bump; set sizes get
  DataConnectionLayer-020's double-count cases right for free.

Behavior is otherwise unchanged — same logging, same handle release, same
unresolved-tag probing, same in-flight-unsubscribe discard semantics (the long
comment block there is updated for the mechanics that changed).

Tests: five TagResolutionCounts_* cases in DataConnectionActorBatchTests
covering the churn repro (3 cycles), a shared tag losing one instance mid
reconnect, connection-level failure then recovery, plain subscribe/unsubscribe
cycles, and a completed reconnect re-subscribe. Verified failing against the
pre-fix actor (churn: total 1 not 0; connection-level: total 0 not 1) and
passing after. Full DCL suite 319/319; solution builds with 0 warnings.

Docs: Component-DataConnectionLayer.md health-reporting section describes the
derived counts; residuals register item 1 marked RESOLVED.
2026-08-15 02:05:35 -04:00

10 KiB

Arch-review remediation — execution log

Ran: 2026-08-14/15 · Plan: docs/plans/2026-08-14-arch-review-remediation-plan.md · Branch: arch-review-remediation (off main) · Status: code-complete, docs-propagated, not yet pushed/merged.

Source: the 2026-08-14 eight-pass performance/architecture review (all 27 components + ZB.MOM.WW.LocalDb). This log is the compact record of what shipped, how it was gated, and what's left. Full package specs live in the plan; this is the after-action summary.

Phases and package → commit map

Every package ran as an isolated-worktree subagent, merged --no-ff onto arch-review-remediation; commit hashes below are the substantive commit on the branch (merge commits omitted).

Phase 0 — Preflight. 0b201e41 docs(plans): the remediation plan itself.

Phase 1 — Quick wins (7 packages, parallel).

WP Commit What
1.1 34a3f4bb Reconnect on graceful (OK-status) stream completion — closes the 4h silent stream death
1.2 2e4e41a8 Site audit DB onto the mounted data volume; required path + soft flush
1.3 7ebdcd37 CDC capture installed only when replication is configured
1.4 600659d5 Sweep/KPI covering indexes + sliced notification terminal purge
1.5 125055d9 O(1) attribute resolution, precomputed types, coalesced static writes, shared JSON options
1.6 c5e66ed4 Fail known-dead sends immediately instead of burning Ask timeouts
1.7 2cfcd890 Batched event-log commits + sliced retention purge

Phase 2 — Seam rework (6 packages, parallel after the WP2.1a design doc 1040dc0f).

WP Commit What
2.1 d15c5f02 DCL batch subscribe/read/write seam, bounded reconnect, sharded subscriptions
2.2 5db2a810 Central set-based ingest, aligned partition purge, KPI query shapes, EF hygiene
2.3 2ce0ad7e Alarms-only stream seed, capped buffers, at-least-once audit pull
2.4 8c0b36b2 Shared KPI cache, live-cache-backed alarm summary, coalesced Debug View renders
2.5 48b3c40a Flatten-session caching, bulk DeploySiteAsync, paged management queries
2.6 a2122831 Cached hot-path lookups, bounded observer queue, alarm-priority stream path

Plus a5882753 closing Phase-2-gate residuals (direct ingest path, monotonic timeouts, synthetic probe, not-reporting set, cursor-exact audit pull) found while gating.

Phase 3 — Structural (design-first).

WP Design Commit What
3.1 312216ff c4fc1f8e Script execution pool split — trigger evals off the blocking pool, bounded/deadline-aware execution
3.2 6cfb2dd8 c254d074 site_events volume policy — sampled per-run events, interval summaries, replication pinned
3.3 (scadaproj) cca7f178 + scadaproj 9377fa1 LocalDb 0.2.0 — dereg cleanup, late-opt-in baselining, byte-budget replication

Phase 4 — Verification, adversarial review, docs. Six parallel code-reviewer passes over the full diff, one per area (site runtime, DCL, comms, site persistence, central SQL, UI/deploy), each instructed to try to refute the fixes. Confirmed findings landed as targeted follow-ups:

Area Commit What
Site runtime 950c54c5 Recursion-exempt run cap (nested CallScript no longer double-gated), atomic detach counter, summary edge cases, per-row event-log fallback
DCL 37f13e2e Discard in-flight subscribe results for tags unsubscribed mid-flight; release the orphaned handle
Central SQL 5d075f13 No client-side audit truncation, insert-first upsert (Site Call Audit), QI-safe filtered-index scripts, honest operator-not-found replies
Comms fd5e023d Consumer-based debug-stream orphan net, foreign-cancel triad, honest onConnected, served-row-exact retirement, full-rate reconcile
UI/deploy e0e4b246 Honest CLI HTTP timeouts, watermark-complete staleness (3 missed bump sites), phase-2 PendingDeployment staging, lock-safe cancellation
Site persistence 56c99c92 + f689f495 Required audit DB path on wonder; explicit MaxBatchSize:64 LocalDb read-page cap; rate-limited observer drop logging; LocalDb 0.2.1 (HLC anchor flush on dereg, bounded 64-message sync inbox)

Gate results

  • Baseline: 7587 tests green at Phase 0 entry; test count never reduced across any phase gate.
  • Live probes (rig):
    • Stream lifetime forced to 2 minutes — the alarm stream reconnected in 8.5s, within one reconcile tick of the OK completion, IsLive correctly reflected the gap (WP1.1).
    • CDC conditional-registration: site-b booted clean on LocalDb 0.2.0 with 30 stale triggers dropped at startup (WP1.3/WP3.3) — confirms the self-heal path fires, not just the steady state.
    • S&F due-sweep: EXPLAIN QUERY PLAN confirmed index-terminated (WP1.4).
    • Failover drill (docker/failover-drill.sh) unaffected by the actor/timeout changes.
  • Adversarial review tally: ~25 confirmed findings across the six areas, 4 High, all fixed in the Phase 4 commits above. Zero findings deferred as won't-fix.
  • Test-flake root causes (3, all test-side, not production bugs):
    1. cfa6acbf — an assertion on MarkForwarded ran before the push it depended on was guaranteed to have landed; reordered behind the push.
    2. c4caebe9 — two dispatcher audit-safety tests asserted an attempt count without synchronizing on the async write that produced it; the unsynchronized assertion was removed.
    3. 950c54c5 (embedded) — ScriptDeadlineAtEnqueueTests' "no started event" assertion went vacuous once WP3.2 flipped PerRunScriptEvents to off-by-default (fixed by opting the test back in); ScriptRunLauncherParityTests widened an ExpectMsg window that would have passed for any deadline from 1s to 300s, not just the intended one (sharpened to assert the reported timeout value AND a tight wall-clock range).
    • Follow-up recommended, not done here: a suite-wide sweep for the same AwaitAssert(...)-then-bare-Assert pattern — an AwaitAssert that only proves "eventually true," followed by a plain assertion that silently inherits its timing slack, is the shape behind all three; worth a grep-and-review pass rather than fixing on-demand as flakes surface.

Residuals register

Deliberately not fixed in this program — each has a stated reason, not an oversight:

  1. DCL unsubscribe-during-reconnect count staleness. The 37f13e2e fix discards orphaned in-flight results but a per-connection counter can still drift under rapid subscribe/unsubscribe churn during a reconnect; needs a per-tag counted set. Low severity, cosmetic (a health-report number), deferred. RESOLVED 2026-08-15DataConnectionActor's _totalSubscribed/_resolvedTags scalars are deleted and both health counts are now DERIVED at report time from the authoritative per-tag state (_instancesByTag.Count, the per-tag counted set the residual called for, and _subscriptionIds.Count), so no accumulated counter exists to drift; this also closes the connection-level-failure case that let resolved climb above total. Regression tests: TagResolutionCounts_* in DataConnectionActorBatchTests.
  2. Per-table needs_snapshot in LocalDb. Baselining one table currently re-streams every registered table in both directions. Narrowing it needs an on-disk schema change LocalDb 0.2.1 deliberately avoided (wire/schema compatibility). Documented as a follow-up in the library's own README and RegisterReplicated remarks.
  3. Event batching per proto message. Individual AttributeValueChanged/AlarmStateChanged events still ride one gRPC message each; batching them is a new wire shape (proto + both client/server), deferred rather than folded into this program's additive-only changes.
  4. Deployments page server-side paging + status counts. Still client-materializes the full list; out of scope for this pass (WP2.5 touched the deploy pipeline, not this specific UI surface).
  5. OtOpcUa still pins LocalDb 0.1.3. A supported skew — 0.1.x peers sync with 0.2.x under the library's wire-compatibility guarantee — not a blocker for this program.
  6. Fragile SandboxTests timing pin. Pre-existing, unrelated to this remediation's changes; noted so it isn't mistaken for a regression if it flakes later.
  7. Target-scale load test (deferred-work register #25). This program's exit criterion is the live probes above, not #25 — #25 remains the follow-on validation that the moved ceilings hold under real load; schedule separately.
  8. Playwright 14 pre-existing env failures. Present on main too, rig-state related, not introduced by this branch.
  9. site_events retention purge still oplog-visible. WP3.2's sliced retention DELETE is a row change like any other and is captured by CDC on a replicated site (site-a) — correct per the "CDC does all three jobs" design (no separate resync path to gate), but means a purge burst is visible in the oplog/backlog metrics; not a correctness issue, just a metrics-reading note for operators watching LocalDbOplogBacklog during a purge window.

Docs propagated

Component docs (DataConnectionLayer, SiteRuntime, Communication, AuditLog, SiteEventLogging, StoreAndForward, NotificationOutbox, SiteCallAudit, TemplateEngine, DeploymentManager, CentralUI, ConfigurationDatabase), CLAUDE.md Key Design Decisions (stream-completion reconnect, required site audit DB path, CLI HTTP timeout honesty, bulk DeploySiteAsync, LocalDb 0.2.1), and docs/known-issues/2026-06-26-deploy-config-exceeds-akka-frame-size.md (amended for the MaxBatchSize:64 read-page pin superseding the "retired, left at defaults" note). No components added/removed; README component table unchanged. Cross-reference sweep found no stale live references to ScriptExecutionActor/AlarmExecutionActor or UX_AuditLog_EventId outside historical plan/known-issue records, after correcting four residual mentions in docs/requirements/Component-SiteRuntime.md left over from the WP3.1 doc pass.