diff --git a/docs/Diagnostics.md b/docs/Diagnostics.md index bec160c..0e886b0 100644 --- a/docs/Diagnostics.md +++ b/docs/Diagnostics.md @@ -101,8 +101,12 @@ correlate a log entry to a specific principal. A scheme word survives only when it is one of the recognized authorization schemes (`Bearer`, `Basic`, `Digest`, `Negotiate`, `NTLM`, `ApiKey`, `Token`). An unrecognized leading word is as likely to be credential material as it is to be a scheme, so it is dropped along with the rest. The key id is -also dropped when it runs longer than 64 characters, which no issued key id does — a long run before -the first `_` is secret material, not an identifier. +also dropped when it runs longer than 64 characters — a long run before the first `_` is more likely to +be secret material than an identifier. Neither key-creation path (`ApiKeyAdminCommandLineParser.IsValidKeyId`, +`DashboardApiKeyManagementService.ValidateKeyId`) enforces a length, so this is a redaction heuristic +rather than a guarantee: operators should keep key ids under 64 characters, or the id stops appearing +in logs and only the `mxgw_[redacted]` shape survives. The direction of the failure is deliberate — +losing an identifier is cheap, logging a secret is not. The parse is span-based (no regex, no `Split` allocation): the value is split once at the first space, and the key id is read up to the first `_` of the remainder. diff --git a/docs/Sessions.md b/docs/Sessions.md index 418a368..072110b 100644 --- a/docs/Sessions.md +++ b/docs/Sessions.md @@ -215,7 +215,9 @@ Splitting the phases moves the TOCTOU re-check earlier, and that is an accepted A close that throws no longer abandons the rest of the selected set: the sweep attempts every selected session, captures the first failure, and rethrows it once the pass is done so `SessionLeaseMonitorHostedService` still logs the sweep failure as before. Because the closes run concurrently, *which* failure surfaces when several fail in one pass is nondeterministic; the log line is the diagnostic, not the identity of the exception. -`ShutdownAsync` drains sessions with the same bounded fan-out and the same per-session catch → `KillWorkerAsync` fallback, with two rules that keep a stop deadline from turning into leaked workers. First, its body is **exception-total** — nothing escapes it — because `Parallel.ForEachAsync` cancels the token handed to the sibling bodies as soon as one body throws, which would abort in-flight graceful shutdowns *and* make their kill fallback fail instantly on the freshly cancelled token. Second, the drain loop is deliberately **not** bound to the caller's `CancellationToken` and the kill fallback runs on `CancellationToken.None`: a cancelled `ParallelOptions` token stops dispatching the remaining sessions entirely, whereas the sequential drain this replaced let every remaining session fail its graceful close fast and still kill its worker. The token is passed to the graceful close instead, so a host stop deadline turns the drain into a kill sweep rather than into a leak. This matters because nothing reattaches to a leaked worker — a restarted gateway terminates orphans (see [Design Decisions](DesignDecisions.md)). +`ShutdownAsync` drains sessions with the same bounded fan-out and the same per-session catch → `KillWorkerAsync` fallback, with two rules that keep a stop deadline from turning into leaked workers. First, its body is **exception-total** — nothing escapes it — because `Parallel.ForEachAsync` cancels the token handed to the sibling bodies as soon as one body throws, which would abort in-flight graceful shutdowns *and* make their kill fallback fail instantly on the freshly cancelled token. Second, the drain loop is deliberately **not** bound to the caller's `CancellationToken` and the kill fallback runs on `CancellationToken.None`: a cancelled `ParallelOptions` token stops dispatching the remaining sessions entirely, so the untried tail would be neither closed nor killed. This **fixes a leak the sequential drain also had**, rather than restoring the sequential drain's behavior — there the kill fallback ran on the caller's already-cancelled token, and `KillWorkerAsync`'s entry `ThrowIfCancellationRequested` threw out of the loop on the very first session, producing zero kills. The token is passed to the graceful close only, so a host stop deadline turns the drain into a kill sweep rather than into a leak. This matters because nothing reattaches to a leaked worker — a restarted gateway terminates orphans (see [Design Decisions](DesignDecisions.md)). + +The asymmetry with `CloseExpiredLeasesAsync` — whose `ParallelOptions` *is* token-bound — is intentional: the sweep is periodic maintenance, so a pass abandoned on cancellation loses nothing permanently (the next pass re-selects, and `ShutdownAsync` backstops it), whereas the shutdown drain is terminal and must not be abandoned partway. **Stranded-`Closing` bound.** A sweep pass that is cancelled after selection leaves its unclosed selections in `Closing` with close already started. `IsFaultedReapableCore` requires `state == Faulted`, so a session selected under `FaultedReason` and stranded this way is not re-selected as faulted; it is swept only when its normal lease expires (up to `MxGateway:Sessions:DefaultLeaseSeconds`, default 1800 s), since `IsLeaseExpiredCore` and `IsDetachGraceExpiredCore` are state-agnostic. This bound is documented rather than closed with a re-selection clause: the sweep's only caller cancels on the host's `stoppingToken`, so the very next thing that runs is `ShutdownAsync`, which drains (or kills) the whole registry — and any worker that still survives that is terminated as an orphan on the next gateway start. Adding a "`Closing` and close-started" re-selection clause would also have to distinguish an abandoned close from one that is merely still in flight, which would weaken the single invariant that makes the parallel close phase safe. diff --git a/docs/plans/2026-08-15-perf-review-remediation.md b/docs/plans/2026-08-15-perf-review-remediation.md index 269625e..5b20b1c 100644 --- a/docs/plans/2026-08-15-perf-review-remediation.md +++ b/docs/plans/2026-08-15-perf-review-remediation.md @@ -617,6 +617,7 @@ Commit anything found: `docs: remediation plan doc sweep` | Control-frame completion coupled to event batch drain | Documented, bounded (≤128 frames) behavior of the two-class writer design; revisit only if heartbeat latency shows up in metrics. | | Blazor pages' loopback SignalR hop | Works correctly; in-process `WatchSnapshotsAsync` consumption is a dashboard refactor with payoff only at viewer counts the product doesn't target. | | Event-path triple async-iterator flattening | LOW-rated; touches the most invariant-dense code in the gateway for two `MoveNextAsync` hops per event. Reconsider after Tasks 3/4 land and if profiling still shows it. | +| `SessionEventDistributor._subscribers` `ConcurrentDictionary` → plain `Dictionary` (Task 25 / Task 3 review) | Every mutation is already inside `_lifecycleLock`, so the concurrent type buys nothing. Behavior-neutral refactor with no measurable win, proposed after the windev gate was already green — not worth re-running the verification matrix for. Comment cleanups from the same review landed; this swap did not. | ## Execution notes for the orchestrator diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs index 0607dc3..ac66b00 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs @@ -91,6 +91,11 @@ public sealed class EventsHubViewerRegistry return; } + // Detaching the set is safe against a SubscribeSession that arrives after the disconnect + // only because SignalR dispatches a connection's hub invocations sequentially by default + // (MaximumParallelInvocationsPerClient = 1): OnDisconnectedAsync cannot overlap an + // AddViewer for the same connection, so no late add can re-create the entry and leak a + // count that nothing will ever release. Raising that option would break this. if (!_sessionsByConnection.TryRemove(connectionId, out ConcurrentDictionary? sessions)) { return; diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index c4380b2..fa77d2c 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -101,6 +101,12 @@ public sealed class MxAccessGatewayService( try { requestValidator.ValidateInvoke(request); + + // PERF(followup): this resolve and the sessionManager.InvokeAsync below look the same + // session up twice (a dictionary hit each, so measured cost is negligible). Collapsing + // them needs a SessionManager overload taking an already-resolved GatewaySession, which + // would duplicate InvokeAsync's fault mapping (SessionNotFound / state checks / metrics) + // at a second entry point — deliberately not worth it until a profile says otherwise. GatewaySession session = ResolveSession(request.SessionId); MxCommand command = request.Command; BulkConstraintPlan? bulkConstraintPlan = await ApplyConstraintsAsync( @@ -697,9 +703,13 @@ public sealed class MxAccessGatewayService( default: // Only the four bulk-write kinds above reach FilterWriteBulkAsync, so this is - // unreachable; keep the previous behaviour (the unmodified command) rather than - // emitting a payload-less one if that ever stops holding. - return command.Clone(); + // unreachable. It throws rather than falling back to the unmodified command, + // because that fallback failed OPEN: a fifth bulk-write kind added upstream + // without a case here would silently ship the DENIED entries to the worker while + // still reporting them denied to the caller. Failing loud on a kind nobody can + // reach today is strictly safer than a constraint bypass nobody would notice. + throw new UnreachableException( + $"Command kind {command.Kind} reached bulk-write constraint filtering without a filter case."); } return filtered; diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/AuditDrainService.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/AuditDrainService.cs index d4519fb..0d9570f 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/AuditDrainService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/AuditDrainService.cs @@ -246,6 +246,13 @@ public sealed class AuditDrainService( finally { writer.DetachDrain(); + + // Detaching alone leaves a racer that already passed the attached check enqueueing into + // a channel this loop will never read again — those events would sit in the buffer until + // StopAsync's final drain. Completing the writer as well makes that racer's TryWrite + // return false, which is the write-through branch, so the event reaches the store now. + // TryComplete is idempotent, so StopAsync's own CompleteWriting stays safe either way. + writer.CompleteWriting(); } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/ChannelAuditWriter.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/ChannelAuditWriter.cs index d85c5a2..68be6ae 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/ChannelAuditWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/ChannelAuditWriter.cs @@ -87,8 +87,11 @@ public sealed class ChannelAuditWriter : IAuditWriter public void DetachDrain() => Volatile.Write(ref _drainAttached, 0); /// - /// Enqueues a canonical audit event for the drain to persist. Never blocks, never throws, - /// and never touches the store on the caller's thread while a drain is attached. + /// Enqueues a canonical audit event for the drain to persist. Never blocks and never throws. + /// It also never touches the store on the caller's thread while a drain is attached — with one + /// exception: once the channel has been completed (shutdown, or a drain loop that died), the + /// enqueue fails and this falls through to the synchronous write, which is what keeps the event + /// rather than stranding it in a buffer nobody reads. /// /// The canonical audit event to persist. /// Token honoured only by the direct write-through path. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/SqliteCanonicalAuditStore.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/SqliteCanonicalAuditStore.cs index 3ad7f91..67f4bb8 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/SqliteCanonicalAuditStore.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Audit/SqliteCanonicalAuditStore.cs @@ -31,7 +31,14 @@ namespace ZB.MOM.WW.MxGateway.Server.Security.Audit; /// retries. /// /// -public sealed class SqliteCanonicalAuditStore(AuthSqliteConnectionFactory connectionFactory) : IAuditEventSink +/// Factory for connections to the shared auth database file. +/// +/// Optional logger for row-level read diagnostics. Optional because the store is also constructed +/// directly by the apikey CLI path and by DI-free tests, which have no logger to hand. +/// +public sealed class SqliteCanonicalAuditStore( + AuthSqliteConnectionFactory connectionFactory, + ILogger? logger = null) : IAuditEventSink { private const string CreateTableSql = """ @@ -208,7 +215,7 @@ public sealed class SqliteCanonicalAuditStore(AuthSqliteConnectionFactory connec events.Add(new AuditEvent { EventId = Guid.Parse(reader.GetString(0)), - OccurredAtUtc = ParseUtc(reader.GetString(1)), + OccurredAtUtc = ParseUtcOrMinValue(reader.GetString(1), reader.GetString(0)), Actor = reader.GetString(2), Action = reader.GetString(3), Outcome = Enum.Parse(reader.GetString(4)), @@ -239,6 +246,26 @@ public sealed class SqliteCanonicalAuditStore(AuthSqliteConnectionFactory connec Volatile.Write(ref _tableEnsured, 1); } - private static DateTimeOffset ParseUtc(string value) => - DateTimeOffset.Parse(value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind); + // Reading is defensive where writing is not: an insert always round-trips "O", but the table is + // append-only shared state that an operator (or a future migration) can put an unparseable + // timestamp into, and the retention sweep deliberately keeps such a row — SQLite's datetime() + // yields NULL for it, so the DELETE's comparison is never true. A throwing Parse here would let + // that single row take out the dashboard's whole recent-audit view. MinValue instead sorts the + // row to the far past and keeps every other column readable, which is what an operator looking + // at the view actually needs. + private DateTimeOffset ParseUtcOrMinValue(string value, string eventId) + { + if (DateTimeOffset.TryParse(value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out DateTimeOffset parsed)) + { + return parsed; + } + + // Debug, not warning: the row is still returned and the timestamp text itself is not logged + // (audit rows are not secrets, but the value is attacker-influenceable in the worst case). + logger?.LogDebug( + "Audit event {EventId} has an unparseable occurred_at_utc; reporting it as DateTimeOffset.MinValue.", + eventId); + + return DateTimeOffset.MinValue; + } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/AuthStoreServiceCollectionExtensions.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/AuthStoreServiceCollectionExtensions.cs index 6722fa6..e799758 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/AuthStoreServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/AuthStoreServiceCollectionExtensions.cs @@ -97,8 +97,13 @@ public static class AuthStoreServiceCollectionExtensions sp.GetService() ?? TimeProvider.System)); DecorateVerifierWithCache(services, security); + // GetService, not GetRequiredService, for the same reason the writer registration below + // gives: the DI-only unit tests build a bare ServiceCollection with no AddLogging(). The + // store's logger is optional and only carries row-level read diagnostics. services.AddSingleton(sp => - new SqliteCanonicalAuditStore(sp.GetRequiredService())); + new SqliteCanonicalAuditStore( + sp.GetRequiredService(), + sp.GetService>())); services.AddSingleton(sp => sp.GetRequiredService()); // Resolve the logger defensively: the production host always registers ILogger, but the // DI-only auth/CLI/dashboard unit tests build a bare ServiceCollection without AddLogging(). diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/ISessionManager.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/ISessionManager.cs index 232a17c..7a9b749 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/ISessionManager.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/ISessionManager.cs @@ -79,7 +79,14 @@ public interface ISessionManager CancellationToken cancellationToken); /// Shuts down all sessions and the session manager. - /// Token to cancel the asynchronous operation. + /// + /// Token that degrades the drain rather than cancelling it. It is passed only to each + /// session's graceful close; the drain loop and the kill fallback are not bound to it, so + /// cancelling turns the drain into a kill sweep instead of abandoning the untried sessions as + /// leaked workers. The call therefore overruns a cancelled token by a bounded amount — + /// roughly ceil(sessionCount / 4) batches of the worker shutdown timeout in the worst + /// case, where 4 is MaxParallelSessionCloses. + /// /// A task that represents the asynchronous operation. Task ShutdownAsync(CancellationToken cancellationToken); } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs index ba97a03..a7b653a 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs @@ -112,8 +112,11 @@ public sealed class SessionEventDistributor : IAsyncDisposable // inside the _lifecycleLock section of every register/unregister; never mutated in // place, so the pump can walk the array it captured with no lock and no allocation. // Volatile.Write / Volatile.Read ORDER the access — they keep the publishing store from - // sinking past the lock release and keep the pump's read from being hoisted out of the - // fan-out loop. They do NOT promise freshness, and nothing here needs them to: a reader + // sinking past the lock release, and they keep a lock-free reader's load from being hoisted + // or cached. The pump is NOT that reader: its single capture point sits inside the + // _replayLock section of AppendToReplayBufferAndCaptureSubscribers, so the lock edge already + // orders it. The genuinely lock-free reader is SubscriberCount, which loads the field with no + // lock at all. They do NOT promise freshness, and nothing here needs them to: a reader // may legitimately observe the previous array, which IS the documented "late subscribers // see events after they register" window. Where visibility must be guaranteed — the // RegisterWithReplay handoff — it comes from the _replayLock edge, not from Volatile. @@ -700,8 +703,11 @@ public sealed class SessionEventDistributor : IAsyncDisposable // every path that completes a channel during fan-out (lease disposal via Unregister, // and this method) removes the subscriber from the set BEFORE completing it, so a // completed channel implies the subscriber is already gone and RemoveSubscriber - // returns false. (CompleteAllSubscribers completes without removing, but only after - // the pump has left its loop, so it cannot be observed here.) + // returns false. (CompleteAllSubscribers completes without removing, but only after the + // pump has left its loop, so it cannot be observed here — except on the DisposeAsync + // abandon path: a source factory that ignores cancellation past the 5 s shutdown timeout + // leaves the pump fanning while DisposeAsync completes subscribers, so a spurious overflow + // report is possible there. It is harmless, because the session is already being disposed.) // // Bailing out on false is what keeps a normal stream ending mid-traffic from emitting // a bogus EventQueueOverflow metric and — under the default single-subscriber FailFast diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs index 9042aad..e369141 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs @@ -365,11 +365,19 @@ public sealed class SessionManager : ISessionManager // rather than adopting them). // // For the same reason the loop itself is NOT bound to cancellationToken: a cancelled - // ParallelOptions token stops dispatching the remaining sessions entirely, whereas the - // sequential drain this replaced let every remaining session fail its graceful close fast - // and still kill its worker. The token is passed to the graceful close instead, which - // preserves that behavior — a host stop deadline turns the drain into a kill sweep rather - // than into a leak. + // ParallelOptions token stops dispatching the remaining sessions entirely, so a stop + // deadline would leave the untried tail neither closed nor killed. Note this FIXES a leak + // the sequential drain also had rather than restoring its behavior: there the kill fallback + // ran on the caller's cancelled token, and KillWorkerAsync's entry + // ThrowIfCancellationRequested threw out of the loop on the first session — zero kills, not + // "fail fast and still kill". The token is passed to the graceful close only, and the kill + // runs on CancellationToken.None, so a host stop deadline turns the drain into a kill sweep + // rather than into a leak. + // + // The asymmetry with CloseExpiredLeasesAsync (whose ParallelOptions IS token-bound) is + // intentional: that sweep is periodic maintenance whose missed sessions are picked up by + // the next pass and, ultimately, by this drain. This drain is terminal — nothing runs after + // it — so it must not be abandoned partway. await Parallel.ForEachAsync( _registry.Snapshot(), new ParallelOptions { MaxDegreeOfParallelism = MaxParallelSessionCloses }, diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionShutdownHostedService.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionShutdownHostedService.cs index d126dfc..d02fe36 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionShutdownHostedService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionShutdownHostedService.cs @@ -16,7 +16,14 @@ public sealed class SessionShutdownHostedService( return Task.CompletedTask; } - /// Shuts down all gateway sessions as the host stops, logging (without throwing) if the host's shutdown timeout cancels the operation first. + /// Shuts down all gateway sessions as the host stops. + /// + /// The catch below is now effectively unreachable: + /// no longer aborts on the host's shutdown timeout, it degrades to a kill sweep and logs a + /// per-session warning for each session that failed its graceful close. The clause is kept as + /// a cheap guard against that contract regressing, not as an expected path — the operator + /// signal for a timed-out shutdown is now those per-session warnings. + /// /// Token that signals the host's shutdown timeout has elapsed. /// A task that represents the asynchronous operation. public async Task StopAsync(CancellationToken cancellationToken) diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs index 70df716..41df638 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs @@ -50,6 +50,33 @@ public sealed class GatewayLogRedactorTests Assert.DoesNotContain("super-secret", redacted, StringComparison.Ordinal); } + /// + /// Pins the 64-character key-id boundary in both directions. Nothing validates key-id length at + /// creation time (neither ApiKeyAdminCommandLineParser.IsValidKeyId nor + /// DashboardApiKeyManagementService.ValidateKeyId caps it), so the cap here is a redaction + /// heuristic that a real key id can cross. This test fixes which way it fails when it does: at + /// exactly 64 the id is still an identifier and survives; at 65 the whole run is treated as secret + /// material and goes. Losing an identifier is the cheap failure; logging a secret is not. + /// + /// Length of the key id presented before the secret separator. + /// Whether the key id must survive redaction at that length. + [Theory] + [InlineData(64, true)] + [InlineData(65, false)] + public void RedactClientIdentity_KeyIdLengthBoundary_FailsTowardRedaction( + int keyIdLength, + bool expectsKeyIdPreserved) + { + string keyId = new('a', keyIdLength); + + string? redacted = GatewayLogRedactor.RedactClientIdentity($"Bearer mxgw_{keyId}_super-secret"); + + Assert.Equal( + expectsKeyIdPreserved ? $"Bearer mxgw_{keyId}_[redacted]" : "Bearer mxgw_[redacted]", + redacted); + Assert.DoesNotContain("super-secret", redacted, StringComparison.Ordinal); + } + /// /// Verifies that anything not recognized as a gateway API key fails closed: the scheme word /// survives only when it looks like an auth scheme, and the credential never does. diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs index 4bda8ae..ebf4957 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs @@ -120,6 +120,30 @@ public sealed class MxAccessGrpcMapperTests Assert.Equal(ProtocolStatusCode.ProtocolViolation, publicReply.ProtocolStatus.Code); } + /// + /// Verifies MapCommandReply transfers ownership of the inner MxCommandReply the same way + /// MapEvent does: the returned reference is the instance carried by the WorkerCommandReply, + /// not a clone. The WorkerCommandReply is discarded after mapping and the awaiting Invoke + /// call is its single consumer, so moving the inner reply out is safe and avoids a deep copy + /// of a potentially large bulk-read payload. + /// + [Fact] + public void MapCommandReply_TransfersOwnershipOfInnerReplyWithoutCloning() + { + MxCommandReply innerReply = new() + { + SessionId = "session-1", + Kind = MxCommandKind.Register, + ProtocolStatus = MxAccessGrpcMapper.Ok(), + Register = new RegisterReply { ServerHandle = 50 }, + }; + WorkerCommandReply workerReply = new() { Reply = innerReply }; + + MxCommandReply mapped = new MxAccessGrpcMapper().MapCommandReply(workerReply); + + Assert.Same(innerReply, mapped); + } + /// /// Verifies MapEvent transfers ownership of the inner MxEvent (GWC-07 / IPC-05): the /// returned reference is the same instance carried by the WorkerEvent, not a clone. The diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionEventDistributorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionEventDistributorTests.cs index 1338a71..5220ac3 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionEventDistributorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionEventDistributorTests.cs @@ -1066,6 +1066,14 @@ public sealed class SessionEventDistributorTests /// racing the fan-out must never drop, duplicate, or reorder an event for a subscriber /// registered throughout — nor leave the array and the dictionary disagreeing on the /// subscriber count once the churn stops. + /// + /// One assumption worth naming: the snapshot is rebuilt from + /// ConcurrentDictionary.Values, whose bucket order happens to keep the long-lived + /// stable subscriber ahead of the churned ones here, so it is written to before a churned + /// subscriber's disposal can interleave. If that ever stops holding the test does not + /// silently pass — it fails as a expiry on the read below, because + /// an event dropped for the stable subscriber never arrives. + /// /// /// A task that represents the asynchronous operation. [Fact] diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Audit/ChannelAuditWriterTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Audit/ChannelAuditWriterTests.cs index 3b5a273..9f40f2a 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Audit/ChannelAuditWriterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Audit/ChannelAuditWriterTests.cs @@ -219,6 +219,33 @@ public sealed class ChannelAuditWriterTests : IDisposable (await ListActionsAsync(factory)).OrderBy(action => action, StringComparer.Ordinal)); } + /// + /// The sweep above deliberately preserves rows it cannot date, so the dashboard's recent-audit + /// view is guaranteed to meet one eventually. Reading must therefore be defensive: the + /// undateable row is reported at with every other column + /// intact, rather than one bad row throwing the whole page away. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task SqliteStore_ListRecent_WithUnparseableRow_ReportsMinValueInsteadOfThrowing() + { + (SqliteCanonicalAuditStore store, AuthSqliteConnectionFactory factory) = CreateStore(); + + DateTimeOffset occurred = new(2026, 5, 18, 0, 0, 0, TimeSpan.Zero); + await store.InsertAsync(MakeEvent("dateable", occurred), CancellationToken.None); + await InsertRawRowAsync(factory, "undateable", "0000-not-a-timestamp"); + + IReadOnlyList recent = await store.ListRecentAsync(10, CancellationToken.None); + + Assert.Equal(2, recent.Count); + Assert.Equal(occurred, recent.Single(auditEvent => auditEvent.Action == "dateable").OccurredAtUtc); + + AuditEvent undateable = recent.Single(auditEvent => auditEvent.Action == "undateable"); + Assert.Equal(DateTimeOffset.MinValue, undateable.OccurredAtUtc); + Assert.Equal("operator01", undateable.Actor); + Assert.Equal(AuditOutcome.Denied, undateable.Outcome); + } + private static (ChannelAuditWriter Writer, AuditDrainService Drain) CreateWriterAndDrain( IAuditEventSink sink, SecurityOptions? security = null, @@ -265,7 +292,8 @@ public sealed class ChannelAuditWriterTests : IDisposable await command.ExecuteNonQueryAsync(CancellationToken.None); } - // Reads actions straight from SQL: ListRecentAsync would throw on the unparseable timestamp. + // Reads actions straight from SQL so the sweep assertion depends on the DELETE alone, with no + // opinion from the store's read path about how an undateable row is surfaced. private static async Task> ListActionsAsync(AuthSqliteConnectionFactory factory) { await using SqliteConnection connection = await factory.OpenConnectionAsync(CancellationToken.None); diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs index a75046c..49dce31 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs @@ -281,6 +281,55 @@ public sealed class CachingApiKeyVerifierTests Assert.Same(reparsed, MapConstraints(firstJson)); } + /// + /// Stress: the constraint cache's bound is enforced by the inserting thread itself + /// (GetOrAdd, then enqueue, then evict), so concurrent inserters can each land an + /// entry before any of them reaches the eviction step. That overshoot is real but must be + /// transient and proportional to the in-flight inserters — not unbounded growth — and once + /// the churn stops the cache must be back at or under the cap. Hammered on both the + /// converging path (every iteration maps the same blob, so all but one GetOrAdd + /// loses) and the growth path (a distinct blob per iteration, which is what forces + /// eviction). + /// + [Fact] + public void ToGatewayIdentity_ConcurrentBlobs_OvershootIsTransientAndCacheSettlesUnderCap() + { + const int cap = GatewayApiKeyIdentityMapper.MaxCachedConstraintBlobs; + + // Overshoot is bounded by how many inserters can sit between their GetOrAdd and their own + // EvictIfOverCapacity, so scale the allowance with the available parallelism rather than + // pinning a magic number. Generous on purpose: the assertion under test is "bounded", not + // "bounded by exactly this". + int transientAllowance = (Environment.ProcessorCount * 8) + 64; + string sharedJson = ConstraintsJson("Area_StressShared"); + int peak = 0; + + Parallel.For(0, (cap * 2) + 64, index => + { + MapConstraints(sharedJson); + MapConstraints(ConstraintsJson($"Area_Stress_{index}")); + + int size = GatewayApiKeyIdentityMapper.CurrentCacheSize; + int seen = Volatile.Read(ref peak); + while (size > seen && Interlocked.CompareExchange(ref peak, size, seen) != seen) + { + seen = Volatile.Read(ref peak); + } + }); + + Assert.True( + peak <= cap + transientAllowance, + $"cache peaked at {peak} entries, past the {cap} cap plus the {transientAllowance} transient allowance"); + + // The cache is process-wide static and other test classes in this assembly map identities + // too, so poll rather than asserting on the instant the loop returns. + Assert.True( + SpinWait.SpinUntil( + () => GatewayApiKeyIdentityMapper.CurrentCacheSize <= cap, + TimeSpan.FromSeconds(5)), + $"cache settled at {GatewayApiKeyIdentityMapper.CurrentCacheSize} entries, past the {cap} cap"); + } + private static ApiKeyConstraints MapConstraints(string constraintsJson) => GatewayApiKeyIdentityMapper.ToGatewayIdentity(new LibApiKeyIdentity( KeyId: "operator01", diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs index d843b83..cdcdef1 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs @@ -414,6 +414,46 @@ public sealed class ApiKeyFailureLimiterTests Assert.True(limiter.IsTracked(arriving)); } + /// + /// The PartitionResolution handed back by Check is carried across an await (the + /// inner verification) before Reset consumes it, so the partition it names can be evicted + /// in between. Applying a stale resolution must be inert — never throw, and never clear a + /// partition or aggregate that belongs to somebody else. The failure direction that matters is + /// "clears too little", which costs the caller a probe wait; "clears somebody else's block" + /// would be a throttle bypass. + /// + [Fact] + public void Reset_WithStaleResolutionForEvictedPartition_IsInertAndClearsNoOtherBlock() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 3, maxPartitions: 2); + ApiKeyThrottlePartition victim = new("ipv4:10.0.0.1:1", "victim"); + ApiKeyThrottlePartition other = new("ipv4:10.0.0.2:1", "other"); + ApiKeyThrottlePartition arriving = new("ipv4:10.0.0.3:1", "arriving"); + + RecordFailures(limiter, victim, 3); + Assert.Equal( + ApiKeyThrottleDecision.ThrottledByPeer, + limiter.Check(victim, out ApiKeyFailureLimiter.PartitionResolution stale)); + Assert.True(stale.IsResolved); + + // Age the victim's window out, then push the map past its cap so eviction takes it — + // expired windows are the first eviction preference, so this is deterministic. The + // resolution captured above now names a partition that no longer exists. + clock.Advance(Window + TimeSpan.FromSeconds(1)); + RecordFailures(limiter, other, 3); + RecordFailures(limiter, arriving, 3); + Assert.False(limiter.IsTracked(victim)); + + limiter.Reset(victim, stale); + + Assert.True(limiter.IsTracked(other)); + Assert.True(limiter.IsTracked(arriving)); + Assert.Equal(2, limiter.TrackedAggregateCount); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(other)); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(arriving)); + } + private static void RecordFailures(ApiKeyFailureLimiter limiter, ApiKeyThrottlePartition partition, int count) { for (int i = 0; i < count; i++)