Joseph Doherty
491df111ea
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.
141 lines
10 KiB
Markdown
141 lines
10 KiB
Markdown
# 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-15** — `DataConnectionActor`'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.
|