Six adversarial-review findings in the central SQL/ingest layer.
F1 (AuditLogRepository.InsertChunkAsync) — the set-based ingest declared each
string parameter at its COLUMN width (Actor/Target 256, Action 64, Outcome 16,
Category 32, SourceNode 64), so SqlClient truncated an over-long value at bind
time and committed the mutilated row — silent, in an append-only store, with no
PayloadTruncated flag — while the per-row and reconciliation paths sent the same
value in full and let the server reject it with 2628. Bind at the value's own
length instead; explicit SqlDbType is kept (it fixes the VALUES constructor's
derived column types and datetime2 precision). Design: reject everywhere,
truncate nowhere — matching today's per-row behaviour.
F2 (SiteCallAuditRepository.UpsertAsync) — the single-statement upsert ran the
monotonic UPDATE first and INSERTed only if nothing matched. Two writers racing
the first packet of one TrackedOperationId (the cached dual-write and the
reconciliation pull carry DIFFERENT lifecycle states) both matched nothing, and
the loser then skipped its INSERT or swallowed a 2627 — dropping its
Status/RetryCount/HttpStatus/TerminalAtUtc. Legs swapped to
`IF NOT EXISTS … INSERT; UPDATE <monotonic>` — still one round trip, and the
loser's UPDATE now lands on the winner's row. The duplicate-key catch re-runs
the monotonic UPDATE for the same reason. Moved to raw SQL with explicitly-typed
parameters so the intricate rank predicate exists in exactly one place (an
untyped DateTime would bind as `datetime` and round the freshness tiebreaker).
F3 (docs/plans/sql/*.sql) — filtered-index DDL failed with error 1934 under the
documented `docker exec … sqlcmd` path, which defaults QUOTED_IDENTIFIER OFF;
once IX_Notifications_Delivered exists, QI-OFF DML on Notifications fails too.
All four scripts now open with `SET QUOTED_IDENTIFIER ON; SET ANSI_NULLS ON; GO`
(own batch, so it is in force when the next batch parses), and the migration
convention in Component-ConfigurationDatabase.md documents `sqlcmd -I`. Verified
live: the pre-fix script fails 1934 without -I, the fixed one applies.
F4 (SiteCallAuditActor) — the off-mailbox reconciliation/purge passes reuse the
injected repository, so tests drove one DbContext from the pass and a mailbox
handler concurrently. Serialized at the CALL via a private SerializedRepository
wrapper applied only by the test constructors, rather than running the pass
on-mailbox: production keeps its PipeTo shape untouched, and the existing
"a blocked drain does not stall ingest/query/KPI" regression tests stay
meaningful (they would have been invalidated by suspending the mailbox).
F5 (AuditLogIngestActor) — when the batch failed because the 20 s IngestBudget
expired, the per-row fallback reused the same expired token: N instant failures,
N counter bumps, zero accepted. The fallback now gets a fresh 5 s budget (inside
the 30 s outer Ask), and a blown budget bumps the failure counter ONCE for the
batch instead of once per row.
F6 (NotificationOutboxRepository.UpdateAsync) — ExecuteUpdate's row count was
discarded, so an operator Retry/Discard of a notification the retention purge had
already deleted reported success (the pre-ExecuteUpdate code threw
DbUpdateConcurrencyException). UpdateAsync now returns whether a row matched; the
operator one-shots answer "notification not found" and emit no audit row for the
action that did not happen, while the dispatcher logs a warning (its delivery
already happened; nothing to retry). GetByIdAsync switched to AsNoTracking since
the write is out-of-band.
Tests: 5 new SQL-backed regressions (over-long Target rejected on both paths +
boundary round-trip; concurrent first-write and already-created-by-another-writer
upserts; vanished-row UpdateAsync), a token-identity pin on the ingest fallback,
a repository-concurrency detector for the SiteCallAudit passes, and vanished-row
operator-path tests. The F1/F2/F4 regressions were each confirmed failing against
the pre-fix code. Suites: ConfigurationDatabase 369, AuditLog 378, SiteCallAudit
66, NotificationOutbox 152 — all green, solution builds with 0 warnings.
Additive RequestedBy on Retry/DiscardNotificationRequest, plumbed from the Central
UI. RetryAsync emits a NotifyDeliver Submitted row (records who un-parked); Discard
stamps the operator on the Terminal row. Best-effort — audit failure never aborts
the action.
Gitea renders mermaid inline, so the flow/state/hierarchy/DAG diagrams
move to text-in-markdown: auto-layout (removes the manual overlap-prone
draw.io step), diffable source, no committed binaries, and a dark-text
theme so labels stay legible. Keep draw.io PNGs only for the two complex
bespoke diagrams (logical architecture, env2 topology) where pixel
control still wins. All 24 mermaid blocks validated by rendering.
Replace ASCII-art diagrams across the README and docs/ with editable
.drawio sources plus exported PNGs, so the diagrams render clearly in
rendered markdown and can be maintained/regenerated instead of being
hand-edited as fragile text art. Non-diagram blocks (code, folder
trees, UI wireframes) were left as text.
- Adds SourceNode varchar(64) NULL to AuditLog, Notifications, and SiteCalls
tables with role-name semantics: node-a/node-b for site rows (qualified by
SourceSiteId), central-a/central-b for central direct-write rows.
- New IX_AuditLog_Node_Occurred (SourceNode, OccurredAtUtc) index.
- Reframes CLAUDE.md from documentation-only to implementation project.
- Adds docs/plans/2026-05-23-audit-source-node.md + tasks.json companion.
Final cross-bundle reviewer identified 7 inconsistencies that the per-bundle
reviewers couldn't see; all fixed in one logical commit.
Critical:
- HighLevelReqs AL-3: drop 'then upsert-on-newer-status' — AuditLog is
strictly append-only (correct for SiteCalls/Notifications, wrong for
the immutable AuditLog shadow).
- Component-AuditLog Error rate KPI: align with HealthMonitoring's
exclusion list (Success/Delivered/Enqueued) rather than just non-Success;
otherwise every Delivered notification or Enqueued cached call would be
counted as an error.
Important:
- Component-AuditLog line 154: ISiteAuditWriter -> IAuditWriter (canonical
name per Commons and the rest of this doc).
- Component-AuditLog Central direct-write paragraph: convert remaining
slash notation (ApiInbound/Completed, Notification/Attempt,
Notification/Terminal) to dot notation used everywhere else.
- Component-ClusterInfrastructure: scope SiteCallAuditActor to
reconciliation + KPIs + Retry/Discard relay; cached-telemetry ingest is
AuditLogIngestActor's role per Combined Telemetry contract.
- Component-CentralUI Audit Log page: state the OperationalAudit read
permission and the read-vs-export split (matching CLI doc).
- Component-NotificationOutbox: add never-fail-the-action invariant for
dispatcher audit writes.
Minor:
- Component-InboundAPI: 'Non-blocking semantics' was ambiguous (could be
read as async); reword to 'Fail-soft' — the write is still synchronous
before flush, but failures are caught and don't change the response.
- Component-CLI: realign audit-query/audit-export flags to actually match
the Central UI Audit Log filter set (channel, kind, status, site,
instance, target, actor, correlation-id, errors-only); drop --user and
--entity-id which are IAuditService concepts, not Audit Log columns.
- Component-AuditLog KPI tile names: 'Volume/Error rate/Backlog' ->
'Audit volume/Audit error rate/Audit backlog' (matches Central UI and
Health Monitoring); drop the two orphan KPIs (Top inbound callers, Top
outbound 5xx) that were never surfaced anywhere.
- Component-AuditLog Interactions: re-attribute DbOutbound emissions to
ESG (where Database.* lives) with a note that Site Runtime is the API
surface for scripts.
- HighLevelReqs AL-12: drop 'and reconciliation operations' (CLI has no
reconcile command; reconciliation is an internal self-healing pull).
Add note that verify-chain becomes operational once AL-11's hash chain
ships.
Joseph Doherty