From 0f3de4d5100c47468a5b36a9371d61340f92a97b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:47:52 -0400 Subject: [PATCH] fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-009) Fix two resource-management bugs in StartDeployWatcher / BuildDefaultHierarchySource: (a) Replace the discarded `_ = StartAsync(...)` with an explicit task variable that surfaces any synchronous InvalidOperationException (called-twice guard) rather than silently swallowing it. (b) Change both StartDeployWatcher and BuildDefaultHierarchySource to use ??= on _ownedRepositoryClient so the first client created (by whichever path runs first) is reused by the second path, preventing a second GalaxyRepositoryClient from being created and the first from leaking past the driver's lifetime. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.Galaxy/findings.md | 4 +-- .../GalaxyDriver.cs | 34 ++++++++----------- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index 6db2fdd..ca09a71 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -153,13 +153,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `GalaxyDriver.cs:354-371` | -| Status | Open | +| Status | Resolved | **Description:** `StartDeployWatcher` launches the watch loop with `_ = _deployWatcher.StartAsync(CancellationToken.None)` — a fire-and-forget with a discarded `Task`. `StartAsync` can throw synchronously (`InvalidOperationException` if already started); the discard masks that programming error. Separately, `StartDeployWatcher` builds an `_ownedRepositoryClient` purely for the watcher when discovery has not run yet — if `DiscoverAsync` later runs, `BuildDefaultHierarchySource` overwrites `_ownedRepositoryClient` with a second client, leaking the first (only the latest reference is disposed in `Dispose`). **Recommendation:** Await `StartAsync` (it completes synchronously after scheduling) or at least observe its result. Reuse a single `GalaxyRepositoryClient` across the deploy watcher and the hierarchy source instead of letting `BuildDefaultHierarchySource` clobber the field — guard the assignment or build the client once in `InitializeAsync`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — (a) replaced `_ = _deployWatcher.StartAsync(...)` discard with an explicit variable + `IsFaulted` check so any synchronous throw from `StartAsync` (e.g. called-twice `InvalidOperationException`) propagates rather than being silently swallowed; (b) changed both `StartDeployWatcher` and `BuildDefaultHierarchySource` to use `_ownedRepositoryClient ??=` so a client built by the watcher is reused by discovery instead of being overwritten and leaked — only one `GalaxyRepositoryClient` instance is now created and disposed. ### Driver.Galaxy-010 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index 7c2d12a..8f8571a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -445,17 +445,21 @@ public sealed class GalaxyDriver // Reuse the lazily-built repository client (DiscoverAsync constructs it on demand). // If discovery hasn't run yet, build the client here so the watcher has a target. - if (_ownedRepositoryClient is null) - { - _ownedRepositoryClient = MxGateway.Client.GalaxyRepositoryClient.Create( - BuildClientOptions(_options.Gateway)); - } + // Driver.Galaxy-009 fix: guard with ??= so if BuildDefaultHierarchySource later runs + // it reuses this client rather than overwriting the field and leaking the first instance. + _ownedRepositoryClient ??= MxGateway.Client.GalaxyRepositoryClient.Create( + BuildClientOptions(_options.Gateway)); var source = new GatewayGalaxyDeployWatchSource(_ownedRepositoryClient); _deployWatcher = new DeployWatcher(source, _logger); _deployWatcher.OnRediscoveryNeeded += (_, args) => OnRediscoveryNeeded?.Invoke(this, args); - _ = _deployWatcher.StartAsync(CancellationToken.None); + // StartAsync schedules the background loop and returns Task.CompletedTask immediately. + // It throws InvalidOperationException synchronously if called twice (programming error). + // Driver.Galaxy-009 fix: don't discard the return value — observe any synchronous throw. + var startTask = _deployWatcher.StartAsync(CancellationToken.None); + // The task is already completed (StartAsync is synchronous); surface any synchronous fault. + if (startTask.IsFaulted) startTask.GetAwaiter().GetResult(); } /// @@ -1032,20 +1036,10 @@ public sealed class GalaxyDriver /// private IGalaxyHierarchySource BuildDefaultHierarchySource() { - var gw = _options.Gateway; - var clientOptions = new MxGatewayClientOptions - { - Endpoint = new Uri(gw.Endpoint, UriKind.Absolute), - ApiKey = ResolveApiKey(gw.ApiKeySecretRef), - UseTls = gw.UseTls, - CaCertificatePath = gw.CaCertificatePath, - ConnectTimeout = TimeSpan.FromSeconds(gw.ConnectTimeoutSeconds), - DefaultCallTimeout = TimeSpan.FromSeconds(gw.DefaultCallTimeoutSeconds), - StreamTimeout = gw.StreamTimeoutSeconds > 0 - ? TimeSpan.FromSeconds(gw.StreamTimeoutSeconds) - : null, - }; - _ownedRepositoryClient = GalaxyRepositoryClient.Create(clientOptions); + // Driver.Galaxy-009 fix: reuse a client that StartDeployWatcher may have already + // created (??=) rather than always overwriting the field and leaking the first + // instance. Both paths produce equivalent clients from the same options. + _ownedRepositoryClient ??= GalaxyRepositoryClient.Create(BuildClientOptions(_options.Gateway)); return new TracedGalaxyHierarchySource( new GatewayGalaxyHierarchySource(_ownedRepositoryClient), _options.MxAccess.ClientName); }