docs: close residual #3 (R2 merged); register row 47 (CTS-dispose bug, fixed in-flight); correct stale MaxBatchSize story in Component-StoreAndForward (R1 finding)
This commit is contained in:
@@ -40,6 +40,7 @@ All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round
|
|||||||
| 44 | **`BundleUnlockRateLimiter` is dead code — the documented per-IP unlock throttle is not enforced** *(Medium — security)* | Found by R6 | Registered, injected into `BundleImporter`, unit-tested, documented on `TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10`, pinned in `appsettings.Central.json` — but `_unlockRateLimiter` is never invoked anywhere in `src/`. A brute-force passphrase attacker is bounded only by the per-bundle 3-strike lockout, not the advertised 10/hour/IP cap. Fix: call it on the unlock path (or delete it and the config surface honestly). | Fix-now candidate — next Transport/security session |
|
| 44 | **`BundleUnlockRateLimiter` is dead code — the documented per-IP unlock throttle is not enforced** *(Medium — security)* | Found by R6 | Registered, injected into `BundleImporter`, unit-tested, documented on `TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10`, pinned in `appsettings.Central.json` — but `_unlockRateLimiter` is never invoked anywhere in `src/`. A brute-force passphrase attacker is bounded only by the per-bundle 3-strike lockout, not the advertised 10/hour/IP cap. Fix: call it on the unlock path (or delete it and the config surface honestly). | Fix-now candidate — next Transport/security session |
|
||||||
| 45 | **Notification Outbox dispatcher shares one `DbContext` across up to 4 parallel deliveries** *(Medium — correctness under load)* | Found by R6; observed continuously on rig central-b: `InvalidOperationException: A second operation was started on this context instance` on dispatch | `NotificationOutboxActor.ResolveAdapters()` caches adapter instances in an actor-lifetime `IServiceScope` while the Email/Sms adapters are `AddScoped` and hold a scoped `INotificationRepository` (`ScadaBridgeDbContext`); `MaxParallelDeliveries` (4) then shares one context across concurrent deliveries. The per-delivery scope added in `DeliverGatedAsync` for exactly this hazard covers only `INotificationOutboxRepository` — the adapter escapes it. Effect today: failed attempts + retries + error noise; sustained contention could burn a row to `Parked`. Fix: resolve adapters inside the per-delivery scope. | Fix-now candidate — next Notification Outbox session, or first `Parked` row attributed to it |
|
| 45 | **Notification Outbox dispatcher shares one `DbContext` across up to 4 parallel deliveries** *(Medium — correctness under load)* | Found by R6; observed continuously on rig central-b: `InvalidOperationException: A second operation was started on this context instance` on dispatch | `NotificationOutboxActor.ResolveAdapters()` caches adapter instances in an actor-lifetime `IServiceScope` while the Email/Sms adapters are `AddScoped` and hold a scoped `INotificationRepository` (`ScadaBridgeDbContext`); `MaxParallelDeliveries` (4) then shares one context across concurrent deliveries. The per-delivery scope added in `DeliverGatedAsync` for exactly this hazard covers only `INotificationOutboxRepository` — the adapter escapes it. Effect today: failed attempts + retries + error noise; sustained contention could burn a row to `Parked`. Fix: resolve adapters inside the per-delivery scope. | Fix-now candidate — next Notification Outbox session, or first `Parked` row attributed to it |
|
||||||
| 46 | **CLI `audit-config query` returns `{}` for every filter combination** *(Low)* | Found by R6 against the rig; underlying `AuditLogEntries` rows verified present | Possibly a CLI output-shape/serialization issue; not investigated (out of R6 scope). | Next CLI session |
|
| 46 | **CLI `audit-config query` returns `{}` for every filter combination** *(Low)* | Found by R6 against the rig; underlying `AuditLogEntries` rows verified present | Possibly a CLI output-shape/serialization issue; not investigated (out of R6 scope). | Next CLI session |
|
||||||
|
| 47 | ~~**`SiteStreamGrpcServer` duplicate-stream replacement disposed another handler's `CancellationTokenSource`**~~ **RESOLVED 2026-08-15 (found and fixed in-flight by R2, `e6842c10`).** `SubscribeInstance`'s duplicate-prevention path called `Cancel()` AND `Dispose()` on the replaced stream's CTS, which belongs to that still-running handler's `using var` and is still read — escaping as an unhandled `ObjectDisposedException` under scheduling pressure (confirmed pre-existing on `2b74851f`; load-only, passed 10/10 isolated). Fixed to cancel-only (the owner's `using` disposes exactly once), `Cancel` wrapped for the converse race, mirroring `CancelAllStreams()`. Deterministic regression test via a gate inside the setup window, fail-before/pass-after verified by reinstating the `Dispose()` as a negative control. Recorded here (though closed) because it is a product-bug instance of the disposal-race class, useful precedent for future stream-lifecycle work. | Closed 2026-08-15. |
|
||||||
|
|
||||||
## Resolved (verified against the code 2026-07-10)
|
## Resolved (verified against the code 2026-07-10)
|
||||||
Rows removed from the Deferred table above once confirmed shipped. Kept here for traceability.
|
Rows removed from the Deferred table above once confirmed shipped. Kept here for traceability.
|
||||||
|
|||||||
@@ -113,9 +113,19 @@ Deliberately not fixed in this program — each has a stated reason, not an over
|
|||||||
Library suite 199/199 incl. old-peer interop; ScadaBridge Host 490/490 on the bump. Adding an
|
Library suite 199/199 incl. old-peer interop; ScadaBridge Host 490/490 on the bump. Adding an
|
||||||
eleventh table to `ReplicatedTables` on a replicating site now snapshots ONE table instead of
|
eleventh table to `ReplicatedTables` on a replicating site now snapshots ONE table instead of
|
||||||
re-streaming all, both directions.
|
re-streaming all, both directions.
|
||||||
3. **Event batching per proto message.** Individual `AttributeValueChanged`/`AlarmStateChanged`
|
3. ~~**Event batching per proto message.**~~ **RESOLVED 2026-08-15 (`9b5cb3dd`+`e6842c10`,
|
||||||
events still ride one gRPC message each; batching them is a new wire shape (proto + both
|
R2).** Additive wire shape, no new RPC: `batching_supported` request flags +
|
||||||
client/server), deferred rather than folded into this program's additive-only changes.
|
`SiteStreamEvent.batch` (oneof field 4) carrying `SiteStreamEventBatch`. Negotiation is the
|
||||||
|
proto3 default itself and is LOAD-BEARING — an un-negotiated batch frame would parse as
|
||||||
|
`EventOneofCase.None` on an old central and vanish silently, so an old central (which cannot
|
||||||
|
set the flag) never receives one; an old site ignores the flag and keeps per-event frames.
|
||||||
|
Per-subscriber `SiteStreamEventBatcher` downstream of the relay's bounded channel (framing
|
||||||
|
only — never delays a lone event; lingers only once a backlog is proven); defaults 100
|
||||||
|
events / 25 ms, options-validated strictly < 250 ms to pin the load-test latency budget.
|
||||||
|
Measured worst case P99 25.4 ms; 600 queued events → 6 frames. Skew proven both directions
|
||||||
|
over real wire bytes. Histogram `scadabridge.site.stream.batch_size` (allowlist-verified).
|
||||||
|
Does NOT change the burst ceiling of register row 31 (documented). In-flight it also found
|
||||||
|
and fixed a pre-existing `SiteStreamGrpcServer` bug (register row 47).
|
||||||
4. ~~**Deployments page server-side paging + status counts.**~~ **RESOLVED 2026-08-15
|
4. ~~**Deployments page server-side paging + status counts.**~~ **RESOLVED 2026-08-15
|
||||||
(`35ce1413`, R3).** The Deployments page is database-paged: `QueryDeploymentListPageAsync`
|
(`35ce1413`, R3).** The Deployments page is database-paged: `QueryDeploymentListPageAsync`
|
||||||
returns one instance-joined page + the filtered total, `GetDeploymentStatusCountsAsync`
|
returns one instance-joined page + the filtered total, `GetDeploymentStatusCountsAsync`
|
||||||
|
|||||||
@@ -80,7 +80,7 @@ There is **no maximum buffer size**. Messages accumulate in the buffer until del
|
|||||||
- The standby node applies the same operations to its own local SQLite database but is **passive**: it never runs the delivery sweep. The retry sweep is **gated to the active node** (the oldest Up member / singleton host, re-evaluated every sweep tick), so only one node delivers at a time. The standby applies replicated operations purely to keep its copy warm for a future failover.
|
- The standby node applies the same operations to its own local SQLite database but is **passive**: it never runs the delivery sweep. The retry sweep is **gated to the active node** (the oldest Up member / singleton host, re-evaluated every sweep tick), so only one node delivers at a time. The standby applies replicated operations purely to keep its copy warm for a future failover.
|
||||||
- On failover, the new active node has a near-complete copy of the buffer. In rare cases, the most recent operations may not have been replicated (e.g., a message added or removed just before failover). This can result in a few **duplicate deliveries** (message delivered but its `Remove` not yet replicated) or a few **missed retries** (message added but not replicated). Duplicate deliveries are therefore confined to the **failover window** — an in-flight delivery whose `Remove` had not yet replicated — and never occur in steady-state operation (the standby's gate keeps it from delivering the same rows). Both are acceptable trade-offs for the latency benefit.
|
- On failover, the new active node has a near-complete copy of the buffer. In rare cases, the most recent operations may not have been replicated (e.g., a message added or removed just before failover). This can result in a few **duplicate deliveries** (message delivered but its `Remove` not yet replicated) or a few **missed retries** (message added but not replicated). Duplicate deliveries are therefore confined to the **failover window** — an in-flight delivery whose `Remove` had not yet replicated — and never occur in steady-state operation (the standby's gate keeps it from delivering the same rows). Both are acceptable trade-offs for the latency benefit.
|
||||||
- On failover, the new active node's gate flips to active within one sweep interval and it resumes delivery from its local copy.
|
- On failover, the new active node's gate flips to active within one sweep interval and it resumes delivery from its local copy.
|
||||||
- **Peer-join anti-entropy resync (LocalDb CDC).** *(Rewritten for LocalDb Phase 2, 2026-07-20. The previous specification of a chunked, ack-confirmed `SfBufferSnapshotChunk` protocol described the bespoke `ReplicationService`, which Phase 2 deleted. The discussion is rewritten rather than removed, because the failure modes it reasoned about still exist — they are simply bounded differently now.)* The buffer lives in the consolidated LocalDb database as the replicated `sf_messages` table, and both nodes exchange changes over a gRPC sync stream rather than Akka remoting. A node that was down for an extended period no longer requests a full-buffer snapshot and **replaces** its local buffer; LocalDb's snapshot resync merges **per row under last-writer-wins and never deletes**. Several of the old hazards are therefore structurally gone rather than guarded against: **(a) the 128 000-byte Akka frame limit no longer applies** — the transport is gRPC, whose successor ceiling is the 4 MB default receive limit, managed by bounding `LocalDb:Replication:MaxBatchSize` (set to 16 on the rig, sized against a ~70 KB worst-case `config_json`; see the Phase 2 plan, D6). Chunking, `ResyncId` assembly, assembly timeouts, and the truncation flag are all retired with it. **(b) The N1 directional-authority hazard is gone.** That guard existed because the bespoke resync applied a destructive delete-all-then-insert-all, so running it in the wrong direction wiped a live buffer. With a non-destructive merge there is no wipe to gate, and replication is symmetric — either node may write. `ActiveNodeEvaluator` survives, but only for the **delivery** gate and the heartbeat, which still genuinely need a single active node. **(c) The N5 orphan-row race is gone.** A `Remove` ordered before a re-add can no longer resurrect a row: deletes are tombstoned with an HLC, and a tombstone beats any older write for the same key. **The duplicate-delivery bound, stated explicitly.** Delivery remains single-node: only the primary runs the sweep (`IClusterNodeProvider.SelfIsPrimary`). A message can therefore be delivered twice only when the OLD primary delivered it and the resulting status change had not yet replicated at the instant the gate flipped. The window is one replication flush interval plus the in-flight ack, not an unbounded divergence — and unlike the old model it does not grow with backlog depth or with how long a node was absent. **One new bound replaces the old ones:** a node offline longer than `LocalDb:Replication:TombstoneRetention` (default 7 days) may resurrect deleted rows on rejoin, because the tombstones that would have suppressed them have been pruned. Stop-and-start a site pair together, and do not leave one node of a pair offline across that horizon.
|
- **Peer-join anti-entropy resync (LocalDb CDC).** *(Rewritten for LocalDb Phase 2, 2026-07-20. The previous specification of a chunked, ack-confirmed `SfBufferSnapshotChunk` protocol described the bespoke `ReplicationService`, which Phase 2 deleted. The discussion is rewritten rather than removed, because the failure modes it reasoned about still exist — they are simply bounded differently now.)* The buffer lives in the consolidated LocalDb database as the replicated `sf_messages` table, and both nodes exchange changes over a gRPC sync stream rather than Akka remoting. A node that was down for an extended period no longer requests a full-buffer snapshot and **replaces** its local buffer; LocalDb's snapshot resync merges **per row under last-writer-wins and never deletes**. Several of the old hazards are therefore structurally gone rather than guarded against: **(a) the 128 000-byte Akka frame limit no longer applies** — the transport is gRPC, whose successor ceiling is the 4 MB default receive limit, managed since LocalDb 0.2.0 by the **byte-budget** bound `LocalDb:Replication:MaxBatchBytes` (default 2 MB, enforced per wire message). `MaxBatchSize` is demoted to a secondary row cap whose remaining job is bounding the DB **read page** that `OplogStore`/`SnapshotStreamer` materialize in memory before the byte split runs (arch-review adversarial finding F2) — the rig pins it to 64 for that reason; the old rig pin of 16, hand-sized against a ~70 KB worst-case `config_json` as a byte-budget proxy (Phase 2 plan, D6), is retired. Chunking, `ResyncId` assembly, assembly timeouts, and the truncation flag are all retired with it. **(b) The N1 directional-authority hazard is gone.** That guard existed because the bespoke resync applied a destructive delete-all-then-insert-all, so running it in the wrong direction wiped a live buffer. With a non-destructive merge there is no wipe to gate, and replication is symmetric — either node may write. `ActiveNodeEvaluator` survives, but only for the **delivery** gate and the heartbeat, which still genuinely need a single active node. **(c) The N5 orphan-row race is gone.** A `Remove` ordered before a re-add can no longer resurrect a row: deletes are tombstoned with an HLC, and a tombstone beats any older write for the same key. **The duplicate-delivery bound, stated explicitly.** Delivery remains single-node: only the primary runs the sweep (`IClusterNodeProvider.SelfIsPrimary`). A message can therefore be delivered twice only when the OLD primary delivered it and the resulting status change had not yet replicated at the instant the gate flipped. The window is one replication flush interval plus the in-flight ack, not an unbounded divergence — and unlike the old model it does not grow with backlog depth or with how long a node was absent. **One new bound replaces the old ones:** a node offline longer than `LocalDb:Replication:TombstoneRetention` (default 7 days) may resurrect deleted rows on rejoin, because the tombstones that would have suppressed them have been pruned. Stop-and-start a site pair together, and do not leave one node of a pair offline across that horizon.
|
||||||
|
|
||||||
### Operation Tracking Table (lives in Site Runtime, not here)
|
### Operation Tracking Table (lives in Site Runtime, not here)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user