Merge small-fixes wave: rows 74-77, 62, 79-runbook (gauge delta asserts, Approve caution styling via DialogSeverity, CLI README sample, ActorCell sweep clean, ISiteEventLogger lifetime pin, OrbStack/virtiofs runbook)
This commit is contained in:
@@ -311,6 +311,103 @@ a rejoin is safe and self-correcting (verified live: a node stopped and restarte
|
||||
with both nodes byte-identical and zero duplicates). Beyond it, rebuild the returning node's
|
||||
database from its peer rather than letting it rejoin.
|
||||
|
||||
### Docker-rig data files on macOS/OrbStack — a virtiofs corruption + availability hazard
|
||||
|
||||
**Scope: the local `docker/` and `docker-env2/` development rigs on macOS only.** This is a
|
||||
host-level property of the filesystem the rig's data files sit on, **not a ScadaBridge defect** and
|
||||
not something a production Linux deployment is exposed to. It is documented here because it has
|
||||
already cost two databases and half a session, and because the recovery is non-obvious.
|
||||
|
||||
Every ScadaBridge data file on the rig rides a **relative bind mount** out of the repo working tree
|
||||
— `./site-a-node-a/data:/app/data` and its siblings, which is where `site-localdb.db`,
|
||||
`auditlog.db` and the secrets store all live. On macOS that path crosses **virtiofs** to reach the
|
||||
Linux VM, and **virtiofs does not give SQLite true `fsync` durability**. SQLite's crash-safety
|
||||
contract assumes a successful `fsync` means the bytes are on stable storage; when that is not true,
|
||||
an abrupt container kill mid-write can leave a torn file. `docker compose up --force-recreate`
|
||||
— which `deploy.sh` uses — is exactly such an abrupt kill.
|
||||
|
||||
Two distinct failure modes have been observed live (both 2026-08-15):
|
||||
|
||||
**1. Silent database corruption.** Both site-a LocalDb files were found malformed:
|
||||
|
||||
```
|
||||
SQLite Error 11: 'database disk image is malformed'
|
||||
```
|
||||
|
||||
Note how differently the two nodes presented, because this is the part that makes it hard to spot:
|
||||
|
||||
- **site-a-b** surfaced it as a *running* fault — every sync call threw, replication reported
|
||||
`connected: false` with a backlog of 892, and the faulted session retried about once a minute.
|
||||
The node stayed up and otherwise healthy-looking.
|
||||
- **site-a-a** was corrupt too, but **latently** — nothing on the running build ever read the
|
||||
damaged pages. It only became fatal on the *next upgrade*, when a new build's boot-time
|
||||
re-registration read pages the old build never touched: `SqliteLocalDb` construction threw and
|
||||
the container **SIGABRT crash-looped** (×12).
|
||||
|
||||
The lesson to carry: a clean-looking node proves nothing about its file. Corruption can sit latent
|
||||
until an unrelated code change happens to read further into the database.
|
||||
|
||||
**2. The OrbStack VM idle-sleeping mid-operation.** The VM can suspend under its idle policy while
|
||||
work is in flight. `~/.orbstack/log/vmgr.log` records it plainly:
|
||||
|
||||
```
|
||||
vmgr ... msg=sleep
|
||||
```
|
||||
|
||||
On wake, the virtiofs layer logs repeated errors of the form:
|
||||
|
||||
```
|
||||
refresh_nodeid: race with new lookup
|
||||
```
|
||||
|
||||
and `docker` hangs — commands stop returning, `docker ps` comes back empty — for as long as it
|
||||
takes to notice. **The fix is `orb restart docker`**; data survives and containers auto-restart.
|
||||
Observed cost: roughly 20 minutes of a session lost to diagnosing what looked like a cluster
|
||||
failure but was a host one. If the rig goes unresponsive as a whole, check `vmgr.log` **before**
|
||||
investigating ScadaBridge.
|
||||
|
||||
#### Recovery runbook — LocalDb corruption on a site pair (proven)
|
||||
|
||||
This is the procedure that recovered site-a. It relies on the pair being able to reseed itself,
|
||||
so it is a *replicated* pair's recovery; an unreplicated node has no peer to reseed from and must
|
||||
be restored from a backup instead.
|
||||
|
||||
1. **Stop BOTH nodes of the site pair, together.** Not one at a time — this is the same
|
||||
stop-and-start-together rule as *Site Pair Upgrades* above, for the same reason.
|
||||
2. **Quarantine the database and its sidecars — do not delete them.** Rename all three, keeping
|
||||
them for forensics:
|
||||
|
||||
```sh
|
||||
# per affected node's data directory
|
||||
mv site-localdb.db site-localdb.db.corrupt-$(date +%F)
|
||||
mv site-localdb.db-wal site-localdb.db-wal.corrupt-$(date +%F)
|
||||
mv site-localdb.db-shm site-localdb.db-shm.corrupt-$(date +%F)
|
||||
```
|
||||
|
||||
The `-wal` and `-shm` files **must** move with the main file. Leaving a stale WAL beside a fresh
|
||||
database is its own corruption.
|
||||
3. **Start both nodes together.** The fresh files reseed themselves from two independent paths:
|
||||
`SiteReconciliationActor`'s node-startup fetch pulls config back from central when central
|
||||
reports gaps, and LocalDb's **CDC snapshot resync** merges the peer's rows under LWW.
|
||||
4. **Verify convergence**, do not assume it: `LocalDbReplicationConnected` should read `true` and
|
||||
`LocalDbOplogBacklog` should settle to ~0 on the site health report. (See *Reading the
|
||||
replication backlog* above for why a transient spike is normal and not a fault.)
|
||||
|
||||
On the live recovery, both quarantined files were large (394 MB and 416 MB), the pair converged to
|
||||
`connected: true` / backlog 0, and a schema change in flight at the time additionally proved the
|
||||
digest handshake green.
|
||||
|
||||
#### Mitigation options, with their trade-offs
|
||||
|
||||
None of these is applied today; the rig deliberately still runs on bind mounts, and the accepted
|
||||
position is the runbook above. Recorded so the choice is a choice:
|
||||
|
||||
| Option | Gains | Costs |
|
||||
|--------|-------|-------|
|
||||
| Move rig data dirs to **named docker volumes** | Files live on native ext4 inside the VM, out of virtiofs entirely — removes the corruption mechanism at the source | Loses the host-side inspectability the debugging sessions lean on constantly: no more opening `site-localdb.db` from the Mac, no `ls` of a node's data dir from the repo tree. Requires a compose change. |
|
||||
| **Disable OrbStack's idle pause** | Removes failure mode 2 (the mid-operation sleep and the virtiofs `refresh_nodeid` storm) | A host-level setting outside this repo, so it cannot be version-controlled or applied by `deploy.sh` — it silently does not exist on a new machine. Does nothing about failure mode 1. |
|
||||
| **Accept, and use this runbook** (current) | No changes to compose or host config; full host-side inspectability retained | Corruption recurs on the next unlucky `--force-recreate`, and can sit latent until a later upgrade turns it into a crash loop. |
|
||||
|
||||
### Central-Site Communication
|
||||
|
||||
Three transports cross the boundary, not one — **all now gRPC or HTTP; Akka ClusterClient was removed
|
||||
|
||||
@@ -14,6 +14,11 @@ Central cluster only. Sites have no user interface.
|
||||
- Keeps the entire stack in C#/.NET, consistent with the rest of the system (Akka.NET, EF Core).
|
||||
- SignalR provides built-in support for real-time UI updates.
|
||||
- **Dialog service confirm labels.** Every confirmation goes through `IDialogService.ConfirmAsync(title, message, danger, confirmLabel)`, rendered by the single `DialogHost` in `MainLayout`. `danger` selects **styling only** (`btn-danger`); `confirmLabel` names the action. When no label is supplied the button falls back to "Delete" for a danger confirm and "Confirm" otherwise — a convenience for the ~20 genuine delete call sites, and the reason the Health dashboard's *Trigger failover* dialog used to offer a red **Delete** button (deferred-work register row 28). **Any destructive action that is not literally a delete must pass `confirmLabel`**: failover → *Fail over*, secured-write approve/reject → *Approve* / *Reject*, instance disable → *Disable*, and every parked-message / notification / cached-call discard → *Discard*.
|
||||
- **Dialog confirm severity — three rungs, not two** (deferred-work register row 75). `DialogSeverity` drives the confirm button's colour: `Normal` → `btn-primary`, `Caution` → `btn-warning`, `Destructive` → `btn-danger`. Only `Destructive` inherits the "Delete" label fallback above; an unlabelled `Caution` reads *Confirm*, because a caution action is by definition not a delete.
|
||||
- **Why a middle rung exists.** The boolean was a two-state flag for a three-state concept, so a consequential-but-not-destructive action had to choose between looking like a delete and looking like the safe default. The **Secured Writes** page is where that bit: *Approve* and *Reject* both rendered as identical red buttons, so at the moment of a two-person-control decision the colour carried **no discriminating information at all** — and the affirmative half of the control wore the delete affordance.
|
||||
- **The decision.** *Approve* is `Caution` (amber): approving executes a real write to live plant equipment, so it is not a benign primary action, but it is not destroying anything either. *Reject* **keeps** `Destructive` (red): it is terminal and irreversible for the request — the Operator must resubmit — and holding it red is what keeps the two opposite decisions visually distinct. Dropping *Reject* to a neutral button was considered and rejected: it would simply invert the bias, making declining look like the safe default.
|
||||
- Colour is never the only signal — the button always carries its verb (see the label contract above), so the mapping is an accessibility affordance, not the meaning.
|
||||
- **`DialogSeverity` is an additive overload**, not a replacement: the boolean form remains the signature for the ~34 existing call sites, which are genuinely two-state. Adding a fifth optional parameter to the boolean form instead would have silently un-matched every existing four-argument NSubstitute stub — the row-28 gotcha, where a stub that stops matching returns a null `Task` and the caller NREs with no compile error. Reach for the overload only when `Caution` is what you mean.
|
||||
|
||||
## Failover Behavior
|
||||
|
||||
@@ -226,7 +231,7 @@ Per-leaf alarm rendering (leaf nodes are individual conditions for native alarms
|
||||
- A **Secured Writes** page (`/operations/secured-writes`) drives the **two-person** authorization workflow for writes through the MxAccess Gateway: an **Operator** initiates the write, a separate **Verifier** approves it, and only an approved write reaches the site.
|
||||
- The **page itself is gated by `RequireSecuredWriteAccess`** — satisfied by any of `Operator` / `Verifier` / `Administrator`. Although the pending/history lists are read-only, they expose process-sensitive tag values, so the page is not open to every authenticated user; this aligns with the ManagementActor `ListSecuredWritesCommand` any-of gate (arch-review UA1). The submit and approve/reject sub-actions are further gated by `RequireOperator` / `RequireVerifier` respectively (below).
|
||||
- **Operator (submit)** — a submit form gated by `RequireOperator`: pick the site → an **MxGateway** connection on that site → the tag path → a typed value → an optional comment. Submission inserts a `Pending` `PendingSecuredWrite` row centrally; it does **not** write anything yet.
|
||||
- **Verifier (approve / reject)** — a pending queue gated by `RequireVerifier` with **Approve** / **Reject** (+comment) actions. Approve shows a confirmation of the exact site / connection / tag / value before firing, and surfaces the **submission age** (`Submitted <age> ago (<UTC timestamp>)`) so a verifier can catch a stale setpoint before it reaches the device. The verifier's **own submissions are disabled in the UI and rejected server-side** (no self-approval). On approve, central marks the row `Approved` and relays the write to the site MxGateway (records `Executed` / `Failed`); reject moves it to `Rejected` with a reason. The pending queue is **unpaged** — it is bounded by the submission **TTL**: a `Pending` row that is neither approved nor rejected before the TTL elapses expires to the terminal **`Expired`** status and drops out of the queue into History.
|
||||
- **Verifier (approve / reject)** — a pending queue gated by `RequireVerifier` with **Approve** / **Reject** (+comment) actions. Approve shows a confirmation of the exact site / connection / tag / value before firing, and surfaces the **submission age** (`Submitted <age> ago (<UTC timestamp>)`) so a verifier can catch a stale setpoint before it reaches the device. The two confirmations are deliberately styled apart: **Approve is amber (`DialogSeverity.Caution`) and Reject stays red** — see the confirm-severity contract under Technology for why (register row 75). The verifier's **own submissions are disabled in the UI and rejected server-side** (no self-approval). On approve, central marks the row `Approved` and relays the write to the site MxGateway (records `Executed` / `Failed`); reject moves it to `Rejected` with a reason. The pending queue is **unpaged** — it is bounded by the submission **TTL**: a `Pending` row that is neither approved nor rejected before the TTL elapses expires to the terminal **`Expired`** status and drops out of the queue into History.
|
||||
- **History** — terminal rows (Executed / Failed / Rejected / **Expired**, where `Expired` is a real TTL-driven terminal status) with the full who/when/outcome trail (operator, verifier, comments, timestamps, any execution error). Terminal rows accumulate without bound, so the History table is **offset-paged** (page-size 50, Prev/Next) driven by the server's unpaged `TotalCount` (`ListSecuredWritesCommand` `Skip`/`Take`); the pager buttons disable at the real bounds.
|
||||
- Every lifecycle event (submit / approve / reject / execute) is written to the central Audit Log; the rows share the `PendingSecuredWrite.Id` as `CorrelationId` so they join into one operation (see Component-ManagementService.md, Component-AuditLog.md).
|
||||
- **Dev caveat**: with `DisableLogin` on, the auto-login identity holds all roles, so the two-person flow cannot be exercised end-to-end by a single user via the dev UI — no-self-approval is covered by handler tests; real two-person use requires two real identities.
|
||||
|
||||
@@ -1829,13 +1829,32 @@ Output shape:
|
||||
"entityType": "Template",
|
||||
"entityId": "7",
|
||||
"entityName": "PumpTemplate",
|
||||
"timestamp": "2026-08-15T12:00:00+00:00"
|
||||
"afterStateJson": "{\"Id\":7,\"Name\":\"PumpTemplate\",\"FolderId\":3}",
|
||||
"timestamp": "2026-08-15T12:00:00+00:00",
|
||||
"bundleImportId": null
|
||||
},
|
||||
{
|
||||
"id": 413,
|
||||
"user": "designer",
|
||||
"action": "Deleted",
|
||||
"entityType": "ExternalSystem",
|
||||
"entityId": "3",
|
||||
"entityName": "MES",
|
||||
"afterStateJson": null,
|
||||
"timestamp": "2026-08-15T12:05:00+00:00",
|
||||
"bundleImportId": "9f2c1e40-5b31-4a7e-8d66-0c1a4b93ff21"
|
||||
}
|
||||
],
|
||||
"totalCount": 137
|
||||
}
|
||||
```
|
||||
|
||||
Both nullable fields are always present on the wire. `afterStateJson` is a JSON snapshot
|
||||
of the entity's state *after* the change, embedded as a string — it is `null` for deletes,
|
||||
since there is no after-state. `bundleImportId` is the import session id when the change
|
||||
was made by a bundle import (`transport import`), and `null` for a change made directly;
|
||||
it is the join key for "what did this bundle actually change".
|
||||
|
||||
`totalCount` is the unpaged number of rows matching the filters, so a client can page
|
||||
without a second call. JSON is the primary format for this command and is what scripts
|
||||
should consume — it is never altered by the table projection below.
|
||||
|
||||
+13
-1
@@ -424,7 +424,13 @@
|
||||
$"Value: {row.ValueJson} ({row.ValueType})\n" +
|
||||
$"Submitted {FormatAge(row.SubmittedAtUtc)} ago ({row.SubmittedAtUtc:u})\n" +
|
||||
$"Operator: {row.OperatorUser}",
|
||||
danger: true,
|
||||
// Caution, NOT danger (deferred-work register row 75). Approving executes a
|
||||
// real write to live plant equipment, so this is not a benign primary action
|
||||
// — but it is not destroying anything either, and red made it look like a
|
||||
// delete while rendering it identical to the Reject button right beside it.
|
||||
// Amber keeps the beat of hesitation the write deserves and restores the
|
||||
// colour contrast between the two opposite decisions.
|
||||
DialogSeverity.Caution,
|
||||
confirmLabel: "Approve");
|
||||
if (!confirmed) return;
|
||||
|
||||
@@ -461,6 +467,12 @@
|
||||
var confirmed = await Dialog.ConfirmAsync(
|
||||
"Reject secured write",
|
||||
$"Reject the secured write to {row.SiteId}/{row.ConnectionName}/{row.TagPath}?",
|
||||
// Reject KEEPS danger styling (register row 75). It is terminal and
|
||||
// irreversible for the request — the Operator has to resubmit — and holding
|
||||
// it red is what makes the pair readable: with Approve amber, the two
|
||||
// opposite decisions no longer look alike. Dropping Reject to a neutral
|
||||
// button would just invert the bias, making declining look like the safe
|
||||
// default.
|
||||
danger: true,
|
||||
confirmLabel: "Reject");
|
||||
if (!confirmed) return;
|
||||
|
||||
@@ -50,7 +50,7 @@
|
||||
<div class="btn-group btn-group-sm" role="group">
|
||||
<button type="button" class="btn btn-outline-secondary" @onclick="Cancel">Cancel</button>
|
||||
<button type="button"
|
||||
class="btn @(state.Danger ? "btn-danger" : "btn-primary")"
|
||||
class="btn @ConfirmButtonClass(state)"
|
||||
@onclick="Confirm">
|
||||
@ConfirmLabel(state)
|
||||
</button>
|
||||
@@ -185,17 +185,32 @@
|
||||
svc.Resolve(resolveValue);
|
||||
}
|
||||
|
||||
// An explicit label from the caller always wins. The kind/danger fallback below
|
||||
// is a convenience for the common cases only: a bare danger confirm is assumed to
|
||||
// be a delete, which is exactly the assumption that put "Delete" on the Trigger
|
||||
// Bootstrap class for the confirm button, by severity (deferred-work register row 75).
|
||||
// Caution is the middle rung: amber, for an action that is consequential but is not
|
||||
// destroying anything — the Secured Writes "Approve" is the case it was added for,
|
||||
// where red made the affirmative half of a two-person control look like a delete and
|
||||
// made Approve and Reject visually indistinguishable. Colour is never the only signal:
|
||||
// the button is always labelled with its verb.
|
||||
private static string ConfirmButtonClass(DialogState state) => state.Severity switch
|
||||
{
|
||||
DialogSeverity.Destructive => "btn-danger",
|
||||
DialogSeverity.Caution => "btn-warning",
|
||||
_ => "btn-primary",
|
||||
};
|
||||
|
||||
// An explicit label from the caller always wins. The kind/severity fallback below
|
||||
// is a convenience for the common cases only: a bare destructive confirm is assumed
|
||||
// to be a delete, which is exactly the assumption that put "Delete" on the Trigger
|
||||
// failover button (deferred-work register row 28). Callers whose destructive verb
|
||||
// is not "Delete" must pass ConfirmLabel — see IDialogService.ConfirmAsync.
|
||||
// Note Caution deliberately does NOT inherit that fallback: an unlabelled caution
|
||||
// dialog reads "Confirm", because a caution action is by definition not a delete.
|
||||
private static string ConfirmLabel(DialogState state) =>
|
||||
!string.IsNullOrWhiteSpace(state.ConfirmLabel)
|
||||
? state.ConfirmLabel
|
||||
: state.Kind switch
|
||||
{
|
||||
DialogKind.Prompt => "Save",
|
||||
_ => state.Danger ? "Delete" : "Confirm",
|
||||
_ => state.Severity == DialogSeverity.Destructive ? "Delete" : "Confirm",
|
||||
};
|
||||
}
|
||||
|
||||
@@ -35,7 +35,15 @@ public class DialogService : IDialogService
|
||||
private TaskCompletionSource<object?>? _tcs;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) =>
|
||||
ConfirmAsync(
|
||||
title,
|
||||
message,
|
||||
danger ? DialogSeverity.Destructive : DialogSeverity.Normal,
|
||||
confirmLabel);
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
{
|
||||
EnsureNoActiveDialog();
|
||||
var tcs = new TaskCompletionSource<object?>(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
@@ -44,7 +52,7 @@ public class DialogService : IDialogService
|
||||
title,
|
||||
DialogKind.Confirm,
|
||||
message,
|
||||
danger,
|
||||
severity,
|
||||
PromptInitial: string.Empty,
|
||||
Placeholder: null,
|
||||
ConfirmLabel: confirmLabel);
|
||||
@@ -58,7 +66,7 @@ public class DialogService : IDialogService
|
||||
EnsureNoActiveDialog();
|
||||
var tcs = new TaskCompletionSource<object?>(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
_tcs = tcs;
|
||||
Current = new DialogState(title, DialogKind.Prompt, label, Danger: false, PromptInitial: initialValue, Placeholder: placeholder);
|
||||
Current = new DialogState(title, DialogKind.Prompt, label, DialogSeverity.Normal, PromptInitial: initialValue, Placeholder: placeholder);
|
||||
OnChange?.Invoke();
|
||||
return Project(tcs.Task, static r => r as string);
|
||||
}
|
||||
@@ -85,7 +93,7 @@ public class DialogService : IDialogService
|
||||
title,
|
||||
DialogKind.Custom,
|
||||
Body: string.Empty,
|
||||
Danger: false,
|
||||
DialogSeverity.Normal,
|
||||
PromptInitial: string.Empty,
|
||||
Placeholder: null,
|
||||
Content: content,
|
||||
@@ -146,7 +154,9 @@ public class DialogService : IDialogService
|
||||
/// <param name="Title">Modal title text.</param>
|
||||
/// <param name="Kind">Discriminates between confirm, prompt, and custom rendering.</param>
|
||||
/// <param name="Body">For confirm: the message; for prompt: the input label; ignored for custom.</param>
|
||||
/// <param name="Danger">When true, the confirm button uses danger styling.</param>
|
||||
/// <param name="Severity">Drives the confirm button's styling, and the verb the host
|
||||
/// falls back to when <paramref name="ConfirmLabel"/> is blank. Single source of truth
|
||||
/// for both — there is deliberately no separate boolean, so the two cannot disagree.</param>
|
||||
/// <param name="PromptInitial">Initial value for prompt-kind dialogs.</param>
|
||||
/// <param name="Placeholder">Placeholder shown when the prompt input is empty.</param>
|
||||
/// <param name="Content">For <see cref="DialogKind.Custom"/>: the body fragment to
|
||||
@@ -160,7 +170,7 @@ public record DialogState(
|
||||
string Title,
|
||||
DialogKind Kind,
|
||||
string Body,
|
||||
bool Danger,
|
||||
DialogSeverity Severity,
|
||||
string PromptInitial,
|
||||
string? Placeholder,
|
||||
RenderFragment? Content = null,
|
||||
@@ -173,3 +183,39 @@ public enum DialogKind
|
||||
Prompt,
|
||||
Custom
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// How consequential the confirmed action is — drives the confirm button's colour.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Deferred-work register row 75. The dialog surface used to carry a two-state
|
||||
/// <c>bool danger</c>, which forced every consequential-but-not-destructive action to
|
||||
/// choose between looking like a delete and looking like the safe default. The Secured
|
||||
/// Writes page is where that bit them: <em>Approve</em> and <em>Reject</em> both rendered
|
||||
/// as identical red buttons, so at the moment of a two-person-control decision the colour
|
||||
/// carried no discriminating information at all — and the affirmative half of the control
|
||||
/// was painted with the delete affordance.
|
||||
/// <para>
|
||||
/// <see cref="Caution"/> is the middle rung that was missing. It is for actions that are
|
||||
/// consequential and worth a beat of hesitation, but are not destroying anything: the
|
||||
/// canonical case is a write to live plant equipment.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public enum DialogSeverity
|
||||
{
|
||||
/// <summary>Ordinary confirmation — renders <c>btn-primary</c>.</summary>
|
||||
Normal,
|
||||
|
||||
/// <summary>
|
||||
/// Consequential but not destructive — renders <c>btn-warning</c>. Does NOT inherit
|
||||
/// the "Delete" label fallback; a caution dialog with no explicit label reads
|
||||
/// "Confirm".
|
||||
/// </summary>
|
||||
Caution,
|
||||
|
||||
/// <summary>
|
||||
/// Destructive or irreversible — renders <c>btn-danger</c>, and is the only severity
|
||||
/// that falls back to the "Delete" verb (see <c>DialogHost.ConfirmLabel</c>).
|
||||
/// </summary>
|
||||
Destructive
|
||||
}
|
||||
|
||||
@@ -20,7 +20,10 @@ public interface IDialogService
|
||||
/// <param name="message">Body text shown to the user.</param>
|
||||
/// <param name="danger">When <c>true</c>, the confirm button renders in
|
||||
/// <c>btn-danger</c> styling; otherwise a primary button is shown. This
|
||||
/// controls <em>styling only</em> — it does not name the action.</param>
|
||||
/// controls <em>styling only</em> — it does not name the action. For an action that
|
||||
/// is consequential but NOT destructive, use the <see cref="DialogSeverity"/>
|
||||
/// overload and pass <see cref="DialogSeverity.Caution"/> rather than reaching for
|
||||
/// red (deferred-work register row 75).</param>
|
||||
/// <param name="confirmLabel">Verb rendered on the confirm button. When
|
||||
/// <c>null</c> or blank the button falls back to "Delete" for a danger
|
||||
/// dialog and "Confirm" otherwise. <strong>Any destructive action that is
|
||||
@@ -31,6 +34,35 @@ public interface IDialogService
|
||||
/// <returns>A task that resolves to <c>true</c> when the user confirms, or <c>false</c> when cancelled.</returns>
|
||||
Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null);
|
||||
|
||||
/// <summary>
|
||||
/// Shows a confirmation dialog with an explicit <see cref="DialogSeverity"/>, for
|
||||
/// actions the <c>danger</c> boolean cannot describe.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Deferred-work register row 75. This is an ADDITIVE overload, deliberately: the
|
||||
/// boolean form stays the signature for the ~34 existing call sites, all of which are
|
||||
/// genuinely two-state. Changing the boolean's type instead would have rippled into
|
||||
/// pages this change has no business touching, and adding a fifth optional parameter
|
||||
/// to the boolean form would have silently un-matched every existing four-argument
|
||||
/// NSubstitute stub — the exact gotcha register row 28 recorded (a stub that stops
|
||||
/// matching returns a null Task and the caller NREs, with no compile error). A
|
||||
/// separate overload cannot do either.
|
||||
/// <para>
|
||||
/// Reach for this only when <see cref="DialogSeverity.Caution"/> is what you mean.
|
||||
/// For Normal / Destructive the boolean form says the same thing more briefly.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="title">Modal title text.</param>
|
||||
/// <param name="message">Body text shown to the user.</param>
|
||||
/// <param name="severity">How consequential the action is; selects the confirm
|
||||
/// button's styling.</param>
|
||||
/// <param name="confirmLabel">Verb rendered on the confirm button. Only
|
||||
/// <see cref="DialogSeverity.Destructive"/> falls back to "Delete" when this is
|
||||
/// blank; the other severities fall back to "Confirm". The row-28 rule still
|
||||
/// applies — name the action.</param>
|
||||
/// <returns>A task that resolves to <c>true</c> when the user confirms, or <c>false</c> when cancelled.</returns>
|
||||
Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null);
|
||||
|
||||
/// <summary>
|
||||
/// Shows a single-line text prompt and resolves to the entered value, or
|
||||
/// <c>null</c> if the user cancels.
|
||||
|
||||
@@ -45,8 +45,14 @@ public class SecuredWritesTests : BunitContext
|
||||
Services.AddSingleton(_browse);
|
||||
|
||||
// Confirm dialogs resolve to "yes" without a DialogHost in the render scope.
|
||||
// BOTH overloads must be stubbed: Reject takes the bool form, Approve takes the
|
||||
// DialogSeverity form (register row 75). An unstubbed NSubstitute call returns a
|
||||
// null Task, so missing one here surfaces as an NRE inside the click handler
|
||||
// rather than as anything that names the dialog — the row-28 gotcha.
|
||||
_dialog.ConfirmAsync(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<bool>(), Arg.Any<string?>())
|
||||
.Returns(Task.FromResult(true));
|
||||
_dialog.ConfirmAsync(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<DialogSeverity>(), Arg.Any<string?>())
|
||||
.Returns(Task.FromResult(true));
|
||||
|
||||
// Empty list by default; individual tests seed the queue/history.
|
||||
_service.ListAsync(
|
||||
@@ -303,10 +309,60 @@ public class SecuredWritesTests : BunitContext
|
||||
_dialog.Received(1).ConfirmAsync(
|
||||
Arg.Any<string>(),
|
||||
Arg.Is<string>(m => Regex.IsMatch(m, "Submitted .* ago")),
|
||||
Arg.Any<bool>(),
|
||||
Arg.Any<DialogSeverity>(),
|
||||
Arg.Any<string?>());
|
||||
}
|
||||
|
||||
// ── (f) Approve/Reject confirm-button severity (register row 75) ──
|
||||
|
||||
[Fact]
|
||||
public void Approve_ConfirmDialog_IsCautionNotDestructive()
|
||||
{
|
||||
// Row 75. Approving executes a real write to live plant equipment, so it is not a
|
||||
// benign primary action — but it is not a delete either, and red made it both
|
||||
// look like one AND look identical to the Reject button beside it, leaving colour
|
||||
// carrying no information at the moment of a two-person-control decision.
|
||||
AddAuth("verifier-user", Roles.Verifier);
|
||||
SeedSites(new Site("Plant-A", "plant-a") { Id = 1 });
|
||||
SeedList(new[] { Dto(11, "Pending", "someone-else") });
|
||||
_service.ApproveAsync(Arg.Any<long>(), Arg.Any<string?>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Task.FromResult(SecuredWriteActionResult.Ok(Dto(11, "Executed", "someone-else", "verifier-user"))));
|
||||
|
||||
var cut = RenderPage();
|
||||
cut.WaitForAssertion(() => Assert.NotNull(cut.Find("[data-test='secured-write-approve']")));
|
||||
cut.Find("[data-test='secured-write-approve']").Click();
|
||||
|
||||
_dialog.Received(1).ConfirmAsync(
|
||||
Arg.Any<string>(), Arg.Any<string>(), DialogSeverity.Caution, "Approve");
|
||||
|
||||
// Negative control: it must not ALSO have gone through the boolean form, which is
|
||||
// the one that renders red. Without this the assertion above would still pass if
|
||||
// the call site had been left on danger:true and merely gained a severity call.
|
||||
_dialog.DidNotReceive().ConfirmAsync(
|
||||
Arg.Any<string>(), Arg.Any<string>(), Arg.Any<bool>(), Arg.Any<string?>());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Reject_ConfirmDialog_KeepsDestructiveStyling()
|
||||
{
|
||||
// Row 75 decided Reject KEEPS red: it is terminal and irreversible for the request
|
||||
// (the Operator must resubmit), and holding it red is what keeps the two opposite
|
||||
// decisions visually distinct now that Approve is amber. Pinned so a later
|
||||
// "make the page less alarming" pass cannot quietly flatten both to one colour.
|
||||
AddAuth("verifier-user", Roles.Verifier);
|
||||
SeedSites(new Site("Plant-A", "plant-a") { Id = 1 });
|
||||
SeedList(new[] { Dto(12, "Pending", "someone-else") });
|
||||
_service.RejectAsync(Arg.Any<long>(), Arg.Any<string?>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Task.FromResult(SecuredWriteActionResult.Ok(Dto(12, "Rejected", "someone-else", "verifier-user"))));
|
||||
|
||||
var cut = RenderPage();
|
||||
cut.WaitForAssertion(() => Assert.NotNull(cut.Find("[data-test='secured-write-reject']")));
|
||||
cut.Find("[data-test='secured-write-reject']").Click();
|
||||
|
||||
_dialog.Received(1).ConfirmAsync(
|
||||
Arg.Any<string>(), Arg.Any<string>(), danger: true, confirmLabel: "Reject");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Viewer_DoesNotSeeOperatorOrVerifierRegions()
|
||||
{
|
||||
|
||||
@@ -213,6 +213,12 @@ public class SchemaLibraryPageTests : BunitContext
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
// Register row 75 added a DialogSeverity overload. This stub auto-confirms
|
||||
// regardless of severity — it exists to let action paths run end to end, not to
|
||||
// assert anything about styling.
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
public Task<string?> PromptAsync(
|
||||
string title, string label, string initialValue = "", string? placeholder = null)
|
||||
=> Task.FromResult<string?>(null);
|
||||
|
||||
@@ -194,6 +194,12 @@ public class NotificationListsPageTests : BunitContext
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
// Register row 75 added a DialogSeverity overload. This stub auto-confirms
|
||||
// regardless of severity — it exists to let action paths run end to end, not to
|
||||
// assert anything about styling.
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
public Task<string?> PromptAsync(
|
||||
string title, string label, string initialValue = "", string? placeholder = null)
|
||||
=> Task.FromResult<string?>(null);
|
||||
|
||||
+6
@@ -339,6 +339,12 @@ public class NotificationReportDetailModalTests : BunitContext
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
// Register row 75 added a DialogSeverity overload. This stub auto-confirms
|
||||
// regardless of severity — it exists to let action paths run end to end, not to
|
||||
// assert anything about styling.
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
public Task<string?> PromptAsync(
|
||||
string title, string label, string initialValue = "", string? placeholder = null)
|
||||
=> Task.FromResult<string?>(null);
|
||||
|
||||
@@ -274,6 +274,12 @@ public class NotificationReportPageTests : BunitContext
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
// Register row 75 added a DialogSeverity overload. This stub auto-confirms
|
||||
// regardless of severity — it exists to let action paths run end to end, not to
|
||||
// assert anything about styling.
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
public Task<string?> PromptAsync(
|
||||
string title, string label, string initialValue = "", string? placeholder = null)
|
||||
=> Task.FromResult<string?>(null);
|
||||
|
||||
@@ -213,6 +213,12 @@ public sealed class QueryStringDrillInTests
|
||||
{
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
// Register row 75 added a DialogSeverity overload. This stub auto-confirms
|
||||
// regardless of severity — it exists to let action paths run end to end, not to
|
||||
// assert anything about styling.
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
public Task<string?> PromptAsync(string title, string label, string initialValue = "", string? placeholder = null)
|
||||
=> Task.FromResult<string?>(null);
|
||||
|
||||
|
||||
@@ -682,6 +682,12 @@ public class SiteCallsReportPageTests : BunitContext
|
||||
public Task<bool> ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
// Register row 75 added a DialogSeverity overload. This stub auto-confirms
|
||||
// regardless of severity — it exists to let action paths run end to end, not to
|
||||
// assert anything about styling.
|
||||
public Task<bool> ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null)
|
||||
=> Task.FromResult(true);
|
||||
|
||||
public Task<string?> PromptAsync(
|
||||
string title, string label, string initialValue = "", string? placeholder = null)
|
||||
=> Task.FromResult<string?>(null);
|
||||
|
||||
@@ -0,0 +1,123 @@
|
||||
using Bunit;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using ZB.MOM.WW.ScadaBridge.CentralUI.Components.Shared;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Shared;
|
||||
|
||||
/// <summary>
|
||||
/// Deferred-work register row 75 — pins the confirm button's severity → Bootstrap class
|
||||
/// mapping against the REAL <see cref="DialogService"/> behind a real <c>DialogHost</c>,
|
||||
/// so these cover what the user actually sees rather than what an argument was.
|
||||
///
|
||||
/// <para>
|
||||
/// Row 28 split the label from the styling but left the styling a two-state boolean, so a
|
||||
/// consequential-but-not-destructive action had to choose between looking like a delete
|
||||
/// and looking like the safe default. Row 75 added <see cref="DialogSeverity.Caution"/> as
|
||||
/// the middle rung. The three-way mapping is the whole of that change, and these are its
|
||||
/// pins; <c>SecuredWritesTests</c> separately pins that the Secured Writes page asks for
|
||||
/// the right one.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// bUnit cannot run real JS interop, so the host's <c>sbDialog.*</c> focus-trap calls run
|
||||
/// in <see cref="JSRuntimeMode.Loose"/> as no-ops (same arrangement as
|
||||
/// <c>FailoverConfirmLabelTests</c>).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public class DialogSeverityStylingTests : BunitContext
|
||||
{
|
||||
private readonly DialogService _dialog = new();
|
||||
|
||||
public DialogSeverityStylingTests()
|
||||
{
|
||||
Services.AddSingleton<IDialogService>(_dialog);
|
||||
JSInterop.Mode = JSRuntimeMode.Loose;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Opens a confirm dialog on a live host and returns its confirm button.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The <c>ConfirmAsync</c> task is deliberately not awaited — with the real service it
|
||||
/// does not complete until someone answers the dialog, so awaiting it would deadlock
|
||||
/// the test. The state is published synchronously before the task is returned.
|
||||
/// </remarks>
|
||||
private AngleSharp.Dom.IElement OpenConfirm(Func<IDialogService, Task<bool>> open)
|
||||
{
|
||||
var cut = Render<DialogHost>();
|
||||
cut.InvokeAsync(() => { _ = open(_dialog); }).GetAwaiter().GetResult();
|
||||
cut.Render();
|
||||
|
||||
var footer = cut.FindAll("div.modal-footer button").ToList();
|
||||
return footer.Single(b => b.TextContent.Trim() != "Cancel");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Caution_rendersAmber_notRedAndNotPrimary()
|
||||
{
|
||||
// The row-75 case. Amber is the point: not red (which reads "delete"), and not
|
||||
// the primary blue (which reads "safe default" — wrong for a live device write).
|
||||
var confirm = OpenConfirm(d => d.ConfirmAsync("Approve thing", "Sure?", DialogSeverity.Caution, "Approve"));
|
||||
|
||||
Assert.Contains("btn-warning", confirm.ClassList);
|
||||
Assert.DoesNotContain("btn-danger", confirm.ClassList);
|
||||
Assert.DoesNotContain("btn-primary", confirm.ClassList);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Destructive_stillRendersRed()
|
||||
{
|
||||
// Regression guard on the ~20 genuine delete call sites: row 75 is additive and
|
||||
// must not have shifted what danger looks like.
|
||||
var confirm = OpenConfirm(d => d.ConfirmAsync("Delete thing", "Sure?", DialogSeverity.Destructive));
|
||||
|
||||
Assert.Contains("btn-danger", confirm.ClassList);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Normal_stillRendersPrimary()
|
||||
{
|
||||
var confirm = OpenConfirm(d => d.ConfirmAsync("Do thing", "Sure?", DialogSeverity.Normal));
|
||||
|
||||
Assert.Contains("btn-primary", confirm.ClassList);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(true, "btn-danger")]
|
||||
[InlineData(false, "btn-primary")]
|
||||
public void BooleanForm_mapsOntoTheSameTwoSeverities(bool danger, string expectedClass)
|
||||
{
|
||||
// The boolean overload is now a thin adapter onto the severity one. Every existing
|
||||
// call site goes through it, so this pins that the adapter did not shift any of
|
||||
// them: true is still red, false is still primary.
|
||||
//
|
||||
// One case per fact, deliberately: DialogService permits only one open dialog at a
|
||||
// time and throws on the second, so two OpenConfirm calls in one body would fail
|
||||
// on the service's own nested-dialog guard rather than on anything about styling.
|
||||
var confirm = OpenConfirm(d => d.ConfirmAsync("Do thing", "Sure?", danger: danger));
|
||||
|
||||
Assert.Contains(expectedClass, confirm.ClassList);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void UnlabelledCaution_readsConfirm_notDelete()
|
||||
{
|
||||
// Caution deliberately does NOT inherit the danger "Delete" fallback: a caution
|
||||
// action is by definition not a delete, so falling back to that verb would
|
||||
// reintroduce register row 28 through the new door.
|
||||
var confirm = OpenConfirm(d => d.ConfirmAsync("Approve thing", "Sure?", DialogSeverity.Caution));
|
||||
|
||||
Assert.Equal("Confirm", confirm.TextContent.Trim());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void UnlabelledDestructive_stillFallsBackToDelete()
|
||||
{
|
||||
// The row-28 fallback is retained deliberately (the Playwright suites click the
|
||||
// literal "Delete"). Pinned here alongside its new sibling so the pair is read
|
||||
// together.
|
||||
var confirm = OpenConfirm(d => d.ConfirmAsync("Delete thing", "Sure?", DialogSeverity.Destructive));
|
||||
|
||||
Assert.Equal("Delete", confirm.TextContent.Trim());
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,146 @@
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using ZB.MOM.WW.ScadaBridge.SiteEventLogging;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Deferred-work register row 62 — hardening pin, not a bug fix.
|
||||
///
|
||||
/// <para>
|
||||
/// Six SiteRuntime call sites reach <c>ISiteEventLogger</c> straight off the injected
|
||||
/// <see cref="IServiceProvider"/>, with no scope in between:
|
||||
/// </para>
|
||||
/// <list type="bullet">
|
||||
/// <item><description><c>SiteRuntime/Actors/AlarmActor.cs</c> — resolves once in the
|
||||
/// constructor and CACHES it in <c>_siteEventLogger</c>, so it holds the instance for
|
||||
/// the actor's whole lifetime.</description></item>
|
||||
/// <item><description><c>SiteRuntime/Actors/ScriptActor.cs</c> (two sites) — per-call
|
||||
/// resolve.</description></item>
|
||||
/// <item><description><c>SiteRuntime/Actors/InstanceActor.cs</c> (two sites) — per-call
|
||||
/// resolve.</description></item>
|
||||
/// <item><description><c>SiteRuntime/Actors/NativeAlarmActor.cs</c> — per-call
|
||||
/// resolve.</description></item>
|
||||
/// <item><description><c>SiteRuntime/Actors/DeploymentManagerActor.cs</c> — per-call
|
||||
/// resolve.</description></item>
|
||||
/// <item><description><c>SiteRuntime/Scripts/ScriptRunLauncher.cs</c> — per-run resolve.
|
||||
/// Not among the five the register row named: it is the run body WP3.1 lifted verbatim
|
||||
/// out of the deleted ScriptExecutionActor/AlarmExecutionActor, so it inherited the
|
||||
/// pattern rather than adding it.</description></item>
|
||||
/// </list>
|
||||
///
|
||||
/// <para>
|
||||
/// Every one of those is CORRECT TODAY, and this test is the reason it stays that way.
|
||||
/// They are correct for exactly one reason: <c>ISiteEventLogger</c> is registered
|
||||
/// <see cref="ServiceLifetime.Singleton"/>, so resolving it from the root provider yields
|
||||
/// the same instance a scope would. The day that registration changes lifetime, every one
|
||||
/// of those call sites silently becomes register row 45 — a captive dependency /
|
||||
/// resolved-outside-its-scope bug — and nothing else in the tree would notice:
|
||||
/// </para>
|
||||
/// <list type="bullet">
|
||||
/// <item><description><c>ValidateScopes</c> catches a SCOPED service resolved from the
|
||||
/// root provider — but the actors hold the root provider legitimately, and .NET's
|
||||
/// check fires at the offending <c>GetService</c> call, i.e. at runtime on a site
|
||||
/// node, on whatever code path happens to log an event first. Not at boot, and not
|
||||
/// in CI.</description></item>
|
||||
/// <item><description><c>ValidateOnBuild</c> walks CONSTRUCTOR graphs. These are
|
||||
/// imperative <c>GetService</c> calls inside method bodies, invisible to it.</description></item>
|
||||
/// <item><description><c>CaptiveDependencyGuard</c> catches the AlarmActor field case
|
||||
/// (a cached resolve) by design, but cannot see the five per-call resolves at
|
||||
/// all.</description></item>
|
||||
/// </list>
|
||||
///
|
||||
/// <para>
|
||||
/// So the lifetime is a load-bearing invariant with no other enforcement. Pinning it here
|
||||
/// means a future lifetime change fails loudly, in CI, with a message naming the call
|
||||
/// sites that have to be fixed alongside it — rather than shipping and being found on the
|
||||
/// rig months later, which is how row 45 was found.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// This deliberately asserts the REGISTRATION, not the call sites: the SiteRuntime actor
|
||||
/// files are correct as written and need no change. The pin is a tripwire on the
|
||||
/// assumption they rest on.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public class SiteEventLoggerLifetimePinTests
|
||||
{
|
||||
/// <summary>
|
||||
/// The failure message is the whole point of the test — it has to carry the reader
|
||||
/// from "a lifetime assertion failed" to "here is what breaks and why".
|
||||
/// </summary>
|
||||
private const string Why =
|
||||
"ISiteEventLogger MUST be registered Singleton in the site service graph " +
|
||||
"(deferred-work register row 62).\n" +
|
||||
"Six SiteRuntime call sites resolve it straight off the root IServiceProvider with " +
|
||||
"no scope: AlarmActor (cached in a field for the actor's lifetime), ScriptActor (x2), " +
|
||||
"InstanceActor (x2), NativeAlarmActor, DeploymentManagerActor, and " +
|
||||
"Scripts/ScriptRunLauncher.\n" +
|
||||
"Under any non-singleton lifetime every one of those becomes a captive-dependency / " +
|
||||
"resolved-outside-its-scope bug (register row 45), and NOTHING else catches it: " +
|
||||
"ValidateOnBuild only walks constructor graphs, ValidateScopes fires at runtime on a " +
|
||||
"site node rather than at boot, and CaptiveDependencyGuard sees only the AlarmActor " +
|
||||
"field case.\n" +
|
||||
"If this lifetime change is intended, fix those six call sites to resolve through an " +
|
||||
"explicit scope in the same change, then update this pin.";
|
||||
|
||||
/// <summary>
|
||||
/// Builds the real Site service graph the way <c>Program.cs</c>'s Site branch does and
|
||||
/// asserts the lifetime of the registration the SiteRuntime root-resolves depend on.
|
||||
/// Registration only — the provider is never built, so this costs nothing and touches
|
||||
/// no SQLite file.
|
||||
/// </summary>
|
||||
/// <returns>The configured site service collection.</returns>
|
||||
private static IServiceCollection BuildSiteServiceGraph()
|
||||
{
|
||||
var configuration = new ConfigurationBuilder()
|
||||
.AddInMemoryCollection(new Dictionary<string, string?>
|
||||
{
|
||||
["ScadaBridge:Node:Role"] = "Site",
|
||||
["ScadaBridge:Node:NodeName"] = "node-a",
|
||||
["ScadaBridge:Node:NodeHostname"] = "test-site",
|
||||
["ScadaBridge:Node:SiteId"] = "TestSite",
|
||||
["ScadaBridge:Node:RemotingPort"] = "0",
|
||||
["ScadaBridge:Node:GrpcPort"] = "0",
|
||||
["LocalDb:Path"] = "/tmp/sb-row62-pin-never-opened.db",
|
||||
["AuditLog:SiteWriter:DatabasePath"] = "/tmp/sb-row62-pin-never-opened-audit.db",
|
||||
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
|
||||
["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552",
|
||||
})
|
||||
.Build();
|
||||
|
||||
var services = new ServiceCollection();
|
||||
SiteServiceRegistration.Configure(services, configuration);
|
||||
return services;
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ISiteEventLogger_IsRegisteredSingleton_InTheSiteServiceGraph()
|
||||
{
|
||||
var services = BuildSiteServiceGraph();
|
||||
|
||||
var descriptor = Assert.Single(services, d => d.ServiceType == typeof(ISiteEventLogger));
|
||||
|
||||
Assert.True(descriptor.Lifetime == ServiceLifetime.Singleton,
|
||||
$"{Why}\nActual lifetime: {descriptor.Lifetime}.");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void SiteEventLogger_TheConcreteTypeTheInterfaceForwardsTo_IsAlsoSingleton()
|
||||
{
|
||||
// The interface registration is a FACTORY —
|
||||
// AddSingleton<ISiteEventLogger>(sp => sp.GetRequiredService<SiteEventLogger>())
|
||||
// — so a Singleton lifetime on ISiteEventLogger alone does not prove callers get a
|
||||
// shared instance. Downgrading the concrete SiteEventLogger to Scoped would leave
|
||||
// the assertion above passing while making the singleton factory resolve a scoped
|
||||
// service from the root: the exact bug, with the pin still green. Both halves have
|
||||
// to be pinned for the invariant to actually hold.
|
||||
var services = BuildSiteServiceGraph();
|
||||
|
||||
var descriptor = Assert.Single(services, d => d.ServiceType == typeof(SiteEventLogger));
|
||||
|
||||
Assert.True(descriptor.Lifetime == ServiceLifetime.Singleton,
|
||||
$"{Why}\nISiteEventLogger forwards to the concrete SiteEventLogger, whose actual " +
|
||||
$"lifetime is: {descriptor.Lifetime}.");
|
||||
}
|
||||
}
|
||||
+77
-16
@@ -215,30 +215,39 @@ public sealed class BundleSessionObservabilityTests
|
||||
public async Task EvictionService_RegistersTheGaugeProvider_AndClearsItOnStop()
|
||||
{
|
||||
// The eviction service is Transport's only lifecycle seam, so it owns the
|
||||
// process-global gauge slot. The gauge is read as a delta against a baseline
|
||||
// because the instrument is a process-wide static.
|
||||
// process-global gauge slot. Every assertion here is a DELTA over a scoped action
|
||||
// against this test's own store (register row 74) — see ReadGaugeDelta.
|
||||
var clock = new TestClock(DateTimeOffset.UtcNow);
|
||||
var store = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger<BundleSessionStore>.Instance);
|
||||
var service = new BundleSessionEvictionService(store, NullLogger<BundleSessionEvictionService>.Instance);
|
||||
|
||||
var baseline = ReadGauge();
|
||||
|
||||
await service.StartAsync(CancellationToken.None);
|
||||
try
|
||||
{
|
||||
store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)));
|
||||
store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)));
|
||||
// While registered, this store's opens are what the gauge reports.
|
||||
var whileRegistered = ReadGaugeDelta(() =>
|
||||
{
|
||||
store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)));
|
||||
store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)));
|
||||
});
|
||||
|
||||
Assert.Equal(2, ReadGauge());
|
||||
Assert.Equal(2, whileRegistered);
|
||||
}
|
||||
finally
|
||||
{
|
||||
await service.StopAsync(CancellationToken.None);
|
||||
}
|
||||
|
||||
// After the clear the gauge falls back to reporting 0, not through a dead store.
|
||||
Assert.Equal(0, ReadGauge());
|
||||
Assert.Equal(0, baseline);
|
||||
// After the clear the gauge no longer reads through this store: opening more
|
||||
// sessions on it moves the instrument by nothing. Stated as a delta this is a
|
||||
// STRONGER claim than the old `Assert.Equal(0, ReadGauge())` — that one merely
|
||||
// said "the instrument reports zero", which any unregistered slot satisfies,
|
||||
// and which a foreign registrant would have failed for reasons unrelated to
|
||||
// the clear.
|
||||
var afterClear = ReadGaugeDelta(() =>
|
||||
store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))));
|
||||
|
||||
Assert.Equal(0, afterClear);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
@@ -257,29 +266,81 @@ public sealed class BundleSessionObservabilityTests
|
||||
await newService.StartAsync(CancellationToken.None);
|
||||
try
|
||||
{
|
||||
newStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)));
|
||||
|
||||
// Late stop of the superseded instance.
|
||||
// Late stop of the superseded instance, then prove the NEWER store is still
|
||||
// the one driving the gauge — by moving it and measuring the movement.
|
||||
await oldService.StopAsync(CancellationToken.None);
|
||||
|
||||
Assert.Equal(1, ReadGauge());
|
||||
var newStoreStillDrivesIt = ReadGaugeDelta(() =>
|
||||
newStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))));
|
||||
|
||||
Assert.Equal(1, newStoreStillDrivesIt);
|
||||
|
||||
// Negative control on the same window: the superseded store is detached, so
|
||||
// its opens must not reach the instrument. Without this, a provider slot left
|
||||
// pointing at oldStore would still satisfy the assertion above by coincidence
|
||||
// whenever both stores happened to hold equal counts.
|
||||
var oldStoreIsDetached = ReadGaugeDelta(() =>
|
||||
oldStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))));
|
||||
|
||||
Assert.Equal(0, oldStoreIsDetached);
|
||||
}
|
||||
finally
|
||||
{
|
||||
await newService.StopAsync(CancellationToken.None);
|
||||
}
|
||||
|
||||
Assert.Equal(0, ReadGauge());
|
||||
var afterClear = ReadGaugeDelta(() =>
|
||||
newStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))));
|
||||
|
||||
Assert.Equal(0, afterClear);
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// Helpers
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
/// <summary>
|
||||
/// Runs <paramref name="act"/> between two observations of the open-import-sessions
|
||||
/// gauge and returns how far the instrument MOVED across it.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Register row 74. These tests used to assert absolute gauge values —
|
||||
/// <c>Assert.Equal(0, baseline)</c>, <c>Assert.Equal(2, ReadGauge())</c> — against
|
||||
/// what is a single process-global provider slot
|
||||
/// (<c>SetImportSessionCountProvider</c> / <c>ClearImportSessionCountProvider</c>).
|
||||
/// That is the same shape as the register row 32 flake in
|
||||
/// <c>QueueDepthGaugeTests</c>: correct only for as long as this file is the slot's
|
||||
/// sole registrant, and silently wrong the day a sibling suite writes it. A delta
|
||||
/// carries no assumption about what the slot held on entry, so a non-zero inherited
|
||||
/// baseline cancels out.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Honest residual, and the reason this is a delta rather than a full fix: unlike
|
||||
/// <see cref="ReadRejectionCounter"/> — where every measurement in the window is
|
||||
/// driven synchronously by this test's own <c>Open</c> call, making it genuinely
|
||||
/// immune — a gauge has ONE slot and last-writer-wins. If a concurrent registrant
|
||||
/// overwrote the slot BETWEEN the two reads, no read-side technique could recover;
|
||||
/// that case needs mutual exclusion on the slot (row 32's serialized collection).
|
||||
/// It cannot arise today: this is the only file in the assembly that constructs a
|
||||
/// <c>BundleSessionEvictionService</c>, and test assemblies run in separate
|
||||
/// processes. A second such file should bring the collection treatment with it.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="act">The action to measure across.</param>
|
||||
/// <returns>The gauge's movement while <paramref name="act"/> ran.</returns>
|
||||
private static long ReadGaugeDelta(Action act)
|
||||
{
|
||||
var before = ReadGauge();
|
||||
act();
|
||||
return ReadGauge() - before;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Forces one synchronous observation of the open-import-sessions gauge and returns
|
||||
/// the measured value. Mirrors the MeterListener helper used by the Communication
|
||||
/// and StoreAndForward telemetry suites.
|
||||
/// and StoreAndForward telemetry suites. Private to <see cref="ReadGaugeDelta"/> by
|
||||
/// convention — see its remarks for why no test reads this absolutely.
|
||||
/// </summary>
|
||||
/// <returns>The gauge's current value, or 0 if it never reported.</returns>
|
||||
private static long ReadGauge()
|
||||
|
||||
Reference in New Issue
Block a user