diff --git a/docs/deployment/topology-guide.md b/docs/deployment/topology-guide.md index 5320168c..8836cd8c 100644 --- a/docs/deployment/topology-guide.md +++ b/docs/deployment/topology-guide.md @@ -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 diff --git a/docs/requirements/Component-CentralUI.md b/docs/requirements/Component-CentralUI.md index 8fe002eb..c1a65cd1 100644 --- a/docs/requirements/Component-CentralUI.md +++ b/docs/requirements/Component-CentralUI.md @@ -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 ago ()`) 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 ago ()`) 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. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index 5e9a4e38..0555e0dc 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -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. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor index cbe873e3..e1ee95b4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor @@ -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; diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor index 22bac228..ab5bea09 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor @@ -50,7 +50,7 @@
@@ -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", }; } diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs index 3baa15e3..d2d457c7 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs @@ -35,7 +35,15 @@ public class DialogService : IDialogService private TaskCompletionSource? _tcs; /// - public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) + public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => + ConfirmAsync( + title, + message, + danger ? DialogSeverity.Destructive : DialogSeverity.Normal, + confirmLabel); + + /// + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) { EnsureNoActiveDialog(); var tcs = new TaskCompletionSource(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(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 /// Modal title text. /// Discriminates between confirm, prompt, and custom rendering. /// For confirm: the message; for prompt: the input label; ignored for custom. -/// When true, the confirm button uses danger styling. +/// Drives the confirm button's styling, and the verb the host +/// falls back to when is blank. Single source of truth +/// for both — there is deliberately no separate boolean, so the two cannot disagree. /// Initial value for prompt-kind dialogs. /// Placeholder shown when the prompt input is empty. /// For : 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 } + +/// +/// How consequential the confirmed action is — drives the confirm button's colour. +/// +/// +/// Deferred-work register row 75. The dialog surface used to carry a two-state +/// bool danger, 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: 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 +/// was painted with the delete affordance. +/// +/// 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. +/// +/// +public enum DialogSeverity +{ + /// Ordinary confirmation — renders btn-primary. + Normal, + + /// + /// Consequential but not destructive — renders btn-warning. Does NOT inherit + /// the "Delete" label fallback; a caution dialog with no explicit label reads + /// "Confirm". + /// + Caution, + + /// + /// Destructive or irreversible — renders btn-danger, and is the only severity + /// that falls back to the "Delete" verb (see DialogHost.ConfirmLabel). + /// + Destructive +} diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs index 0b90d48e..6a7fd9de 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs @@ -20,7 +20,10 @@ public interface IDialogService /// Body text shown to the user. /// When true, the confirm button renders in /// btn-danger styling; otherwise a primary button is shown. This - /// controls styling only — it does not name the action. + /// controls styling only — it does not name the action. For an action that + /// is consequential but NOT destructive, use the + /// overload and pass rather than reaching for + /// red (deferred-work register row 75). /// Verb rendered on the confirm button. When /// null or blank the button falls back to "Delete" for a danger /// dialog and "Confirm" otherwise. Any destructive action that is @@ -31,6 +34,35 @@ public interface IDialogService /// A task that resolves to true when the user confirms, or false when cancelled. Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null); + /// + /// Shows a confirmation dialog with an explicit , for + /// actions the danger boolean cannot describe. + /// + /// + /// 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. + /// + /// Reach for this only when is what you mean. + /// For Normal / Destructive the boolean form says the same thing more briefly. + /// + /// + /// Modal title text. + /// Body text shown to the user. + /// How consequential the action is; selects the confirm + /// button's styling. + /// Verb rendered on the confirm button. Only + /// falls back to "Delete" when this is + /// blank; the other severities fall back to "Confirm". The row-28 rule still + /// applies — name the action. + /// A task that resolves to true when the user confirms, or false when cancelled. + Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null); + /// /// Shows a single-line text prompt and resolves to the entered value, or /// null if the user cancels. diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs index d57ba929..6dcbf49a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs @@ -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(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(true)); + _dialog.ConfirmAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .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(), Arg.Is(m => Regex.IsMatch(m, "Submitted .* ago")), - Arg.Any(), + Arg.Any(), Arg.Any()); } + // ── (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(), Arg.Any(), Arg.Any()) + .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(), Arg.Any(), 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(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [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(), Arg.Any(), Arg.Any()) + .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(), Arg.Any(), danger: true, confirmLabel: "Reject"); + } + [Fact] public void Viewer_DoesNotSeeOperatorOrVerifierRegions() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs index c9182cf0..2d4ad654 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs @@ -213,6 +213,12 @@ public class SchemaLibraryPageTests : BunitContext public Task 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 ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs index 196f7818..d8efa79f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs @@ -194,6 +194,12 @@ public class NotificationListsPageTests : BunitContext public Task 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 ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs index d1daade5..22947bd3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs @@ -339,6 +339,12 @@ public class NotificationReportDetailModalTests : BunitContext public Task 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 ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs index 7d9fda24..bb6a97b1 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs @@ -274,6 +274,12 @@ public class NotificationReportPageTests : BunitContext public Task 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 ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs index 72521239..de1864aa 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs @@ -213,6 +213,12 @@ public sealed class QueryStringDrillInTests { public Task 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 ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); public Task PromptAsync(string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs index e7629476..71477834 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs @@ -682,6 +682,12 @@ public class SiteCallsReportPageTests : BunitContext public Task 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 ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs new file mode 100644 index 00000000..f48bb9fb --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs @@ -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; + +/// +/// Deferred-work register row 75 — pins the confirm button's severity → Bootstrap class +/// mapping against the REAL behind a real DialogHost, +/// so these cover what the user actually sees rather than what an argument was. +/// +/// +/// 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 as +/// the middle rung. The three-way mapping is the whole of that change, and these are its +/// pins; SecuredWritesTests separately pins that the Secured Writes page asks for +/// the right one. +/// +/// +/// +/// bUnit cannot run real JS interop, so the host's sbDialog.* focus-trap calls run +/// in as no-ops (same arrangement as +/// FailoverConfirmLabelTests). +/// +/// +public class DialogSeverityStylingTests : BunitContext +{ + private readonly DialogService _dialog = new(); + + public DialogSeverityStylingTests() + { + Services.AddSingleton(_dialog); + JSInterop.Mode = JSRuntimeMode.Loose; + } + + /// + /// Opens a confirm dialog on a live host and returns its confirm button. + /// + /// + /// The ConfirmAsync 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. + /// + private AngleSharp.Dom.IElement OpenConfirm(Func> open) + { + var cut = Render(); + 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()); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs new file mode 100644 index 00000000..1f17c8a9 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs @@ -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; + +/// +/// Deferred-work register row 62 — hardening pin, not a bug fix. +/// +/// +/// Six SiteRuntime call sites reach ISiteEventLogger straight off the injected +/// , with no scope in between: +/// +/// +/// SiteRuntime/Actors/AlarmActor.cs — resolves once in the +/// constructor and CACHES it in _siteEventLogger, so it holds the instance for +/// the actor's whole lifetime. +/// SiteRuntime/Actors/ScriptActor.cs (two sites) — per-call +/// resolve. +/// SiteRuntime/Actors/InstanceActor.cs (two sites) — per-call +/// resolve. +/// SiteRuntime/Actors/NativeAlarmActor.cs — per-call +/// resolve. +/// SiteRuntime/Actors/DeploymentManagerActor.cs — per-call +/// resolve. +/// SiteRuntime/Scripts/ScriptRunLauncher.cs — 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. +/// +/// +/// +/// Every one of those is CORRECT TODAY, and this test is the reason it stays that way. +/// They are correct for exactly one reason: ISiteEventLogger is registered +/// , 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: +/// +/// +/// ValidateScopes 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 GetService 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. +/// ValidateOnBuild walks CONSTRUCTOR graphs. These are +/// imperative GetService calls inside method bodies, invisible to it. +/// CaptiveDependencyGuard catches the AlarmActor field case +/// (a cached resolve) by design, but cannot see the five per-call resolves at +/// all. +/// +/// +/// +/// 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. +/// +/// +/// +/// 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. +/// +/// +public class SiteEventLoggerLifetimePinTests +{ + /// + /// 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". + /// + 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."; + + /// + /// Builds the real Site service graph the way Program.cs'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. + /// + /// The configured site service collection. + private static IServiceCollection BuildSiteServiceGraph() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["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(sp => sp.GetRequiredService()) + // — 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}."); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs index ea6d17e2..1c85e604 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs @@ -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.Instance); var service = new BundleSessionEvictionService(store, NullLogger.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 // ------------------------------------------------------------------ + /// + /// Runs between two observations of the open-import-sessions + /// gauge and returns how far the instrument MOVED across it. + /// + /// + /// + /// Register row 74. These tests used to assert absolute gauge values — + /// Assert.Equal(0, baseline), Assert.Equal(2, ReadGauge()) — against + /// what is a single process-global provider slot + /// (SetImportSessionCountProvider / ClearImportSessionCountProvider). + /// That is the same shape as the register row 32 flake in + /// QueueDepthGaugeTests: 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. + /// + /// + /// Honest residual, and the reason this is a delta rather than a full fix: unlike + /// — where every measurement in the window is + /// driven synchronously by this test's own Open 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 + /// BundleSessionEvictionService, and test assemblies run in separate + /// processes. A second such file should bring the collection treatment with it. + /// + /// + /// The action to measure across. + /// The gauge's movement while ran. + private static long ReadGaugeDelta(Action act) + { + var before = ReadGauge(); + act(); + return ReadGauge() - before; + } + /// /// 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 by + /// convention — see its remarks for why no test reads this absolutely. /// /// The gauge's current value, or 0 if it never reported. private static long ReadGauge()