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) <noreply@anthropic.com>
This commit is contained in:
@@ -153,13 +153,13 @@
|
|||||||
| Severity | Medium |
|
| Severity | Medium |
|
||||||
| Category | Error handling & resilience |
|
| Category | Error handling & resilience |
|
||||||
| Location | `GalaxyDriver.cs:354-371` |
|
| 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`).
|
**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`.
|
**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
|
### Driver.Galaxy-010
|
||||||
|
|
||||||
|
|||||||
@@ -445,17 +445,21 @@ public sealed class GalaxyDriver
|
|||||||
|
|
||||||
// Reuse the lazily-built repository client (DiscoverAsync constructs it on demand).
|
// 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 discovery hasn't run yet, build the client here so the watcher has a target.
|
||||||
if (_ownedRepositoryClient is null)
|
// 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(
|
_ownedRepositoryClient ??= MxGateway.Client.GalaxyRepositoryClient.Create(
|
||||||
BuildClientOptions(_options.Gateway));
|
BuildClientOptions(_options.Gateway));
|
||||||
}
|
|
||||||
|
|
||||||
var source = new GatewayGalaxyDeployWatchSource(_ownedRepositoryClient);
|
var source = new GatewayGalaxyDeployWatchSource(_ownedRepositoryClient);
|
||||||
_deployWatcher = new DeployWatcher(source, _logger);
|
_deployWatcher = new DeployWatcher(source, _logger);
|
||||||
_deployWatcher.OnRediscoveryNeeded += (_, args) => OnRediscoveryNeeded?.Invoke(this, args);
|
_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();
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
@@ -1032,20 +1036,10 @@ public sealed class GalaxyDriver
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
private IGalaxyHierarchySource BuildDefaultHierarchySource()
|
private IGalaxyHierarchySource BuildDefaultHierarchySource()
|
||||||
{
|
{
|
||||||
var gw = _options.Gateway;
|
// Driver.Galaxy-009 fix: reuse a client that StartDeployWatcher may have already
|
||||||
var clientOptions = new MxGatewayClientOptions
|
// created (??=) rather than always overwriting the field and leaking the first
|
||||||
{
|
// instance. Both paths produce equivalent clients from the same options.
|
||||||
Endpoint = new Uri(gw.Endpoint, UriKind.Absolute),
|
_ownedRepositoryClient ??= GalaxyRepositoryClient.Create(BuildClientOptions(_options.Gateway));
|
||||||
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);
|
|
||||||
return new TracedGalaxyHierarchySource(
|
return new TracedGalaxyHierarchySource(
|
||||||
new GatewayGalaxyHierarchySource(_ownedRepositoryClient), _options.MxAccess.ClientName);
|
new GatewayGalaxyHierarchySource(_ownedRepositoryClient), _options.MxAccess.ClientName);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user