Merge branch 'worktree-agent-adf34e265d2dcae96' into arch-review-remediation
This commit is contained in:
@@ -305,6 +305,24 @@ per-row / per-entry path — so the documented invariant that one bad row cannot
|
||||
sink the rest of the batch still holds, it is simply no longer paid for on the
|
||||
healthy path.
|
||||
|
||||
The set-based path binds its string parameters at the VALUE's own length, never
|
||||
at the column width. Declaring the width makes the client TRUNCATE an over-long
|
||||
value at bind time and commit the shortened row — silent mutilation in an
|
||||
append-only store, with no `PayloadTruncated` flag to admit it, and inconsistent
|
||||
with the per-row and reconciliation paths, which send the value in full and let
|
||||
the server reject it. Length enforcement belongs to the server on every path:
|
||||
reject everywhere, truncate nowhere. (Deliberate, policy-driven truncation of
|
||||
`RequestSummary`/`ResponseSummary` under the payload caps is a different thing
|
||||
entirely — it happens before the write and always sets `PayloadTruncated`.)
|
||||
|
||||
The per-row fallback runs on its OWN short cancellation budget rather than the
|
||||
batch's. Sharing it meant that a batch failing *because* the ingest budget
|
||||
expired handed every fallback insert an already-cancelled token: N instant
|
||||
failures, N counter bumps, nothing accepted — the fallback defeated at exactly
|
||||
the moment it was needed. A blown budget is also counted ONCE for the batch
|
||||
rather than once per row, so the health counter reads as one timeout instead of
|
||||
a burst of write failures.
|
||||
|
||||
**Timeout ladder.** The ingest budget is deliberately the smallest on the path:
|
||||
the site's Ask and the central gRPC handler's Ask are both 30 s, the actor's
|
||||
own database budget is 20 s and the per-statement SQL timeout is 15 s. Before
|
||||
|
||||
@@ -379,6 +379,22 @@ dotnet ef migrations script FromMigration ToMigration --output migration.sql
|
||||
|
||||
Generated scripts are idempotent — they can be safely re-run without causing errors or duplicate changes.
|
||||
|
||||
### Running a Script: QUOTED_IDENTIFIER
|
||||
|
||||
`SiteCalls`, `Notifications` and `AuditLog` all carry **filtered indexes**, and `AuditLog` additionally carries persisted computed columns. SQL Server refuses both the filtered-index DDL and any subsequent `INSERT`/`UPDATE`/`DELETE` on such a table with **error 1934** unless the session has `QUOTED_IDENTIFIER ON`.
|
||||
|
||||
`sqlcmd` defaults that option **OFF** — including the `docker exec … /opt/mssql-tools18/bin/sqlcmd` invocation used throughout the test-infra docs — so a script that applies cleanly in SSMS (which defaults ON) fails there. Two mitigations, both in place:
|
||||
|
||||
- Every checked-in script under `docs/plans/sql/` begins with a `SET QUOTED_IDENTIFIER ON; SET ANSI_NULLS ON; GO` header, in its own batch so the setting is in force when the following batch parses.
|
||||
- Run `sqlcmd` with **`-I`** (enable quoted identifiers) regardless — it is the correct default for every script this repo generates, and it protects hand-written one-off statements that carry no header:
|
||||
|
||||
```bash
|
||||
docker exec -i scadabridge-mssql /opt/mssql-tools18/bin/sqlcmd \
|
||||
-S localhost -U sa -P 'ScadaBridge_Dev1#' -C -I -d ScadaBridgeConfig -i /path/to/script.sql
|
||||
```
|
||||
|
||||
The application itself is unaffected: SqlClient sets `QUOTED_IDENTIFIER ON` on every connection, so EF Core migrations and all runtime DML already run with it on.
|
||||
|
||||
---
|
||||
|
||||
## Seed Data
|
||||
|
||||
@@ -167,6 +167,8 @@ A notification is **stuck** if it is `Pending` or `Retrying` and older than a co
|
||||
- **Health Monitoring dashboard** — headline KPI tiles: queue depth, stuck count, parked count. These are central-computed and are not part of the site health report. The site S&F notification backlog remains a separate site health metric covering the site→central leg.
|
||||
- **Central UI "Notification Outbox" page** — KPI tiles plus a queryable notification list: filter by status, type, source site, list, and time range; a stuck-only toggle; keyword search on subject. Parked notifications offer **Retry** (→ `Pending`, reset `RetryCount` / `NextAttemptAt`) and **Discard** (→ `Discarded`) actions. Stuck rows are badged.
|
||||
|
||||
Both operator actions are **read-then-write against a row the daily retention purge may delete in between**, so the write reports whether it matched a row and the actor answers `Success: false` / `"notification not found"` when it did not — never a success against a notification that no longer exists (and no audit row is emitted for the action that did not happen). The dispatcher's own delivery-state write takes the opposite stance on the same signal: the delivery has already happened, there is nothing left to retry, so a vanished row is logged as a warning and the audit rows stand as the durable record.
|
||||
|
||||
## Configuration
|
||||
|
||||
The component is configured via `NotificationOutboxOptions`, bound from an `appsettings.json` section on the central host (Options pattern):
|
||||
|
||||
@@ -148,15 +148,27 @@ and the mailbox supplies the memory barrier between consecutive passes. The
|
||||
daily terminal-row purge uses the same shape. (`NotificationOutboxActor`'s
|
||||
dispatch sweep is the in-repo reference.)
|
||||
|
||||
**The central upsert is one statement.** `SiteCallAuditRepository.UpsertAsync`
|
||||
issues a single batch that runs the monotonic UPDATE first and INSERTs only when
|
||||
nothing matched *and* the row genuinely does not exist — instead of an
|
||||
unconditional insert-if-absent followed by the update, which cost two round trips
|
||||
on every packet and wasted the insert half for every packet after the first. The
|
||||
existence re-check is load-bearing: a zero row count also means "the monotonic
|
||||
**The central upsert is one round trip, INSERT-first.**
|
||||
`SiteCallAuditRepository.UpsertAsync` ships both statements in a single command
|
||||
text — `IF NOT EXISTS … INSERT;` then the monotonic `UPDATE` — so a packet costs
|
||||
one round trip, not two. The insert leg is gated on the row's existence *alone*,
|
||||
never on "the update matched nothing": a zero row count also means "the monotonic
|
||||
guard rejected this packet", and inserting there would fork the mirror with a
|
||||
second row for an id that already exists.
|
||||
|
||||
The ORDER is load-bearing, and running the UPDATE first is a data-loss bug. The
|
||||
two writers — the cached dual-write and the reconciliation pull — routinely carry
|
||||
*different* lifecycle states for the same `TrackedOperationId`, and both can race
|
||||
its very first packet. Under UPDATE-first both find no row, so both updates match
|
||||
nothing; the loser then either fails its existence re-check and skips its insert,
|
||||
or attempts it and takes a duplicate-key fault — either way its
|
||||
`Status`/`RetryCount`/`HttpStatus`/`TerminalAtUtc` are dropped, because it never
|
||||
ran an update against the winner's row. Under INSERT-first the loser's insert is
|
||||
skipped or faults and its monotonic update still applies to whichever row won, so
|
||||
the newer state survives every interleaving while a stale one is still rejected by
|
||||
the rank guard. The duplicate-key catch re-runs the monotonic update for the same
|
||||
reason (it is idempotent and rank-guarded, so a redundant re-run is inert).
|
||||
|
||||
## Retry / Discard Relay
|
||||
|
||||
Parked cached calls live in the owning site's S&F buffer. Operator Retry/Discard
|
||||
|
||||
Reference in New Issue
Block a user