diff --git a/code-reviews/AuditLog/findings.md b/code-reviews/AuditLog/findings.md index ce9b1923..de0c30ac 100644 --- a/code-reviews/AuditLog/findings.md +++ b/code-reviews/AuditLog/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 3 | +| Open findings | 2 | ## Summary @@ -263,7 +263,7 @@ tests already exercise the success path). |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.AuditLog/Site/SqliteAuditWriter.cs:597-657` | **Description** @@ -293,9 +293,20 @@ lazily on a dedicated background tick so the reporter reads a pre-computed snaps without acquiring the write lock. Option (a) also unblocks `ReadPendingAsync` / `ReadPendingSinceAsync` from competing with the writer. -**Resolution** +**Resolution (2026-05-28):** -_Unresolved._ +Took option (a). `SqliteAuditWriter` now opens a second `SqliteConnection` +(`_readConnection`) on the same file in the ctor, after `InitializeSchema` +sets `PRAGMA journal_mode = WAL` on the writer connection — WAL lets a +second connection read concurrently with the active writer without taking +`_writeLock`. The read connection is guarded by its own `_readLock` (since +`SqliteConnection` itself is not thread-safe across callers) and used by +`GetBacklogStatsAsync`, `ReadPendingAsync`, `ReadPendingSinceAsync`, and +`ReadForwardedAsync`. `DisposeAsync` disposes it after the writer drains. +Regression test `SqliteAuditWriterBacklogStatsTests.GetBacklogStatsAsync_DoesNotBlockOnConcurrentWriteLoad` +saturates the writer with a 2 000-row burst and asserts the probe returns +in under 1 s — would fail against the pre-fix code (the probe queued +behind every batch INSERT under `_writeLock`). ### AuditLog-006 — `SqliteAuditWriter.Dispose()` does sync-over-async and may deadlock diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index 909eea65..9a9f95aa 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 3 | ## Summary @@ -888,9 +888,11 @@ and pass after. |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/BundleCommands.cs:117-124`, `src/ScadaLink.CLI/ManagementHttpClient.cs:47-92` | +**Resolution (2026-05-28):** Replaced the `Convert.FromBase64String(...)` + `File.WriteAllBytes(...)` pair with a new `StreamBase64ToFile(base64, output)` helper that slices the base64 string into 4-char-aligned chunks (1 MB by default) and decodes each chunk straight into a `FileStream` via `Convert.TryFromBase64Chars`. The intermediate `byte[]` and the synchronous full-bundle write are gone — peak working set drops from ~base64-string + ~byte[] + ~envelope-string to ~base64-string + small-chunk-buffer + ~envelope-string. The response body is still buffered into the management envelope string (the `POST /management` wire format does not currently support streaming responses — this finding is bounded by that limit per the recommendation's stop-gap), so a streaming `POST /api/bundle/export` REST endpoint remains a follow-up. Regression tests `BundleCommandsStreamingTests` (6 tests) cover small payload round-trip, multi-chunk boundaries, empty input, invalid base64 → `FormatException`, and argument validation. + **Description** `Component-Transport.md:271` ceilings the raw bundle at 100 MB and notes the diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 05b95f2f..40154135 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 3 | ## Summary @@ -1498,9 +1498,11 @@ the expected line count regardless of thread interleaving. |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs:72,104-142,160-161` | +**Resolution (2026-05-28):** Replaced the `private byte[]? _bundleBytes` field with `private string? _bundleTempPath`. `OnFileSelectedAsync` now creates `Path.GetTempPath()/scadalink-transport-staging/` (created on first use) and streams the upload via `InputFile.OpenReadStream(maxBytes).CopyToAsync(FileStream)` straight to a `Guid.NewGuid():N + .scadabundle` temp file; `TryLoadAsync` opens the same path as a fresh `FileStream` for each `IBundleImporter.LoadAsync` call. The component now implements `IDisposable` and a `DeleteBundleTempFile()` helper that runs on `ResetSessionState`, `OnFileSelectedAsync` (before a new upload), and `Dispose` (circuit teardown); IO failures during cleanup are swallowed so audit-failure-style defensive semantics hold. Per-circuit working set drops from up to `MaxBundleSizeMb` (default 100 MB) per open wizard to the 80 KB FileStream buffer. The existing reflection-based test helper `SeedAtPassphraseStep` was migrated to write bytes to a real temp file and set `_bundleTempPath`, so the 7 existing TransportImport bUnit tests still pass against the new staging model. + **Description** `OnFileSelectedAsync` reads the uploaded `.scadabundle` into a `MemoryStream`, diff --git a/code-reviews/Communication/findings.md b/code-reviews/Communication/findings.md index b8d59886..6c2125a0 100644 --- a/code-reviews/Communication/findings.md +++ b/code-reviews/Communication/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 3 | ## Summary @@ -857,9 +857,17 @@ Either way, replace `CentralCommunicationActorTests.ConnectionLost_DebugStreamsK |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Communication/Actors/CentralCommunicationActor.cs:73`, `src/ScadaLink.Communication/Actors/CentralCommunicationActor.cs:501`, `src/ScadaLink.Communication/Actors/CentralCommunicationActor.cs:357-367` | +**Resolution (2026-05-28):** Closed by Comm-016 — field removed in commit ac96b83. +The `_inProgressDeployments` dictionary, the `TrackMessageForCleanup` helper, +and the `HandleConnectionStateChanged` handler that consumed them were all +deleted as part of Comm-016's dead-code purge. A grep for `_inProgressDeployments` +in `CentralCommunicationActor.cs` finds only the explanatory comment block at +line 63-74 that documents the removal. There is no longer any unbounded-growth +hazard — the field does not exist. + **Description** `TrackMessageForCleanup` inserts `_inProgressDeployments[deploy.DeploymentId] = diff --git a/code-reviews/DeploymentManager/findings.md b/code-reviews/DeploymentManager/findings.md index 6f789ba4..df3ff339 100644 --- a/code-reviews/DeploymentManager/findings.md +++ b/code-reviews/DeploymentManager/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 3 | ## Summary @@ -1157,9 +1157,11 @@ Either: |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.DeploymentManager/ArtifactDeploymentService.cs:82-144,169-173` | +**Resolution (2026-05-28):** Hoisted the global artifact queries (shared scripts, external systems + methods, DB connections, notification lists, SMTP configurations) out of the per-site loop into a new private `FetchGlobalArtifactsAsync` that produces a `GlobalArtifactSnapshot` record. `DeployToAllSitesAsync` now calls it ONCE before the loop and threads the snapshot through a new prefetched-globals overload of `BuildDeployArtifactsCommandAsync`; the public single-site overload keeps the prior fetch-then-build behaviour for `RetryForSiteAsync`. Only the per-site data-connection query remains inside the loop. Regression tests `DeployToAllSitesAsync_HoistsGlobalArtifactQueriesOutOfPerSiteLoop` (three sites; pins exactly-one call to each global getter and one per-site call to `GetDataConnectionsBySiteIdAsync`) and `RetryForSiteAsync_SingleSitePath_StillRunsTheGlobalQueriesOnce` (single-site path still owns its own fetch). + **Description** `DeployToAllSitesAsync` loops over sites and calls diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 4916a19d..6bcd456a 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 3 | +| Open findings | 2 | ## Summary @@ -969,7 +969,9 @@ synchronous throw) to pin the new contract. | Severity | Low | | Category | Performance & resource management | | Location | `src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs:141` | -| Status | Open | +| Status | Resolved | + +**Resolution (2026-05-28):** Added a `RequestHasBody(HttpRequest)` guard that returns `true` only when `ContentLength > 0` or the method is POST / PUT / PATCH (the `HttpMethods.IsPost/Put/Patch` helpers); the `EnableBuffering` + `ReadBufferedRequestBodyAsync` call now sits behind that guard. Bodyless GET / HEAD / DELETE / TRACE / OPTIONS requests (and any explicit `Content-Length: 0`) skip the `FileBufferingReadStream` allocation; body-carrying methods with no Content-Length (chunked POST etc.) still buffer. Regression tests `BodylessMethod_SkipsEnableBuffering_RequestStreamIsNotReplaced` (GET/HEAD/DELETE theory), `BodylessPost_ContentLengthZero_SkipsEnableBuffering`, and `PostWithBody_StillEnablesBuffering_AndCapturesRequestSummary` (anti-regression). **Description** diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index b04c9ffe..7ec93b14 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 (1 Deferred — see ManagementService-012) | +| Open findings | 3 (1 Deferred — see ManagementService-012) | ## Summary @@ -1081,9 +1081,11 @@ Update `Component-ManagementService.md`: |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:1276`–`:1295` | +**Resolution (2026-05-28):** Swapped the per-`InstanceId` `GetInstanceByIdAsync` lookup loop for a single `templateRepo.GetAllInstancesAsync()` bulk fetch, projected into a `Dictionary` keyed by `Instance.Id`. The unfiltered branch now hits the configuration database exactly twice (deployment records + instances) regardless of fleet size. Existing test `QueryDeployments_UnfilteredForSiteScopedUser_DropsOutOfScopeRecords` was updated to mock the bulk path and to assert `GetInstanceByIdAsync` is no longer used on the unfiltered branch; new regression test `QueryDeployments_UnfilteredForSiteScopedUser_UsesBulkInstanceLoad_NotPerRecordLookup` pins `GetAllInstancesAsync` is invoked exactly once even with duplicate-instance records. + **Description** The site-scoped unfiltered branch of `HandleQueryDeployments` (added under diff --git a/code-reviews/NotificationOutbox/findings.md b/code-reviews/NotificationOutbox/findings.md index 24415f04..09d81b05 100644 --- a/code-reviews/NotificationOutbox/findings.md +++ b/code-reviews/NotificationOutbox/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 9 | +| Open findings | 8 | ## Summary @@ -279,7 +279,7 @@ remains the CD-015 raw-SQL `IF NOT EXISTS … INSERT` with `2601/2627` catch in |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationOutbox/NotificationOutboxActor.cs:267-277` | **Description** @@ -310,9 +310,7 @@ sweeps") in the actor XML, or — preferred — cache only the *types* at startu (`PreStart`) and resolve the scoped instance per sweep, so future adapters with stateful intent (timeouts, circuit breakers) cannot accidentally lose state. -**Resolution** - -_Unresolved._ +**Resolution (2026-05-28):** Cached the `NotificationType → adapter` dictionary on a new actor field `_adaptersCache`, built lazily on the first dispatch sweep that needs it. The cache is paired with an actor-lifetime `IServiceScope` (`_adaptersScope`, created on first use and disposed in `PostStop`) so the resolved scoped adapter instances live as long as the cache itself — respecting `EmailNotificationDeliveryAdapter`'s scoped lifetime without leaking captive-DbContext references across the actor's full lifetime. `RunDispatchPass` now calls `ResolveAdapters()` (instance method, no args), and the existing "resolved from the per-sweep scope" comment was rewritten to cross-reference the new cache rationale. Adapter set is static per process lifetime (confirmed against the DI registration in `AddNotificationOutbox`), so no invalidation hook is needed. Regression test `Dispatch_ResolvesAdaptersOnce_AcrossMultipleSweeps` registers a counting factory and pins the resolution count at exactly 1 across three end-to-end dispatch sweeps. ### NotificationOutbox-007 — `NotificationOutboxOptions.DispatchBatchSize`, `DeliveredKpiWindow`, and `PurgeInterval` are not in the design document diff --git a/code-reviews/NotificationService/findings.md b/code-reviews/NotificationService/findings.md index 4d0f3f3e..fa9f0bd8 100644 --- a/code-reviews/NotificationService/findings.md +++ b/code-reviews/NotificationService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 6 | +| Open findings | 5 | ## Summary @@ -735,9 +735,11 @@ Pass the sender mailbox into the wrapper's `AuthenticateAsync` path. The cleanes |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:14`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:19` | +**Resolution (2026-05-28):** Took option (a) — updated the class-level XML doc on `MailKitSmtpClientWrapper` with an explicit lifetime section (NS-022) describing single-connection ownership and the per-delivery factory contract (NOT a pool; `MailKit.Net.Smtp.SmtpClient` holds one TCP/TLS connection and is not thread-safe; the DI `Func` is a factory, callers run connect/auth/send/disconnect/dispose per send). A field-level comment was added to the `_client` declaration cross-referencing the class docs so a maintainer hunting from the field-initializer immediately sees the constraint. The wrapper code itself is unchanged — option (b) (transient SmtpClient per Send) was deliberately not taken because it would re-handshake TLS per email which is materially more expensive than the current per-delivery factory model the callers already implement. The concurrent-connection limit regression on the central-side delivery path (per-site semaphore on `EmailNotificationDeliveryAdapter`) is out of NotificationService scope and tracked separately. + **Description** `MailKitSmtpClientWrapper` declares `private readonly SmtpClient _client = new();` — a single `SmtpClient` is constructed when the wrapper is constructed and lives for the wrapper's lifetime. The DI registration is `services.AddSingleton>(_ => () => new MailKitSmtpClientWrapper());` (`ServiceCollectionExtensions.cs:19`) — every invocation of the factory creates a **new** wrapper and therefore a **new** `SmtpClient`. `NotificationDeliveryService.DeliverAsync` (the orphan, per NS-019) and `EmailNotificationDeliveryAdapter.SendAsync` both invoke the factory per send and dispose the wrapper at end of send. So in practice there is no connection pooling — every send pays a full TCP+TLS handshake. diff --git a/code-reviews/README.md b/code-reviews/README.md index 8ff39d46..a98e4721 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,37 +41,37 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 22 | -| Low | 43 | -| **Total** | **65** | +| Medium | 19 | +| Low | 35 | +| **Total** | **54** | ## Module Status | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| -| [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 11 | -| [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 23 | -| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 33 | +| [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 11 | +| [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | +| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 14 | | [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 23 | -| [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 22 | +| [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 22 | | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | -| [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 24 | +| [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 24 | | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/3 | 4 | 22 | -| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 25 | -| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 23 | -| [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 10 | -| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 25 | +| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 25 | +| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 23 | +| [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 10 | +| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 25 | | [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 6 | -| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 23 | +| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 26 | | [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/2 | 5 | 24 | | [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/0 | 3 | 22 | -| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 12 | +| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 12 | ## Pending Findings @@ -88,14 +88,11 @@ _None open._ _None open._ -### Medium (22) +### Medium (19) | ID | Module | Title | |----|--------|-------| | AuditLog-001 | [AuditLog](AuditLog/findings.md) | Combined-telemetry transport is plumbed end-to-end but never invoked in production | -| AuditLog-005 | [AuditLog](AuditLog/findings.md) | `GetBacklogStatsAsync` holds the SQLite hot-path write lock for the full COUNT+MIN scan | -| CLI-019 | [CLI](CLI/findings.md) | `bundle export` decodes the entire base64 bundle into memory before writing | -| Communication-017 | [Communication](Communication/findings.md) | `_inProgressDeployments` grows unboundedly — successful deployments are never cleaned up | | ConfigurationDatabase-016 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `InboundApiRepository.GetApiKeyByValueAsync` hashes the candidate with the unpeppered `ApiKeyHasher.Default` | | ExternalSystemGateway-020 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `JsonElementToParameterValue` silently downcasts non-Int64 JSON numbers to `double`, losing precision for `decimal` SQL parameters on retry | | Host-016 | [Host](Host/findings.md) | Site `CentralContactPoints` second entry targets the site's own remoting port | @@ -115,7 +112,7 @@ _None open._ | TemplateEngine-020 | [TemplateEngine](TemplateEngine/findings.md) | `Create*` audit entries are written with `EntityId = "0"` before `SaveChangesAsync` populates the real key | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | -### Low (43) +### Low (35) | ID | Module | Title | |----|--------|-------| @@ -123,7 +120,6 @@ _None open._ | CLI-020 | [CLI](CLI/findings.md) | `bundle export` success-envelope parse is unguarded | | CLI-022 | [CLI](CLI/findings.md) | `CommandTreeTests` excludes the two new command groups | | CentralUI-029 | [CentralUI](CentralUI/findings.md) | `ConfigurationAuditLog` uses `JS.InvokeAsync("eval", ...)` instead of a dedicated JS module | -| CentralUI-031 | [CentralUI](CentralUI/findings.md) | `TransportImport` buffers the full bundle bytes in component state | | CentralUI-032 | [CentralUI](CentralUI/findings.md) | `AuditResultsGrid` paging is forward-only, no Previous button | | CentralUI-033 | [CentralUI](CentralUI/findings.md) | Drill-in / query-string code paths for the new Transport + SiteCalls pages are untested | | ClusterInfrastructure-011 | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | `SectionName` constant is decorative — no binding site references it | @@ -138,7 +134,6 @@ _None open._ | ConfigurationDatabase-024 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | Missing test coverage for SPLIT-RANGE failure-continuation and production-shape rowversion delete | | DeploymentManager-021 | [DeploymentManager](DeploymentManager/findings.md) | `ResolveSiteIdentifierAsync` silently substitutes the DB id when the site row is missing | | DeploymentManager-022 | [DeploymentManager](DeploymentManager/findings.md) | `Pending` and `InProgress` are written back-to-back with no intervening work | -| DeploymentManager-023 | [DeploymentManager](DeploymentManager/findings.md) | `BuildDeployArtifactsCommandAsync` re-queries system-wide artifacts once per site | | DeploymentManager-024 | [DeploymentManager](DeploymentManager/findings.md) | Test probe actors hold mutable static state across tests | | ExternalSystemGateway-021 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `ApplyAuth` silently sends an unauthenticated request on unknown `AuthType`, empty `AuthConfiguration`, or malformed Basic config | | HealthMonitoring-021 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralSiteId = "central"` reserved constant silently collides with a real site named "central" | @@ -146,19 +141,13 @@ _None open._ | Host-018 | [Host](Host/findings.md) | Shipped per-role configs omit `NodeOptions.NodeName`, leaving `SourceNode` null | | Host-020 | [Host](Host/findings.md) | `MinimumLevel.Is` silently overrides any operator-set `Serilog:MinimumLevel` | | Host-021 | [Host](Host/findings.md) | Microsoft `Logging:LogLevel` section in `appsettings.json` is dead config under Serilog | -| InboundAPI-019 | [InboundAPI](InboundAPI/findings.md) | `EnableBuffering()` called unconditionally on every request, including bodyless requests | | InboundAPI-023 | [InboundAPI](InboundAPI/findings.md) | `EndpointExtensions.HandleInboundApiRequest` composition wiring has no test coverage | -| ManagementService-023 | [ManagementService](ManagementService/findings.md) | HandleQueryDeployments unfiltered branch is N+1 on instance lookup | -| NotificationOutbox-006 | [NotificationOutbox](NotificationOutbox/findings.md) | `ResolveAdapters` rebuilds the `NotificationType → adapter` dictionary on every dispatch sweep | | NotificationOutbox-008 | [NotificationOutbox](NotificationOutbox/findings.md) | `FallbackMaxRetries` / `FallbackRetryDelay` path is unreachable in production AND untested | -| NotificationService-022 | [NotificationService](NotificationService/findings.md) | `MailKitSmtpClientWrapper` holds a long-lived `SmtpClient`; combined with per-send factory, the design comment about pooling is contradicted | | NotificationService-025 | [NotificationService](NotificationService/findings.md) | `CredentialRedactor` over-masks: any 4-character credential component is masked anywhere it appears, including unrelated log text | | Security-021 | [Security](Security/findings.md) | `RequireHttpsCookie=false` dev opt-out has no warning path — an HTTP production deployment silently transmits the JWT bearer credential in cleartext | | SiteCallAudit-006 | [SiteCallAudit](SiteCallAudit/findings.md) | Stuck-only paging test does not exercise the multi-page boundary with an interleaved non-stuck row at the cursor | | SiteEventLogging-018 | [SiteEventLogging](SiteEventLogging/findings.md) | `FailedWriteCount` is exposed but never consumed by Health Monitoring | -| SiteEventLogging-022 | [SiteEventLogging](SiteEventLogging/findings.md) | `Cache=Shared` is redundant for a single-connection logger | | SiteEventLogging-023 | [SiteEventLogging](SiteEventLogging/findings.md) | Concurrent-stress test uses a non-volatile `stop` flag | | StoreAndForward-022 | [StoreAndForward](StoreAndForward/findings.md) | `NotifyCachedCallObserverAsync` silently drops the entire audit lifecycle when the message id is not a parseable `TrackedOperationId` | | StoreAndForward-023 | [StoreAndForward](StoreAndForward/findings.md) | `siteId` silently defaults to empty when no `IStoreAndForwardSiteContext` is registered, degrading audit telemetry correlation | -| Transport-008 | [Transport](Transport/findings.md) | `PreviewAsync` issues an N+1 `GetTemplateWithChildrenAsync` per matching template name | | Transport-012 | [Transport](Transport/findings.md) | "Bundle Import" filter promised in design doc not surfaced in Configuration Audit Log Viewer UI | diff --git a/code-reviews/SiteEventLogging/findings.md b/code-reviews/SiteEventLogging/findings.md index af2d8cc4..764710dd 100644 --- a/code-reviews/SiteEventLogging/findings.md +++ b/code-reviews/SiteEventLogging/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 5 | +| Open findings | 4 | ## Summary @@ -1074,9 +1074,17 @@ avoid all string-parsing. |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:52` | +**Resolution (2026-05-28):** Dropped `Cache=Shared` from the default connection +string in `SiteEventLogger`'s ctor — the logger owns exactly one +`SqliteConnection` serialised under `_writeLock`, so the cross-connection +shared-cache mode is dormant. The `connectionStringOverride` ctor parameter +still lets a test path inject `Cache=Shared` explicitly when needed. No test +relied on the default option (grep across `tests/ScadaLink.SiteEventLogging.Tests` +finds zero references). Behaviour unchanged for production callers. + **Description** The connection string is built as diff --git a/code-reviews/Transport/findings.md b/code-reviews/Transport/findings.md index bca6f463..92557d76 100644 --- a/code-reviews/Transport/findings.md +++ b/code-reviews/Transport/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 9 | +| Open findings | 8 | ## Summary @@ -312,9 +312,11 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Import/BundleImporter.cs:252-272` | +**Resolution (2026-05-28):** Added a bulk `GetTemplatesWithChildrenAsync(IEnumerable names, CancellationToken)` method on `ITemplateEngineRepository` and implemented it on `TemplateEngineRepository` with a single `Where(t => names.Contains(t.Name))` + `Include(Attributes/Alarms/Scripts/Compositions).AsSplitQuery()` round-trip; null / empty / duplicate names are filtered before the IN clause. `BundleImporter.PreviewAsync` now calls the bulk method once with `content.Templates.Select(t => t.Name)` and indexes the result by name, replacing the previous `GetAllTemplatesAsync` scan + per-match `GetTemplateWithChildrenAsync(stub.Id)` round-trip pair. Regression tests: `TemplateEngineRepositoryTests.GetTemplatesWithChildrenAsync_*` (4 tests — bulk fetch, empty input, null enumerable, dedup) and `BundleImporterPreviewTests.PreviewAsync_multiple_templates_with_children_diffs_each_correctly` (asserts the bulk fetch hydrates all three templates' Attributes / Alarms / Scripts so the diff classifies each as Identical). + **Description** Building the per-template diff loops over every existing stub returned by diff --git a/src/ScadaLink.AuditLog/Site/SqliteAuditWriter.cs b/src/ScadaLink.AuditLog/Site/SqliteAuditWriter.cs index 7736cc83..7cd71470 100644 --- a/src/ScadaLink.AuditLog/Site/SqliteAuditWriter.cs +++ b/src/ScadaLink.AuditLog/Site/SqliteAuditWriter.cs @@ -40,6 +40,18 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable private const int SqliteErrorConstraint = 19; private readonly SqliteConnection _connection; + // AuditLog-005: dedicated read-only connection used by GetBacklogStatsAsync, + // ReadPendingAsync, ReadPendingSinceAsync, and ReadForwardedAsync so a slow + // backlog scan (COUNT(*) over hundreds of thousands of Pending rows under a + // central outage) never parks the hot-path writer behind _writeLock. + // SQLite-with-WAL allows a second connection on the same file to read + // concurrently with the writer; the writer's WAL pragma is set in + // InitializeSchema before this connection is opened. The reader connection + // has its own _readLock because SqliteConnection itself is not thread-safe + // even in read-only mode — multiple read callers can otherwise interleave + // commands on the shared connection. + private readonly SqliteConnection _readConnection; + private readonly object _readLock = new(); private readonly SqliteAuditWriterOptions _options; private readonly ILogger _logger; private readonly INodeIdentityProvider _nodeIdentity; @@ -74,6 +86,17 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable InitializeSchema(); + // AuditLog-005: open a second connection for read-only callers + // (GetBacklogStatsAsync, ReadPendingAsync, ReadPendingSinceAsync, + // ReadForwardedAsync). InitializeSchema set journal_mode=WAL on the + // writer connection, which is a database-level setting that persists + // for the file — subsequent connections to the same file see WAL and + // can read concurrently with the writer without taking _writeLock. + // Reuse the same connection string so the read connection sees the + // same Data Source / Cache settings as the writer. + _readConnection = new SqliteConnection(connectionString); + _readConnection.Open(); + _writeQueue = Channel.CreateBounded( new BoundedChannelOptions(_options.ChannelCapacity) { @@ -100,6 +123,23 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable pragmaCmd.ExecuteNonQuery(); } + // AuditLog-005: enable WAL so a second connection on the same file can + // serve read-only callers (GetBacklogStatsAsync, ReadPendingAsync, + // ReadPendingSinceAsync, ReadForwardedAsync) concurrently with the + // batched writer, decoupling those reads from _writeLock. WAL is a + // database-level setting persisted in the file header; setting it on + // the writer connection means every connection opened to the file + // afterwards inherits WAL behaviour. PRAGMA journal_mode returns the + // mode actually adopted ("memory" for ":memory:" / shared-cache memory + // mode, "wal" for file-backed) — we don't error if WAL was rejected + // because the read connection's correctness does not depend on WAL + // itself, only its concurrency advantage does. + using (var pragmaCmd = _connection.CreateCommand()) + { + pragmaCmd.CommandText = "PRAGMA journal_mode = WAL"; + pragmaCmd.ExecuteNonQuery(); + } + using var cmd = _connection.CreateCommand(); cmd.CommandText = """ CREATE TABLE IF NOT EXISTS AuditLog ( @@ -392,14 +432,18 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable throw new ArgumentOutOfRangeException(nameof(limit), "limit must be > 0."); } - // SqliteConnection is not thread-safe so we go through the same write - // lock the batch INSERTer uses. The actor caller is single-threaded, - // so contention is bounded. - lock (_writeLock) + // AuditLog-005: read via the dedicated _readConnection so this scan + // (which can be expensive when the backlog grows under a central + // outage) does not block the batched writer on _writeLock. WAL mode + // gives us a stable snapshot of the table while writes proceed on the + // writer connection. _readLock serialises this connection across + // multiple concurrent read callers since SqliteConnection itself is + // not thread-safe. + lock (_readLock) { ObjectDisposedException.ThrowIf(_disposed, this); - using var cmd = _connection.CreateCommand(); + using var cmd = _readConnection.CreateCommand(); cmd.CommandText = """ SELECT EventId, OccurredAtUtc, Channel, Kind, CorrelationId, SourceSiteId, SourceNode, SourceInstanceId, SourceScript, Actor, Target, @@ -445,12 +489,14 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable throw new ArgumentOutOfRangeException(nameof(limit), "limit must be > 0."); } - // Mirror ReadPendingAsync: the write lock guards the single connection. - lock (_writeLock) + // AuditLog-005: mirror ReadPendingAsync — read via _readConnection / + // _readLock so this query never contends with the batched writer on + // _writeLock. + lock (_readLock) { ObjectDisposedException.ThrowIf(_disposed, this); - using var cmd = _connection.CreateCommand(); + using var cmd = _readConnection.CreateCommand(); cmd.CommandText = """ SELECT EventId, OccurredAtUtc, Channel, Kind, CorrelationId, SourceSiteId, SourceNode, SourceInstanceId, SourceScript, Actor, Target, @@ -520,12 +566,13 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable throw new ArgumentOutOfRangeException(nameof(batchSize), "batchSize must be > 0."); } - // Mirror ReadPendingAsync: the write lock guards the single connection. - lock (_writeLock) + // AuditLog-005: read via _readConnection / _readLock — same lock- + // decoupling as ReadPendingAsync. + lock (_readLock) { ObjectDisposedException.ThrowIf(_disposed, this); - using var cmd = _connection.CreateCommand(); + using var cmd = _readConnection.CreateCommand(); cmd.CommandText = """ SELECT EventId, OccurredAtUtc, Channel, Kind, CorrelationId, SourceSiteId, SourceNode, SourceInstanceId, SourceScript, Actor, Target, @@ -599,7 +646,13 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable int pendingCount; DateTime? oldestPending; - lock (_writeLock) + // AuditLog-005: read via the dedicated _readConnection (under + // _readLock) so this probe — polled every 30 s by SiteAuditBacklogReporter + // — never blocks the batched hot-path writer on _writeLock. Under a + // central outage the Pending backlog can grow to hundreds of thousands + // of rows and the COUNT(*) scan correspondingly stretches; that no + // longer adds tail latency to user-facing audit writes. + lock (_readLock) { ObjectDisposedException.ThrowIf(_disposed, this); @@ -607,7 +660,7 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable // index range avoids a second scan. The IX_SiteAuditLog_ForwardState_Occurred // index makes both aggregates cheap (count is a covering scan, min // is the first key). - using var cmd = _connection.CreateCommand(); + using var cmd = _readConnection.CreateCommand(); cmd.CommandText = """ SELECT COUNT(*), MIN(OccurredAtUtc) FROM AuditLog @@ -758,6 +811,15 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable _disposed = true; _connection.Dispose(); } + + // AuditLog-005: dispose the dedicated read connection after the writer + // is fully drained and closed. _readLock is taken to fence out any + // in-flight read caller that grabbed the lock before _disposed flipped + // — they observe ObjectDisposedException on the next attempt. + lock (_readLock) + { + _readConnection.Dispose(); + } } /// An audit event awaiting persistence by the background writer. diff --git a/src/ScadaLink.CLI/Commands/BundleCommands.cs b/src/ScadaLink.CLI/Commands/BundleCommands.cs index e13512d6..3b5e4872 100644 --- a/src/ScadaLink.CLI/Commands/BundleCommands.cs +++ b/src/ScadaLink.CLI/Commands/BundleCommands.cs @@ -121,9 +121,15 @@ public static class BundleCommands using var doc = JsonDocument.Parse(jsonOk); var base64 = doc.RootElement.GetProperty("base64Bundle").GetString()!; var byteCount = doc.RootElement.GetProperty("byteCount").GetInt32(); - var bytes = Convert.FromBase64String(base64); - File.WriteAllBytes(output, bytes); - Console.WriteLine($"Wrote {bytes.Length:N0} bytes to {output} (server reported {byteCount:N0})."); + // CLI-019: stream the base64 → file write so a 100 MB bundle + // doesn't double-buffer through Convert.FromBase64String's + // ~100 MB byte[] on the LOH plus a synchronous File.WriteAllBytes. + // The management envelope's body is still buffered into the + // jsonOk string (wire-format limit), but the decode + write + // are now chunked, so peak working-set drops from + // ~base64+byte[]+envelope to ~base64+small-chunk. + var written = StreamBase64ToFile(base64, output); + Console.WriteLine($"Wrote {written:N0} bytes to {output} (server reported {byteCount:N0})."); return 0; }); }); @@ -250,6 +256,61 @@ public static class BundleCommands // the longer BundleCommandTimeout and a per-command success handler, so the // exit-code contract is unified across every command group. + // CLI-019: chunked base64 → file streaming. The management envelope's + // success body is a single buffered JSON string (the wire format does not + // currently support response-body streaming), so we cannot remove the + // ~base64-string + ~envelope-string allocation. What we CAN — and do — + // remove is the intermediate ~bytecount-sized byte[] that + // Convert.FromBase64String allocates plus the synchronous File.WriteAllBytes: + // we slice the base64 string into 4-byte-multiple chunks (4 base64 chars + // decode into exactly 3 bytes, so any multiple of 4 is a clean boundary) + // and decode each chunk into a small rented buffer that we copy into the + // output FileStream. The chunk size is a tradeoff — large enough that the + // per-chunk loop overhead is negligible, small enough that we never put + // anything on the LOH (1 MB is below the 85 KB LOH threshold's larger + // cousin for buffers we don't keep). Returns the total decoded byte count + // for the post-write summary line. + internal const int Base64StreamChunkChars = 1024 * 1024; // 1 MB of base64 chars ≈ 768 KB decoded + + internal static long StreamBase64ToFile(string base64, string outputPath) + { + if (base64 is null) throw new ArgumentNullException(nameof(base64)); + if (string.IsNullOrEmpty(outputPath)) throw new ArgumentException("Output path required.", nameof(outputPath)); + + // Skip any leading whitespace and trailing padding noise. Convert.TryFromBase64Chars + // tolerates internal whitespace, but slicing on arbitrary positions would split a + // run of base64 chars mid-quad — round the chunk to a multiple of 4 so each slice + // is independently decodable. + var chunkChars = Base64StreamChunkChars - (Base64StreamChunkChars % 4); + var totalChars = base64.Length; + var totalWritten = 0L; + + using var fileStream = new FileStream( + outputPath, FileMode.Create, FileAccess.Write, FileShare.None, + bufferSize: 81920, useAsync: false); + + // 4 base64 chars = 3 bytes, so the decoded buffer is sized accordingly. + var byteBuffer = new byte[(chunkChars / 4) * 3]; + + for (var offset = 0; offset < totalChars; offset += chunkChars) + { + var take = Math.Min(chunkChars, totalChars - offset); + var slice = base64.AsSpan(offset, take); + + // The final slice may be shorter than chunkChars and may carry + // trailing '=' padding; TryFromBase64Chars handles that. + if (!Convert.TryFromBase64Chars(slice, byteBuffer, out var written)) + { + throw new FormatException( + $"Bundle response contained invalid base64 at character offset {offset}."); + } + fileStream.Write(byteBuffer, 0, written); + totalWritten += written; + } + + return totalWritten; + } + private static Option?> NameListOption(string name, string description) { var opt = new Option?>(name) diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor b/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor index 69778362..ae1b0aea 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor @@ -103,7 +103,7 @@
Reading bundle…
} - @if (_bundleBytes is not null && _errorMessage is null) + @if (_bundleTempPath is not null && _errorMessage is null) { @if (_session is not null) { diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs b/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs index 81492135..e0f594c4 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs @@ -39,11 +39,17 @@ namespace ScadaLink.CentralUI.Components.Pages.Design; /// /// Cached bundle bytes: because currently /// peeks the manifest by attempting decryption, encrypted bundles require two -/// LoadAsync invocations. We cache the raw bytes in _bundleBytes after the -/// first read so the user does not need to re-select the file before entering the -/// passphrase. The bytes are cleared on Done / Back-to-Upload. +/// LoadAsync invocations. CentralUI-031: we previously cached the raw bytes in a +/// byte[] _bundleBytes field, which buffered the full upload (default cap +/// 100 MB) in the component's per-circuit state — multiplied across concurrent +/// operator sessions, that produced real central-node memory pressure. The +/// bytes are now streamed once to a per-session temp file under +/// Path.GetTempPath()/scadalink-transport-staging/ and only the path is +/// retained on the component. The file is deleted on Back-to-Upload / Reset / +/// successful Apply / component Dispose, so an abandoned wizard does not leak +/// staged bundle plaintext beyond circuit teardown. /// -public partial class TransportImport : ComponentBase +public partial class TransportImport : ComponentBase, IDisposable { public enum ImportWizardStep { @@ -66,13 +72,23 @@ public partial class TransportImport : ComponentBase private ImportWizardStep _step = ImportWizardStep.Upload; private string? _errorMessage; - // ---- Session + cached bytes ---- - // Bundle bytes are cached so the same file can be re-attempted with a - // passphrase without forcing the user to re-pick it. Cleared in ResetAll. - private byte[]? _bundleBytes; + // ---- Session + cached bundle path ---- + // CentralUI-031: the upload is streamed to a per-session temp file and only + // the path is retained on the component, so we don't hold an entire bundle + // (up to MaxBundleSizeMb, default 100 MB) in per-circuit memory across the + // wizard's lifetime. The file is deleted on every wizard reset path and on + // component disposal so an abandoned wizard cannot leak staged plaintext + // beyond circuit teardown. + private string? _bundleTempPath; private BundleSession? _session; private bool _uploadInProgress; + // Staging directory for in-flight bundle uploads. Lives under the system + // temp directory rather than wwwroot/ because the file is never served to + // a browser — it is only read by the in-process IBundleImporter. + private static readonly string StagingDir = + Path.Combine(Path.GetTempPath(), "scadalink-transport-staging"); + // ---- Step 2: passphrase ---- private string _passphrase = string.Empty; private int _failedUnlockAttempts; @@ -106,7 +122,7 @@ public partial class TransportImport : ComponentBase _errorMessage = null; _uploadInProgress = true; _session = null; - _bundleBytes = null; + DeleteBundleTempFile(); try { var maxBytes = Options.Value.MaxBundleSizeMb * 1024L * 1024L; @@ -116,13 +132,21 @@ public partial class TransportImport : ComponentBase return; } + // CentralUI-031: stream the upload directly to a per-session temp + // file so the central node's working set is bounded by the + // FileStream buffer (~80 KB) rather than the full bundle bytes. // OpenReadStream's MaxAllowedSize defaults to 500_000 bytes — bump // it to the configured cap so the read doesn't throw before we get // to the importer's own length check. - using var fileStream = e.File.OpenReadStream(maxBytes); - using var ms = new MemoryStream(); - await fileStream.CopyToAsync(ms); - _bundleBytes = ms.ToArray(); + Directory.CreateDirectory(StagingDir); + _bundleTempPath = Path.Combine(StagingDir, $"{Guid.NewGuid():N}.scadabundle"); + using (var fileStream = e.File.OpenReadStream(maxBytes)) + await using (var dest = new FileStream( + _bundleTempPath, FileMode.CreateNew, FileAccess.Write, FileShare.None, + bufferSize: 81920, useAsync: true)) + { + await fileStream.CopyToAsync(dest); + } await TryLoadAsync(passphrase: null); } @@ -150,14 +174,19 @@ public partial class TransportImport : ComponentBase /// private async Task TryLoadAsync(string? passphrase) { - if (_bundleBytes is null) + if (_bundleTempPath is null || !File.Exists(_bundleTempPath)) { - _errorMessage = "No bundle bytes cached — please re-select the file."; + _errorMessage = "No bundle staged — please re-select the file."; return; } try { - using var stream = new MemoryStream(_bundleBytes); + // CentralUI-031: read the staged bundle straight off disk; the + // importer's LoadAsync only walks the stream forward, so a plain + // FileStream is sufficient (no need to buffer it back into memory). + using var stream = new FileStream( + _bundleTempPath, FileMode.Open, FileAccess.Read, FileShare.Read, + bufferSize: 81920, useAsync: true); _session = await BundleImporter.LoadAsync(stream, passphrase, CancellationToken.None); _errorMessage = null; } @@ -528,7 +557,7 @@ public partial class TransportImport : ComponentBase private void ResetSessionState() { _session = null; - _bundleBytes = null; + DeleteBundleTempFile(); _preview = null; _resolutions = null; _passphrase = string.Empty; @@ -536,4 +565,44 @@ public partial class TransportImport : ComponentBase _result = null; _validationErrors = null; } + + /// + /// CentralUI-031: deletes the staged bundle temp file if any. Swallows IO + /// failures — an undeletable temp file is best-effort cleanup and must not + /// block the wizard. + /// + private void DeleteBundleTempFile() + { + var path = _bundleTempPath; + _bundleTempPath = null; + if (path is null) return; + try + { + if (File.Exists(path)) + { + File.Delete(path); + } + } + catch (IOException) + { + // Another handle may still be open (e.g. an in-flight LoadAsync + // read). Leave the file behind; the OS temp dir is reaped on its + // own schedule. Audit-failure-style: never block the user-facing + // action. + } + catch (UnauthorizedAccessException) + { + // Same rationale. + } + } + + /// + /// CentralUI-031: ensures the staged temp file does not survive circuit + /// teardown. Blazor invokes Dispose when the user navigates away or the + /// circuit ends, so an abandoned wizard cleans up automatically. + /// + public void Dispose() + { + DeleteBundleTempFile(); + } } diff --git a/src/ScadaLink.Commons/Interfaces/Repositories/ITemplateEngineRepository.cs b/src/ScadaLink.Commons/Interfaces/Repositories/ITemplateEngineRepository.cs index 36a4ff15..be0c2452 100644 --- a/src/ScadaLink.Commons/Interfaces/Repositories/ITemplateEngineRepository.cs +++ b/src/ScadaLink.Commons/Interfaces/Repositories/ITemplateEngineRepository.cs @@ -15,6 +15,18 @@ public interface ITemplateEngineRepository /// The template ID. /// Cancellation token. Task GetTemplateWithChildrenAsync(int id, CancellationToken cancellationToken = default); + /// + /// Bulk variant of + /// that fetches every template whose matches one of + /// in a single SQL/EF query, eager-loading + /// Attributes / Alarms / Scripts / Compositions. Resolves the Transport-008 + /// N+1 in BundleImporter.PreviewAsync — names that don't match an + /// existing template are omitted from the result rather than producing a + /// null entry, so callers should look up by name into the returned list. + /// + /// Template names to load. Duplicate / null / empty names are filtered out. + /// Cancellation token. + Task> GetTemplatesWithChildrenAsync(IEnumerable names, CancellationToken cancellationToken = default); /// Retrieves all templates. /// Cancellation token. Task> GetAllTemplatesAsync(CancellationToken cancellationToken = default); diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs index 06cf50f2..9d3ef663 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs @@ -39,6 +39,30 @@ public class TemplateEngineRepository : ITemplateEngineRepository return await GetTemplateByIdAsync(id, cancellationToken); } + /// + public async Task> GetTemplatesWithChildrenAsync( + IEnumerable names, CancellationToken cancellationToken = default) + { + // Transport-008: bulk lookup replaces the per-name N+1 in + // BundleImporter.PreviewAsync. Filter out null / empty / duplicate + // names before the query so EF emits a clean, deduplicated IN clause. + if (names is null) return Array.Empty