docs(archreview): round-2 re-review (2026-07-12) + 8 fix plans (86 tasks)
Re-ran all 8 domain reviews at HEAD8c888f13against theb910f5ebbaseline: every round-1 finding source-verified (168 fixed, 0 regressions, 0 false claims); 56 new findings (1 Critical / 4 High / 15 Medium / 36 Low), concentrated in post-baseline code (anti-entropy resync, KPI rollup backfill, live alarm stream) and seams the fixes exposed. Headliners: S&F resync predicate inversion can wipe the delivering node's buffer (02-N1 Critical); resync snapshot exceeds the Akka remoting frame size (02-N2); failover drill kills the one node keep-oldest can't survive (01-N1); unbounded rollup backfill per failover (04-R1); live production API key in untracked test.txt (08-NF1). Adds PLAN-R2-01..08 + .tasks.json manifests and the Round-2 board, P0 list, cross-plan mutexes, and wave order in 00-MASTER-TRACKER.
This commit is contained in:
Joseph Doherty
@@ -1,160 +1,135 @@
|
||||
# Architecture Review — Central–Site Communication & Store-and-Forward
|
||||
# Architecture Review — Central–Site Communication & Store-and-Forward (Round 2)
|
||||
|
||||
Reviewer domain: `src/ZB.MOM.WW.ScadaBridge.Communication`, `src/ZB.MOM.WW.ScadaBridge.StoreAndForward`, the telemetry transport paths that ride them, and the corresponding test projects.
|
||||
Date: 2026-07-08.
|
||||
Reviewer domain: `src/ZB.MOM.WW.ScadaBridge.Communication`, `src/ZB.MOM.WW.ScadaBridge.StoreAndForward`, the telemetry transport paths that ride them, the Host wiring that composes them, and — new this round — the **aggregated live alarm stream** shipped 2026-07-10 (`docs/plans/2026-07-10-aggregated-live-alarm-stream-plan.md`): site-wide alarm-only `SubscribeSite` gRPC stream, central `ISiteAlarmLiveCache`/`SiteAlarmLiveCacheService`/`SiteAlarmAggregatorActor`, seed-then-stream, and reconnect telemetry.
|
||||
|
||||
## Scope
|
||||
Date: 2026-07-12 (round 2). Round-1 report dated 2026-07-08 at baseline `b910f5eb`; this round re-reviews everything landed since (PLAN-02's 24 tasks, PLAN-08 options validation, and the post-initiative live-alarm-stream feature) on `main` @ `8c888f13`.
|
||||
|
||||
Read in full: `Component-Communication.md`, `Component-StoreAndForward.md`; all 9 S&F source files (`StoreAndForwardService.cs`, `StoreAndForwardStorage.cs`, `ReplicationService.cs`, `NotificationForwarder.cs`, `ParkedMessageHandlerActor.cs`, options/messages/DI); all Communication source (`CentralCommunicationActor.cs`, `SiteCommunicationActor.cs`, `CommunicationService.cs`, `DebugStreamService.cs`, `DebugStreamBridgeActor.cs`, `StreamRelayActor.cs`, `SiteStreamGrpcServer.cs`, `SiteStreamGrpcClient.cs`, `SiteStreamGrpcClientFactory.cs`, `ReconcileService.cs`, `PendingDeploymentPurgeActor.cs`, `CommunicationOptions.cs`); the transport ends of the telemetry pipelines (`AuditLog/Site/Telemetry/SiteAuditTelemetryActor.cs`, `ClusterClientSiteAuditClient.cs`); and the Host wiring that composes them (`Host/Actors/AkkaHostedService.cs` — `BuildHocon`, `RegisterCentralActors`, `RegisterSiteActorsAsync`), plus `SiteRuntime/Actors/SiteReplicationActor.cs` and the `Notify.Send` enqueue path in `SiteRuntime/Scripts/ScriptRuntimeContext.cs`. Central-side ingest internals (NotificationOutboxActor, AuditLogIngestActor, SiteCallAuditActor) are another reviewer's domain; only their transport contracts are assessed here.
|
||||
## Scope & Method
|
||||
|
||||
## Maturity Verdict
|
||||
- Read the round-1 report and `archreview/plans/PLAN-02-communication-store-and-forward.md` (findings-coverage table), then **verified every round-1 finding against the current source** — no plan claim was trusted without file:line evidence.
|
||||
- Fresh sweep over `git diff b910f5eb..HEAD` scoped to the Communication and StoreAndForward projects and their tests (~4,900 insertions across 50 files, 32 commits), plus the SiteRuntime/Host/CentralUI files the new code touches (`SiteReplicationActor`, `SiteStreamManager`, `AkkaHostedService`, `ScriptRuntimeContext`, `AlarmSummary.razor`).
|
||||
- Static review only (suites verified green 2026-07-10). Known context: the `SiteStreamGrpcServerTests.SiteConnectionUpGauge_*` observable-gauge timing flake pre-exists on main and is not a regression from these plans.
|
||||
|
||||
This is a carefully engineered, unusually well-documented subsystem that is strong *in the small* — idempotency keys threaded end-to-end (NotificationId, EventId, TrackedOperationId), Sender captured before every PipeTo, compare-and-swap storage updates guarding the sweep against operator races, additive-only proto discipline with contract tests, and deliberate, written-down failure semantics on nearly every path. However, it has a small number of serious *in-the-large* lifecycle/topology defects that the (otherwise broad) test suite is structurally blind to, because the risky collaborators are mocked away: the ClusterClient recreate-on-address-change path will almost certainly crash-loop the `CentralCommunicationActor` in production, the standby site node runs the full S&F delivery pipeline with no active-node gate (systematic duplicate-delivery exposure the spec explicitly says should only occur on failover), and one debug session's gRPC failover disposes the shared channel out from under every other session to the same site. The retry sweep's serial, unbounded design also collapses under a realistic central outage backlog. Verdict: **high code quality, mid maturity as a distributed system — 3–4 targeted fixes away from solid.**
|
||||
**One-line verdict:** round 1's Critical and all three Highs are genuinely fixed and well-tested — but the **new** anti-entropy resync was built on the *wrong* active-node predicate (leader instead of the oldest-Up helper the delivery gate uses) and can wipe the delivering node's live buffer after a routine rolling restart, and its snapshot message cannot fit through Akka remoting's default frame size — so round 2 finds one new Critical and one new High, both confined to the peer-resync feature; everything else, including the brand-new live alarm stream, is solid work.
|
||||
|
||||
---
|
||||
|
||||
## Findings — Stability
|
||||
## Round-1 Finding Disposition
|
||||
|
||||
### [Critical] ClusterClient recreate-on-address-change causes actor-name collision and a permanent restart loop
|
||||
`CentralCommunicationActor.HandleSiteAddressCacheLoaded` stops the old client and immediately creates a new one under the same name:
|
||||
| Round-1 finding | R1 severity | Disposition | Evidence (file:line) |
|
||||
|---|---|---|---|
|
||||
| ClusterClient recreate-on-address-change → name collision → restart loop | Critical | **Fixed (verified)** | `CentralCommunicationActor.cs:33-67` — `DefaultSiteClientFactory` mints `site-client-{sanitized}-{generation}` via `Interlocked.Increment` (48); real-factory lifecycle tests exist (`tests/.../DefaultSiteClientFactoryTests.cs`, `CentralCommunicationActorClientLifecycleTests.cs`) |
|
||||
| `siteId` interpolated unvalidated into actor name (sub-finding) | Critical (sub) | **Fixed (verified)** | `CentralCommunicationActor.cs:59-66` — `SanitizeForActorName` maps to valid path elements, falls back to `"site"`; `Create` additionally wrapped in try/catch that logs and skips the site (582-593), stale entry removed before recreate (572-580) |
|
||||
| Standby site node runs full S&F delivery pipeline (systematic duplicates) | High | **Fixed (verified)** | `StoreAndForwardService.cs:89-103` (gate seam), 682-698 (re-evaluated per sweep tick, throwing gate = standby); Host wires it to the shared singleton-host helper `IClusterNodeProvider.SelfIsPrimary` (`AkkaHostedService.cs:868-884`); unit tests cover standby-skip / flip-active / throwing-gate. **But see NEW finding N2** — the *resync* feature added alongside uses a *different* predicate |
|
||||
| Debug-session gRPC failover disposes shared channel; `CleanupGrpc` creates channels during teardown | High | **Fixed (verified)** | `SiteStreamGrpcClientFactory.cs:26,61-73` — cache keyed by `(site, endpoint)`, `GetOrCreate` never disposes, `TryGet` never creates; `RemoveSiteAsync` (95-102) is the only shared-disposal path; `DebugStreamBridgeActor.cs:469-473, 495-498` — both teardown paths use `TryGet(...)?.Unsubscribe(...)` |
|
||||
| Retry sweep serial, unbounded, single-laned — collapses under backlog | High (perf) | **Fixed (verified)** | `StoreAndForwardOptions.cs:31` `SweepBatchLimit` (500) → `StoreAndForwardStorage.cs:392-397` SQL `LIMIT`; per-`(category,target)` lane short-circuit on first transient failure (`StoreAndForwardService.cs:721-730`, `RetryOutcome` 752); parallel lanes capped at `SweepTargetParallelism` 4, sequential within lane (710-734) |
|
||||
| Replication ops applied out of order (`Task.Run` fire-and-forget) | Medium | **Fixed (verified)** | `ReplicationService.cs:142-161` — handler invoked inline; ordering preserved by construction, faults observed via `ContinueWith(OnlyOnFaulted)` |
|
||||
| Replication failures logged at Debug — dead channel invisible | Medium | **Fixed (verified)** | `ReplicationService.cs:153-168` — Warning + `ScadaBridgeTelemetry.RecordReplicationFailure()`; `SiteReplicationActor.cs:225-234` — no-peer drop also Warning + counter |
|
||||
| Park/Requeue replication blind UPDATE — lost on standby missing the row | Medium | **Fixed (verified)** | `ReplicationService.cs:115-140` — Add/Park/Requeue applied via `StoreAndForwardStorage.UpsertMessageAsync` (`ON CONFLICT(id) DO UPDATE`, `StoreAndForwardStorage.cs:319-369`, `created_at` preserved) |
|
||||
| Notifications park after ~25 min; `maxRetries: 0` escape hatch unused | Medium | **Fixed (verified)** | `ScriptRuntimeContext.cs:2209` — `Notify.Send` enqueues with `maxRetries: 0`; park-on-exhaustion is impossible (`StoreAndForwardService.cs:830` guards `MaxRetries > 0`) |
|
||||
| Corrupt buffered notification discarded and reported as delivered | Medium | **Fixed (verified)** | `NotificationForwarder.cs:82-95` — undeserialisable payload returns `false` → engine **parks** the row; Warning log with capped payload preview (170-187) |
|
||||
| Spec-vs-code drift: standby-passive model; `Notify.Send` blocks up to 30 s | Medium | **Fixed (verified)** | `Component-StoreAndForward.md:80-83` — gate + failover-window-only duplicates documented; `:52` — enqueue-only `deferToSweep` semantics documented; code matches (`StoreAndForwardService.cs:588-594` deferToSweep path never invokes the handler inline, kicks `TriggerSweep`) |
|
||||
| No explicit Akka serializer — wire format unpinned | Medium (perf) | **Partially fixed / deferred (accepted)** | Round-trip pin tests added (`tests/.../WireSerializationPinTests.cs` — 191 lines, `ReplicationWireSerializationPinTests.cs`; PLAN-08 T9 additionally found the Newtonsoft ctor-optional-default gotcha and locked contracts). The serializer *swap* remains deferred by design (rolling-compat migration) — reasonable |
|
||||
| Central heartbeat state not replicated to peer central node | Low | **Fixed (verified)** | `CentralCommunicationActor.cs:369-398` — `SiteHeartbeatReplica` fanned over DistributedPubSub, idempotent local mark (194-196, 390-398) |
|
||||
| `SubscribeInstance` concurrency check-then-act (comment only asked) | Low | **Fixed as specified** | `SiteStreamGrpcServer.cs:287-292` — deliberate check-then-act documented with bounded-overshoot rationale |
|
||||
| S&F SQLite: no WAL, no busy_timeout | Low | **Fixed (verified)** | `StoreAndForwardStorage.cs:43-47` (WAL in `InitializeAsync`), 177-185 (5 s busy_timeout per pooled open) |
|
||||
| Due-check per-row `julianday()` parsing | Low | **Fixed (verified)** | Additive `last_attempt_at_ms` column + one-time backfill + `(status, last_attempt_at_ms)` index (`StoreAndForwardStorage.cs:88-114`); due predicate is pure integer math (386-400) |
|
||||
| Audit-observer notification awaited inline in the sweep | Low | **Fixed (verified)** | `StoreAndForwardService.cs:117-135, 898-922` — ordered single-reader channel + pump; inline fallback preserved when not started; drained on `StopAsync` (451-468) |
|
||||
| `StreamRelayActor.WriteToChannel` dead branch + silent DropOldest loss | Low | **Fixed (verified)** | `StreamRelayActor.cs:105-115` — comment corrected (guards closed channel only); real eviction counted by the bounded channel's `itemDropped` callback with first + every-500th Warning (`SiteStreamGrpcServer.cs:303-315`) |
|
||||
| Duplicated Ask-timeout constants between the two ingest transports | Low | **Fixed (verified)** | Single source of truth `SiteStreamGrpcServer.AuditIngestAskTimeout` (`SiteStreamGrpcServer.cs:47`), consumed by `CentralCommunicationActor.cs:140-150, 174` |
|
||||
| Heartbeat `IsActive` leader-check vs oldest-node singleton placement | Low | **Partially fixed — and now materially worse** | PLAN-01 shipped the oldest-Up helper (`ClusterActivityEvaluator.SelfIsOldest`, `Host/Health/ClusterActivityEvaluator.cs:20-31`; `AkkaClusterNodeProvider.SelfIsPrimary:29-37`) and the S&F delivery gate uses it — but `SiteCommunicationActor.DefaultIsActiveCheck` (517-526) and `SiteReplicationActor.DefaultIsActive` (190-198) still use **leader+Up** with "swap point" comments never swapped. The new resync feature was built on the leader predicate → **NEW finding N2 (Critical)** |
|
||||
| `TransportHeartbeatInterval` double duty | Low | **Fixed (verified)** | `CommunicationOptions.cs:59-65` — dedicated `ApplicationHeartbeatInterval` (default 5 s, no behavior change), used at `SiteCommunicationActor.cs:444`; validated (`CommunicationOptionsValidator.cs:54-55`) |
|
||||
| U1: Dual transports for the same ingest operations | Underdev | **Deferred (accepted)** | Both paths still exist (`SiteStreamGrpcServer.cs:388-491` gRPC vs `CentralCommunicationActor.cs:282-325` ClusterClient) with the shared timeout constant removing the tuning drift; consolidation remains a registered follow-up. Acceptable |
|
||||
| U2: No real-collaborator test for ClusterClient cache lifecycle | Underdev | **Fixed (verified)** | `DefaultSiteClientFactoryTests.cs` (real factory, collision + sanitize), `CentralCommunicationActorClientLifecycleTests.cs` (address-edit ×3, throwing factory survives) |
|
||||
| U3: No test/design treatment of standby-node sweep behavior | Underdev | **Fixed (unit level)** | Gate tests in `StoreAndForwardServiceTests.cs` (skip/resume/throwing-gate); doc updated; the live two-node kill rig landed in PLAN-01 (`TwoNodeClusterFixture`) |
|
||||
| U4: Replication has no anti-entropy/resync | Underdev | **Fixed — with two new defects** | `SiteReplicationActor.cs:111-113, 175-181, 398-444` + `StoreAndForwardStorage.GetAllMessagesAsync`/`ReplaceAllAsync` (256-306). The mechanism exists and is authority-checked and capped — but see **N1 (frame size)** and **N2 (predicate inversion)** below |
|
||||
| U5: Parked rows unbounded retention, no aging signal | Underdev | **Fixed (verified)** | `StoreAndForwardStorage.GetOldestParkedCreatedAtAsync` (675-685) → `HealthReportSender.cs:114-115` → additive `SiteHealthReport.OldestParkedMessageAgeSeconds` (`SiteHealthReport.cs:57`) |
|
||||
| U6: Reconciliation cursor edge (`>` vs `>=`) unpinned | Underdev | **Fixed (verified, exceeded)** | `PullSiteCalls` grew an additive composite-keyset cursor (`after_id`, `SiteStreamGrpcServer.cs:598-641`); pinned by `SiteStreamPullSiteCallsTests.cs` |
|
||||
| U7: `DebugStreamService._sessions` lost silently on central restart | Underdev | **Documented (accepted)** | `Component-Communication.md:67` — explicit "Accepted limitation" paragraph citing this review |
|
||||
|
||||
- `CentralCommunicationActor.cs:514-521` — `Context.Stop(_siteClients[siteId].Client)` followed synchronously by `_siteClientFactory.Create(...)`.
|
||||
- `CentralCommunicationActor.cs:36-40` (`DefaultSiteClientFactory`) — `system.ActorOf(ClusterClient.Props(settings), $"site-client-{siteId}")`: a **named top-level actor**.
|
||||
|
||||
`Context.Stop` is asynchronous; the name `site-client-{siteId}` stays reserved until the old actor fully terminates. Creating the replacement in the same message handling will throw `InvalidActorNameException` essentially every time. The `Create` call is **outside** the surrounding try/catch (which only guards `ActorPath.Parse`, `CentralCommunicationActor.cs:492-505`), so the exception escapes the handler → the user-guardian default directive **restarts** the actor → the new incarnation's `_siteClients` dictionary is empty, but all old `site-client-*` actors are still alive at the system level. Every subsequent 60-second refresh (`PreStart`, `CentralCommunicationActor.cs:564-569`) then tries to create clients for **all** sites, collides on the reserved names, and crashes again. Failure scenario: an operator edits one site's NodeA address in the Central UI → central loses ClusterClient routing to *every* site (all `SiteEnvelope`s dropped at `HandleSiteEnvelope`, `CentralCommunicationActor.cs:398-413`) until the central process is restarted. This is the exact path the design doc promises works: "ClusterClient instances are recreated when contact points change" (`Component-Communication.md:238`).
|
||||
|
||||
Secondary issue on the same lines: `siteId` is interpolated into an actor name unvalidated. A `SiteIdentifier` containing `/`, whitespace, or other non-path characters throws `InvalidActorNameException` on first creation with the same restart-loop consequence — contrast with `SiteStreamGrpcServer.SubscribeInstance`, which validates `correlation_id` with `ActorPath.IsValidPathElement` for precisely this reason (`SiteStreamGrpcServer.cs:229-234`).
|
||||
|
||||
Why tests miss it: `CentralCommunicationActorTests` substitutes `ISiteClientFactory` with an NSubstitute mock returning TestProbes (`tests/.../CentralCommunicationActorTests.cs:42-51`) — the real naming/lifecycle machinery is never exercised anywhere.
|
||||
|
||||
Fix shape: unique names (`site-client-{siteId}-{seq}` via a counter, like `SiteStreamGrpcServer._actorCounter`), or watch `Terminated` and defer recreation, plus sanitize/validate the site id, plus one test using `DefaultSiteClientFactory`.
|
||||
|
||||
### [High] Standby site node runs the full S&F delivery pipeline — duplicate delivery is systematic, not a failover residue
|
||||
The design says the standby only *applies* replicated operations and "on failover, the new active node resumes delivery" (`Component-StoreAndForward.md:77-80`). The code starts delivery on **both** nodes:
|
||||
|
||||
- `AkkaHostedService.cs:981` — `await storeAndForwardService.StartAsync()` runs in `RegisterSiteActorsAsync` on **every** site node, starting the retry timer (`StoreAndForwardService.cs:337-341`).
|
||||
- `AkkaHostedService.cs:989, 998, 1033` — all three delivery handlers (ExternalSystem, CachedDbWrite, Notification) are registered unconditionally on every node.
|
||||
- `SiteReplicationActor.cs:280-291` — replicated `Add` operations are applied to the standby's SQLite, so the standby's sweep has real Pending rows to deliver.
|
||||
|
||||
There is no active-node/leader gate anywhere in `StoreAndForwardService.RetryPendingMessagesAsync` (`StoreAndForwardService.cs:563-589`) or in the delivery handlers (`ExternalSystemClient.DeliverBufferedAsync`, `ExternalSystemGateway/ExternalSystemClient.cs:173-224` — checked, no gate). Consequences:
|
||||
|
||||
1. **Duplicate deliveries in steady operation.** Both nodes' sweeps become due for the same message at the same wall-clock moment (the replicated row carries the same `last_attempt_at`). Whichever delivers first replicates a `Remove`, but if both attempts are in flight concurrently — a window the size of the delivery duration (an HTTP call, often ≥ hundreds of ms) plus replication latency — the target receives the call **twice**. Over a long outage with many buffered messages, duplicates recur every recovery. Notifications survive this (central dedups on `NotificationId` insert-if-not-exists), but `ExternalSystem` calls and `CachedDbWrite`s do not — "idempotency is the caller's responsibility" was scoped to *rare failover* duplicates, not steady-state ones.
|
||||
2. **Doubled retry traffic** against an already-struggling target during every outage.
|
||||
3. Both nodes emit `CachedCallTelemetry` attempts for the same operation with independent retry counts, muddying the central audit picture.
|
||||
|
||||
Fix shape: gate the sweep (and ideally the queue-depth gauge registration) behind the same leader-check used by `SiteCommunicationActor.DefaultIsActiveCheck` (`SiteCommunicationActor.cs:498-507`), re-evaluated per sweep tick so failover resumes delivery automatically.
|
||||
|
||||
### [High] One debug session's gRPC failover disposes the shared channel under every other session to the same site
|
||||
`SiteStreamGrpcClientFactory` caches **one** `SiteStreamGrpcClient` per site, keyed by site id, replacing (and disposing) it whenever a caller asks for a different endpoint (`SiteStreamGrpcClientFactory.cs:52-78`). `DebugStreamBridgeActor` holds **per-session** node preference and flips endpoint on every stream error (`DebugStreamBridgeActor.cs:416, 468-483`). With two concurrent debug sessions to the same site:
|
||||
|
||||
- Session A (on NodeA) hits an error and flips to NodeB → `GetOrCreate(site, NodeB)` **disposes** the NodeA-bound client. `SiteStreamGrpcClient.ReleaseResources` cancels **every** registered subscription CTS and disposes the channel (`SiteStreamGrpcClient.cs:314-324`) — killing session B's healthy stream.
|
||||
- Session B's `SubscribeAsync` observes the cancellation as an error → B flips to NodeA → disposes the NodeB client → kills A's brand-new stream. The two sessions ping-pong, burning both sessions' 3-retry budgets (`DebugStreamBridgeActor.cs:45`) and terminating both.
|
||||
|
||||
Also, `CleanupGrpc` (`DebugStreamBridgeActor.cs:486-495`) calls `GetOrCreate` during teardown — if the cached client is bound to the other endpoint, cleanup *creates a fresh channel* (and disposes the cached one) merely to call `Unsubscribe`, with the same collateral effect.
|
||||
|
||||
Fix shape: key the cache by `(siteId, endpoint)` (both node channels can coexist), or reference-count clients, or give each bridge actor its own client instance. The single-flip failover for a *site-wide* address change can stay in the factory; per-session failover must not mutate shared state.
|
||||
|
||||
### [Medium] Replication operations can be applied out of order
|
||||
`ReplicationService.FireAndForget` wraps each operation in its own `Task.Run` (`ReplicationService.cs:134-149`). Two operations for the same message issued in quick succession (Add then Remove; Park then operator Requeue) are handed to the thread pool as independent tasks — the pool does not preserve ordering, so the `replicationActor.Tell(...)` calls (handler wired at `AkkaHostedService.cs:897-901`) can happen inverted. Akka's per-sender/receiver ordering guarantee only applies *after* the Tells, so the standby can apply `Remove` (no-op, row absent) then `Add` → an orphan Pending row that the ungated standby sweep (finding above) or a failover will re-deliver. The `Task.Run` buys nothing — the handler is a non-blocking Tell returning `Task.CompletedTask`; invoking it inline (with a try/catch) preserves ordering by construction.
|
||||
|
||||
### [Medium] Replication failures are logged at Debug — a dead replication channel is invisible
|
||||
`ReplicationService.cs:144-147` logs a failed replication at `LogDebug`. If the handler starts failing systematically (peer unreachable in a way `SendToPeer` doesn't detect, serialization fault of `StoreAndForwardMessage`), the standby's buffer silently diverges and the next failover loses the entire un-replicated delta — with zero operator-visible signal at default log levels and no counter metric. `SiteReplicationActor.SendToPeer`'s no-peer drop is also Debug (`SiteReplicationActor.cs:139-149`). At minimum Warning + a telemetry counter; the repo already has `ScadaBridgeTelemetry` plumbing for exactly this kind of gauge.
|
||||
|
||||
### [Medium] Park/Requeue replication is a blind UPDATE — lost on a standby missing the row
|
||||
`ApplyReplicatedOperationAsync` applies `Park` and `Requeue` via `UpdateMessageAsync` (`ReplicationService.cs:121-130`), which is `UPDATE ... WHERE id = @id` (`StoreAndForwardStorage.cs:210-232`) — zero rows affected is silent. If the original `Add` was lost (fire-and-forget), the park state never materializes on the standby: after failover the message is simply *gone* from the parked view (operator can't retry/discard it) even though the full message payload was in hand in the replication operation. An upsert (INSERT OR REPLACE) for Park/Requeue would self-heal the lost-Add case for free.
|
||||
|
||||
### [Medium] Notifications park after ~25 minutes of central unreachability, and the "no limit" escape hatch is unused
|
||||
`Notify.Send` enqueues with no `maxRetries` (`ScriptRuntimeContext.cs:2197-2203`) → `DefaultMaxRetries = 50` (`StoreAndForwardOptions.cs:21`) at the 30 s default interval = ~25 minutes of central outage before a notification parks and requires per-message operator action. `Component-StoreAndForward.md:52` explicitly documents `maxRetries: 0` as the escape hatch for callers that "genuinely require unbounded retry" — but the *only* notification producer doesn't use it, and no host config raises the cap for the Notification category specifically. A central maintenance window longer than 25 minutes strands every notification raised during it. Compounding: the serial sweep (Performance #1) makes effective intervals much longer than 30 s under backlog, but retries are counted per sweep-attempt, so the wall-clock park horizon is at least honest — the operational cliff is the manual un-parking.
|
||||
|
||||
### [Medium] Corrupt buffered notification is discarded and reported as delivered
|
||||
`NotificationForwarder.DeliverAsync` returns `true` (→ engine removes the row) when the payload fails to deserialize (`NotificationForwarder.cs:79-87`). The message is permanently lost with only a site-log Warning: no parked row, no central `Notifications` row, no audit row. Returning `false` (permanent failure → **park**) would preserve the payload for forensics at zero protocol cost — parking is exactly the designed home for undeliverable-but-preserved messages.
|
||||
|
||||
### [Low] Central heartbeat state is not replicated to the peer central node
|
||||
`HandleSiteHealthReport` fans reports to the peer via DistributedPubSub (`CentralCommunicationActor.cs:360-373`), but `HandleHeartbeat` marks only the local aggregator (`CentralCommunicationActor.cs:349-353`). ClusterClient delivers each site's heartbeat to one central node; the other's aggregator sees none. After a central failover the new active node briefly shows all sites offline until fresh heartbeats arrive (≤ heartbeat interval + offline threshold). Minor, but inconsistent with the care taken for health reports.
|
||||
|
||||
### [Low] `SubscribeInstance` concurrency-limit check is check-then-act
|
||||
`SiteStreamGrpcServer.cs:243-254`: `_activeStreams.Count >= _maxConcurrentStreams` then insert — two concurrent subscribes at the boundary both pass. Bounded overshoot only; not worth locking, worth a comment.
|
||||
**Disposition counts:** 22 Fixed (verified) · 2 Partially fixed (serializer pin-only by design; leader-vs-oldest helper shipped but not swapped into all call sites) · 2 Deferred/accepted (U1 dual transports, U7 session locality) · 0 Not fixed · 0 Regressed. Round-1 Critical **C2/C1 (ClusterClient recreate) specially verified: genuinely fixed**, with real-collaborator tests exactly where round 1 said the blind spot was.
|
||||
|
||||
---
|
||||
|
||||
## Findings — Performance
|
||||
## NEW Findings — Store-and-Forward & Replication
|
||||
|
||||
### [High] The retry sweep is serial, unbounded, and single-laned across categories — it collapses under backlog
|
||||
Three compounding design choices:
|
||||
### [Critical] N1 — Anti-entropy resync authority uses the *leader* predicate while delivery uses *oldest-Up*: after a routine rolling restart the stale rejoining node wipes the delivering node's live buffer
|
||||
|
||||
- `GetMessagesForRetryAsync` has **no LIMIT** — it loads every due row into memory each sweep (`StoreAndForwardStorage.cs:183-203`).
|
||||
- `RetryPendingMessagesAsync` delivers strictly one-at-a-time (`StoreAndForwardService.cs:571-579`) while `_retryInProgress` blocks the next sweep (`StoreAndForwardService.cs:566`).
|
||||
- Every notification forward attempt against a down central costs a full 30 s Ask timeout (`NotificationForwardTimeout`, `CommunicationOptions.cs:36`; `NotificationForwarder.cs:93-95`).
|
||||
The delivery gate installed by PLAN-02 T4 correctly uses PLAN-01's singleton-host helper: `AkkaHostedService.cs:881` → `AkkaClusterNodeProvider.SelfIsPrimary` → `ClusterActivityEvaluator.SelfIsOldest` (`ClusterActivityEvaluator.cs:20-31`, up-number/`IsOlderThan` based — matches where cluster singletons actually run). But the **peer-join resync roles** added in the same plan use a different predicate: `SiteReplicationActor.DefaultIsActive` (`SiteReplicationActor.cs:190-198`) is **leader + Up**, with a comment calling itself the "swap point for plan 01's shared helper" that was never swapped.
|
||||
|
||||
Failure scenario: central is down for an hour and scripts have buffered 200 notifications. One sweep = 200 × 30 s ≈ 100 minutes of serial timeouts — during which no `ExternalSystem` or `CachedDbWrite` retries happen either (single lane, head-of-line blocking across categories and targets: one slow external system delays notification forwarding to a *healthy* central and vice versa). There is no per-target short-circuit — N messages to the same dead target incur N consecutive timeouts per sweep. The queue drains far slower than it fills; retry counts tick toward parking on a wildly distorted cadence.
|
||||
Akka.NET's cluster leader is the **lowest-address** reachable Up member — not the oldest. The two predicates diverge *persistently* (not just transiently) whenever the lower-address node is not the oldest, which is exactly the state produced by the most routine operation there is: a rolling restart (or crash + rejoin) of the lower-address site node. Sequence, with `node-a` (lower address) restarted while `node-b` keeps running:
|
||||
|
||||
Fix shape: `LIMIT` on the due query; group by (category, target) and short-circuit remaining messages for a target after its first transient failure in a sweep; optionally parallelize across targets with a small cap. Any one of the three helps materially.
|
||||
1. `node-b` is now oldest → `SelfIsPrimary` true → node-b runs the delivery sweep and owns the live buffer. `node-a` rejoins with a fresh (higher) up-number but becomes **leader** by address ordering.
|
||||
2. `node-b` observes `MemberUp(node-a)` → `TryTrackPeer` → `OnPeerTracked` (`SiteReplicationActor.cs:175-181`): `SafeIsActive()` is **false** on node-b (it is not leader) → the *delivering* node sends `RequestSfBufferResync` to the stale peer.
|
||||
3. `node-a` handles it (`HandleRequestSfBufferResync`, 398-411): `SafeIsActive()` is **true** on node-a (leader+Up) → it answers with a snapshot of its **stale, pre-outage buffer**.
|
||||
4. `node-b` handles the snapshot (`HandleSfBufferSnapshot`, 420-444): `SafeIsActive()` false → it calls `ReplaceAllAsync` — the method whose own doc says "**Never call on an active node** — it discards every in-flight row" (`StoreAndForwardStorage.cs:281-283`) — **on the node that is actively delivering**.
|
||||
|
||||
### [Medium] No explicit Akka serializer configuration — every cross-cluster contract rides default reflective JSON
|
||||
`BuildHocon` (`AkkaHostedService.cs:214-271`) configures remoting, SBR, and failure detectors, but **no** `akka.actor.serializers` / `serialization-bindings`. All command/control and telemetry traffic — deployment configs, `IngestAuditEventsCommand` batches (entities converted *from* compact proto DTOs back to POCOs before the Akka hop, `ClusterClientSiteAuditClient.cs:72-85`), `ReplicationOperation` with full message payloads — is serialized by Akka.NET's default Newtonsoft JSON serializer with embedded CLR type manifests. Costs: (a) hot-path CPU/allocation and payload bloat on the highest-volume channel (audit telemetry); (b) the documented "additive-only message contract evolution" rule (`CLAUDE.md`, Commons conventions) is only half the story — wire compatibility is also coupled to *type and namespace identity*, so a rename/move of a Commons message type breaks rolling cross-version central↔site communication with no test to catch it. `MessageContractTests` asserts correlation-id presence only (`tests/.../MessageContractTests.cs`), not serialized round-trip stability. Ironic detail: the telemetry path builds efficient proto DTOs, then converts them back to POCOs specifically to cross the ClusterClient hop.
|
||||
Consequences: every message buffered on node-b during and since node-a's outage is **deleted** (notifications enqueued with `maxRetries: 0` are silently gone — the one category the design promises is never lost short of payload corruption), and rows node-b already delivered are **resurrected** from node-a's stale copy → guaranteed duplicate deliveries on subsequent sweeps. `ReplicationEnabled` defaults to true (`StoreAndForwardOptions.cs:12`), so production 2-node sites are exposed by default.
|
||||
|
||||
### [Low] S&F SQLite: no WAL, no explicit busy_timeout, connection-per-operation
|
||||
Every `StoreAndForwardStorage` method opens a fresh `SqliteConnection` (e.g. `StoreAndForwardStorage.cs:137-139, 185-186`) with a bare `Data Source=` connection string (`StoreAndForward/ServiceCollectionExtensions.cs:22-24`) — default rollback-journal mode. Concurrent writers exist by design: script-thread enqueues, the sweep, replication applies (standby), and gRPC `PullSiteCalls`/parked-query reads. WAL + `busy_timeout` pragma would remove writer/reader blocking; Microsoft.Data.Sqlite's pooling makes connection-per-op tolerable but the pragmas are one line each in `InitializeAsync`.
|
||||
Fix shape: swap both remaining leader-based predicates (`SiteReplicationActor.cs:190-198`, `SiteCommunicationActor.cs:517-526`) to the shared oldest-Up helper so *one* definition of "active" governs delivery, resync-request, resync-answer, and heartbeat IsActive; add a two-node rig test that rolls the lower-address node and asserts the surviving node's buffer is untouched. (Also consider a belt-and-braces guard: refuse `ReplaceAllAsync` while this node's *delivery gate* reports active.)
|
||||
|
||||
### [Low] Due-check does per-row `julianday()` string parsing on ISO-8601 timestamps
|
||||
`StoreAndForwardStorage.cs:195-197` computes due-ness with `(julianday('now') - julianday(last_attempt_at)) * 86400000` against `"O"`-format strings with 7-digit fractional seconds and `+00:00` offsets — relying on SQLite's lenient datetime parser, non-indexable, and re-parsed for every Pending row on every 10 s sweep. Storing epoch milliseconds would be cheaper, indexable, and immune to format drift.
|
||||
### [High] N2 — The resync snapshot rides ONE Akka remoting message with no frame-size headroom: it is silently undeliverable for any realistic backlog
|
||||
|
||||
### [Low] Audit-observer notification is awaited inline in the sweep
|
||||
`NotifyCachedCallObserverAsync` is awaited per attempt inside `RetryMessageAsync` (`StoreAndForwardService.cs:620-626, 688-694, 711-717`). It's exception-isolated (good), but not latency-isolated: a slow observer (SQLite audit write) stretches the already-serial sweep. Fire-and-forget with isolation, or post to a channel, would decouple it.
|
||||
`HandleRequestSfBufferResync` pipes up to **10,000 full rows** (id + target + complete `payload_json` + provenance) into a single `SfBufferSnapshot` record and Tells it across Akka remoting (`SiteReplicationActor.cs:40, 407-410, 460`). `BuildHocon` sets no `akka.remote.dot-netty.tcp.maximum-frame-size` (grep across `src/` and `docker/` finds none), so the default **128,000 bytes** applies. That is ~12 bytes per row at the cap — even a few hundred typical cached-call payloads exceed it. An oversized remote message is dropped by the transport (an error in the sender's log; no exception surfaces to the `PipeTo`), the standby never receives the snapshot, nothing retries until the *next* peer-track event, and no telemetry counts it. Net effect: **the anti-entropy fix works only for near-empty buffers and silently fails in exactly the sustained-outage/backlog scenario it was built for** (U4's "standby down for an hour"). Ironically this also caps N1's blast radius — large stale snapshots can't be delivered either — but small-buffer wipes still go through.
|
||||
|
||||
Fix shape: chunk the snapshot (reuse the pull-style paging the audit reconciliation already has), or stream rows as a sequence of upsert ops bracketed by begin/end markers with a completion-driven delete-absent pass; count send failures. Also reconsider the 10k in-memory `List` on the active node.
|
||||
|
||||
### [Medium] N3 — `_sweepTask` handoff is clobbered by every timer tick, defeating the shutdown drain it exists for
|
||||
|
||||
Both the timer callback (`StoreAndForwardService.cs:396-400`) and `TriggerSweep` (664-668) unconditionally `Volatile.Write(ref _sweepTask, RetryPendingMessagesAsync())`. When a sweep is still in flight, the new call no-ops via the `_retryInProgress` CAS and returns a **completed** task — which overwrites `_sweepTask`. `StopAsync` (422-431) then awaits the no-op and proceeds with shutdown while the real sweep (one that has outlived a 10 s tick — i.e. precisely the backlog case the drain was added for) is still mid-delivery. Consequence is bounded (the sweep's own catch logs; storage is connection-per-op), but the "wait for the in-flight sweep" guarantee is illusory whenever it matters. Fix: only publish the task when the CAS is won (e.g. have `RetryPendingMessagesAsync` register itself), or CAS `_sweepTask` from completed→new.
|
||||
|
||||
### [Low] N4 — New sweep options missing from the eager validator
|
||||
|
||||
`StoreAndForwardOptionsValidator` (17-34) validates path/intervals/max-retries but not the two options PLAN-02 added: a negative `SweepBatchLimit` silently means "unlimited (legacy)" (`StoreAndForwardStorage.cs:392` checks `> 0`) and a non-positive `SweepTargetParallelism` is silently clamped to 1 (`StoreAndForwardService.cs:715`). Both deserve fail-fast messages per the §1.5 eager-validation convention the same release adopted elsewhere.
|
||||
|
||||
### [Low] N5 — Resync snapshot vs in-flight replication op race leaves orphan rows on the standby
|
||||
|
||||
A replicated `Remove` sent after the active node read its snapshot but before the snapshot send is ordered *before* the snapshot on the wire (same sender/receiver pair), so the stale snapshot re-adds the removed row on the standby → an orphan Pending row that a later failover re-delivers. Bounded and self-correcting at the next resync, and inherent to no-ack replication — worth a code comment on `HandleSfBufferSnapshot` (`SiteReplicationActor.cs:420-444`) so nobody "fixes" it into something worse.
|
||||
|
||||
---
|
||||
|
||||
## Findings — Conventions
|
||||
## NEW Findings — Aggregated Live Alarm Stream (post-initiative, first review)
|
||||
|
||||
### [Medium] Spec-vs-code drift in the S&F component doc
|
||||
Two material divergences from `Component-StoreAndForward.md`:
|
||||
1. **Standby-passive model** (doc lines 77-80) vs. the ungated standby delivery pipeline (Stability #2). The doc's "a few duplicate deliveries [on failover]... acceptable trade-off" framing materially understates actual behavior.
|
||||
2. **`Notify.Send` latency**: the doc/CLAUDE.md say Send "returns a NotificationId status handle immediately", but `EnqueueAsync`'s immediate-delivery attempt (`StoreAndForwardService.cs:500-529`) runs the `NotificationForwarder` Ask inline on the script's await — when central is down, every `Notify.Send` blocks its script execution for up to `NotificationForwardTimeout` (30 s) before buffering. Either document the worst case or make the Notification category enqueue with `attemptImmediateDelivery: false` + an immediate sweep kick.
|
||||
The feature is well-shaped overall: one gRPC stream per site (not per circuit), reference-counted lifecycle with linger, stream-first seed with ordered delta buffering and per-key dedup (correctly copied from `DebugStreamBridgeActor`, including the inclusive-boundary rule), authority-split with the poll fallback (`IsLive` false until first seed publishes — `SiteAlarmLiveCacheService.cs:131-136, 306-323`), reconcile as the drop-detection backstop (`SiteAlarmAggregatorActor.cs:276-304` rebuilds authoritatively so disappeared rows are dropped), self-healing on both start failure (bounded retry timer, `SiteAlarmLiveCacheService.cs:249-275`) and stream give-up (reconcile tick reopens, `SiteAlarmAggregatorActor.cs:224-236`), balanced telemetry gauges in PreStart/PostStop (188, 212), viewer-cap fail-safe returning a no-op handle instead of throwing on a Blazor render path (100-107), and eager options validation (`CommunicationOptionsValidator.cs:69-87`). Findings:
|
||||
|
||||
### [Low] `StreamRelayActor.WriteToChannel` has a dead branch and silent loss
|
||||
`TryWrite` on a bounded channel in `DropOldest` mode never returns `false` — it evicts the oldest item and succeeds. The "Channel full, dropping event" warning (`StreamRelayActor.cs:105-111`) can never fire, while the *actual* loss (the evicted oldest event) is completely unlogged. Lossy-under-backpressure is fine for debug view per spec (`Component-Communication.md:59`), but the observability is inverted: dead warning, silent real drop. Count evictions (channel `ItemDropped` callback via `CreateBounded(options, itemDropped)`) or log periodically.
|
||||
### [Medium] N6 — No delta coalescing: every applied alarm delta copies the whole site cache and fans out to every viewer circuit
|
||||
|
||||
### [Low] Duplicated Ask-timeout constants and divergent fault semantics between the two ingest transports
|
||||
`CentralCommunicationActor.DefaultAuditIngestAskTimeout` copies `SiteStreamGrpcServer.AuditIngestAskTimeout` by hand (`CentralCommunicationActor.cs:114-124` vs `SiteStreamGrpcServer.cs:45`), and the two handlers deliberately differ on fault behavior (propagate `Status.Failure` vs swallow-to-empty-ack — documented at `CentralCommunicationActor.cs:104-111`). Both are acknowledged in comments, but two transports for the same operation with different failure semantics and copy-pasted tuning is drift waiting to happen (see Underdeveloped, dual transports).
|
||||
`HandleLiveDelta` → `Publish()` materialises a fresh `_cache.Values.ToList()` **per delta** (`SiteAlarmAggregatorActor.cs:365-367, 391-403`), and `OnPublish` invokes every subscriber's `onChanged` (`SiteAlarmLiveCacheService.cs:386-411`), each of which does a Blazor `InvokeAsync` + `GetCurrentAlarms` + re-render (`AlarmSummary.razor:369-388`). An alarm flood — the canonical SCADA storm scenario, hundreds of condition transitions per second on a large site — becomes O(cacheSize) list copies per transition on the actor thread plus a render queue entry per circuit per transition. The seed path already batches; the live path should too (e.g. a ~250 ms publish-coalescing timer, or publish-on-tick-if-dirty). Everything stays correct (last write wins), so this is performance, not correctness.
|
||||
|
||||
### [Low] Heartbeat `IsActive` uses leader-check while singletons follow oldest-node
|
||||
`SiteCommunicationActor.DefaultIsActiveCheck` reports active = cluster leader (`SiteCommunicationActor.cs:498-507`). Akka cluster singletons run on the *oldest* qualifying node; leader and oldest usually coincide in a 2-node stable cluster but can diverge transiently. The comment says it deliberately mirrors `ActiveNodeGate`, so it's a consistent repo convention — but the convention itself can misreport which node hosts the DeploymentManager singleton. Worth one shared helper with an oldest-member check.
|
||||
### [Low] N7 — Aggregator lifecycle nits (bundled)
|
||||
|
||||
### [Low] `TransportHeartbeatInterval` does double duty
|
||||
The Akka.Remote transport failure-detector interval (`BuildHocon`, `AkkaHostedService.cs:246-249`) is also the *application-level* site→central heartbeat cadence (`SiteCommunicationActor.cs:421-425`). These are unrelated concerns (spec treats them separately: `Component-Communication.md:268-274` vs health reporting); tuning one silently retunes the other.
|
||||
1. **Reconnect re-seed can be skipped:** the post-failover-flip `StartFanout(isInitial: false)` (`SiteAlarmAggregatorActor.cs:472`) no-ops if a reconcile fan-out is already in flight (247-253) — deltas missed between stream death and the in-flight snapshot's read-time stay stale until the *next* reconcile (up to 60 s), despite the "never silently serve stale" comment.
|
||||
2. **No stream-generation stamp on `GrpcAlarmStreamError`:** a late error raced out of the *previous* cancelled stream (the `RpcException(Cancelled)` filter at `SiteStreamGrpcClient.cs:256-258` covers the normal path, but a genuine socket fault can beat the cancel) is indistinguishable from a failure of the current stream and burns retry budget / double-flips. The window is small; a monotonic stream-id in the error message would close it.
|
||||
3. **Static mutable test seams:** `ReconnectDelay` / `StabilityWindow` are `internal static` mutable properties (`SiteAlarmAggregatorActor.cs:61-69`) — process-global state that parallel test classes can trample; instance/ctor parameters would match how the rest of the file injects tuning.
|
||||
|
||||
### Positives worth recording
|
||||
- Tell/Ask discipline matches the repo rule everywhere: Ask only at system boundaries (`CommunicationService`, forwarder, telemetry clients), Tell/Forward on hot paths with Sender preservation for reply routing (`SiteCommunicationActor.cs:270-388`, `CentralCommunicationActor.cs:398-413`).
|
||||
- Correlation IDs on every request/response contract, verified by tests.
|
||||
- The at-least-once notification handoff is textbook: ack-after-persist at central, script-generated `NotificationId` as buffered-row id *and* idempotency key, re-stamped provenance at the forwarder (`NotificationForwarder.cs:123-160`).
|
||||
- The sweep's CAS updates (`UpdateMessageIfStatusAsync`, `StoreAndForwardStorage.cs:243-269`) correctly close the operator-retry/discard vs sweep race, replicated with correct captured state (`StoreAndForwardService.cs:861-892`).
|
||||
- gRPC server stream lifecycle is thorough: readiness + shutdown gating, correlation-id validation, duplicate-stream replacement, session-lifetime cap, leak-proofed Subscribe failure cleanup, own-entry-only removal (`SiteStreamGrpcServer.cs:211-327`).
|
||||
- Stream-first debug subscription with buffered gap-window replay and per-entity timestamp dedup (`DebugStreamBridgeActor.cs:134-176, 286-329`) is a correct solution to the snapshot/stream ordering problem, including the flapping-stream retry-budget stability window.
|
||||
- Additive proto evolution is real and tested (`ProtoContractTests`, `ProtoRoundtripTests`), with the manual regen process documented.
|
||||
### [Low] N8 — "Lives only on the active central node" is aspirational, and deleted sites leak idle channels
|
||||
|
||||
`SetActorSystem` is wired on every central node (`AkkaHostedService.cs:430`), so browsing the standby node directly (port 9002, documented for diagnostics) starts a second, fully functional aggregator + gRPC stream there — harmless (read-only) but contradicting the `[PERM]` doc claim in `ISiteAlarmLiveCache.cs:13-17`. Separately, `SiteStreamGrpcClientFactory.RemoveSiteAsync` — designed as "the disposal path when a site record is deleted" (`SiteStreamGrpcClientFactory.cs:88-102`) — has **no production caller** (grep: tests only), so a deleted site's channels (and an aggregator kept alive by an open viewer, which then reconciles to an empty snapshot forever) persist until process restart. Bounded, but wire the deletion path or document the gap.
|
||||
|
||||
### [Low] N9 — A site with only one configured gRPC endpoint can never go live
|
||||
|
||||
`ResolveSiteAsync` requires **both** `GrpcNodeAAddress` and `GrpcNodeBAddress` (`SiteAlarmLiveCacheService.cs:294-300`); a single-node site (or one with only NodeA populated) silently never starts an aggregator and polls forever, with only a log Warning. The aggregator itself would work fine flipping between one address; accept a single endpoint and disable the flip.
|
||||
|
||||
---
|
||||
|
||||
## Underdeveloped Areas
|
||||
## What's genuinely good (round 2)
|
||||
|
||||
1. **Dual transports for the same ingest operations.** `IngestAuditEvents`/`IngestCachedTelemetry` exist both as gRPC unary RPCs (`SiteStreamGrpcServer.cs:330-433`) and as ClusterClient commands (`CentralCommunicationActor.cs:262-305`); production push uses ClusterClient (`ClusterClientSiteAuditClient`), leaving the gRPC ingest surface production-idle (doc admits: "the gRPC-receiving counterpart", `Component-Communication.md:87`). Two code paths, two fault semantics, double the test/maintenance surface — consolidation is pending and should be scheduled.
|
||||
2. **No real-collaborator test for the ClusterClient cache lifecycle.** `DefaultSiteClientFactory` is untested; address-change recreation is only tested through mocks — precisely where the Critical finding lives.
|
||||
3. **No test or design treatment of standby-node sweep behavior.** Neither the S&F tests nor an integration test asserts that only the active node delivers; the docker-cluster smoke topology would expose it with a two-node site and a delayed target.
|
||||
4. **Replication has no anti-entropy/resync.** A standby down for an hour rejoins and applies only *new* operations; the missed delta diverges forever (until rows drain naturally). The doc covers only the "last few operations" loss case. A periodic checksum or full-row reconciliation (the pattern already exists for audit rows via `PullAuditEvents`) is absent for the S&F buffer itself.
|
||||
5. **Parked rows have unbounded retention.** By design parked messages persist until operator action (`Component-StoreAndForward.md:110-116`), but there is no aging alert, cap, or archival — a forgotten site accumulates parked rows indefinitely with only a health-metric count.
|
||||
6. **Reconciliation cursor edge.** `PullAuditEvents`/`PullSiteCalls` batch continuation relies on central advancing a `since_utc` cursor from the last returned timestamp (`SiteStreamGrpcServer.cs:478-484, 563-572`); rows sharing a boundary timestamp are protected only by EventId dedup / upsert-on-newer at central — fine, but the site-side read contract (`>` vs `>=`) isn't pinned by a test in this project.
|
||||
7. **`DebugStreamService._sessions` on central failover** — sessions are process-local with no persistence (documented as acceptable: engineer re-establishes), but the SignalR consumer contract (`OnStreamTerminated`) is the only notification; a central restart drops sessions with no signal at all. Known/accepted, listed for completeness.
|
||||
- **The fix quality is high and test-backed.** Every round-1 fix landed with the failing-test-first shape the plan prescribed; the two places round 1 called out as structurally untested (real `DefaultSiteClientFactory`, standby sweep behavior) now have real-collaborator tests. The wire-format pin tests even caught a real Newtonsoft deserialization gotcha (ctor optional-param defaults) during PLAN-08.
|
||||
- **The sweep redesign is careful.** Skipped lane rows keep `RetryCount`/`LastAttemptAt` untouched so the park horizon reflects real attempts; `GroupBy` stability preserves per-target FIFO; the observer pump preserves per-operation event order while latency-isolating the sweep; the gate is fail-safe (throwing → standby) and re-evaluated per tick.
|
||||
- **Storage hygiene done right:** additive `last_attempt_at_ms` with one-time guarded backfill and a covering `(status, due)` index; shared `InsertMessageSql` + `BindMessageParameters` so column lists can't drift; defensive `Guid.TryParse` on read so one corrupt row can't abort a sweep.
|
||||
- **The live alarm stream reuses instead of reinventing:** `SubscribeSite` rides the same hardened `RunSubscriptionStreamAsync` pipeline (readiness/shutdown gates, correlation-id validation, duplicate replacement, DropOldest with counted evictions, leak-proof subscribe failure cleanup — `SiteStreamGrpcServer.cs:253-385`); the client shares the proto→domain mapping; the actor copies the proven seed-then-stream/buffer/dedup/stability-window design from `DebugStreamBridgeActor` including its NUL-delimited three-part alarm key; the proto change is purely additive (new RPC + new request message).
|
||||
- **Failure-mode thinking is visible everywhere in the new code:** seed fan-out degrades per-instance rather than failing whole; a failed *initial* seed keeps `IsLive` false so the page keeps polling; publish callbacks are exception-isolated outside the lock; the viewer cap rejects with a no-op disposable rather than throwing into a render.
|
||||
- Docs were genuinely synchronized: `Component-StoreAndForward.md` (gate, resync, enqueue-only Send) and `Component-Communication.md` (U7 accepted-limitation) now match the code they describe.
|
||||
|
||||
---
|
||||
|
||||
## Prioritized Recommendations
|
||||
## Severity tally — NEW findings only
|
||||
|
||||
1. **[Critical] Fix ClusterClient recreation** (`CentralCommunicationActor.cs:514-521`, `DefaultSiteClientFactory`): unique per-incarnation actor names (suffix a counter) or Terminated-watch before re-create; validate/sanitize `siteId` for actor-name safety; wrap `Create` in the per-site try/catch; add a test using the real factory that edits a site's address twice.
|
||||
2. **[High] Gate the S&F retry sweep to the active node** — check leader/oldest status at the top of `RetryPendingMessagesAsync` (or gate `StartAsync`'s timer), re-evaluated per tick so failover resumes delivery within one sweep interval. Add a two-node test.
|
||||
3. **[High] Stop disposing shared gRPC clients on per-session failover** — key `SiteStreamGrpcClientFactory` by `(site, endpoint)` (allow both node channels concurrently), reserve dispose-and-replace for site removal/address edits driven by the address cache, not by individual bridge actors.
|
||||
4. **[High] Bound and de-serialize the retry sweep** — `LIMIT` the due query, short-circuit per target after the first transient failure in a sweep, and consider a small parallelism cap across distinct targets; separately consider a much shorter forward timeout for the notification category or `attemptImmediateDelivery: false` on `Notify.Send` to unblock script threads.
|
||||
5. **[Medium] Make replication trustworthy**: replace `Task.Run` fire-and-forget with an inline Tell (ordering), log failures at Warning with a counter metric, and make Park/Requeue applies upserts so a lost Add self-heals.
|
||||
6. **[Medium] Pin the wire format**: configure an explicit serializer (or at least add serializer round-trip compatibility tests for the cross-cluster Commons contracts) so the additive-only rule is actually enforced end-to-end; long-term, stop converting proto DTOs back to POCOs for the ClusterClient hop.
|
||||
7. **[Medium] Unstrand notifications**: pass `maxRetries: 0` (documented no-limit) or a category-specific high cap on the `Notify.Send` enqueue path, or auto-requeue parked Notification-category rows when central connectivity recovers.
|
||||
8. **[Medium] Park (don't silently drop) corrupt notification payloads** in `NotificationForwarder.DeliverAsync` — return `false` instead of `true`.
|
||||
9. **[Low] SQLite hygiene**: enable WAL + busy_timeout in `InitializeAsync`; migrate timestamps to epoch ms for the due-check.
|
||||
10. **[Low] Observability polish**: count DropOldest evictions in `StreamRelayActor`; replicate heartbeat marks to the peer central node (or document the failover blind window); split the app heartbeat interval from the transport failure-detector setting.
|
||||
| Severity | Count | Findings |
|
||||
|---|---|---|
|
||||
| Critical | 1 | N1 (resync predicate inversion wipes the delivering node's buffer) |
|
||||
| High | 1 | N2 (resync snapshot exceeds Akka remoting frame size — silently undeliverable) |
|
||||
| Medium | 2 | N3 (`_sweepTask` clobbering defeats shutdown drain), N6 (no live-delta coalescing) |
|
||||
| Low | 5 | N4 (validator gaps), N5 (resync/op race), N7 (aggregator lifecycle nits), N8 (standby aggregator + channel leak on site delete), N9 (dual-endpoint requirement) |
|
||||
|
||||
Both the Critical and the High are confined to the **peer-join anti-entropy resync** (PLAN-02 T20/T21) — the one round-2 feature that shipped without a two-node integration test — and both are small, well-localized fixes: swap two predicates to the existing shared helper, and chunk one message.
|
||||
|
||||
Reference in New Issue
Block a user