From 75737e4538b3a3bd74c359621a7d572b26db06b9 Mon Sep 17 00:00:00 2001 From: Muhannad Al-Khatib Date: Tue, 25 Aug 2026 15:53:35 +0300 Subject: [PATCH] fix: reuse Azure and Oracle clients across requests; document DI lifetime audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prompted by the S3 client-lifetime incident fixed in 8.1.11, audited the other network-backed providers for the same class of bug (building a fresh SDK client on every DI resolution instead of reusing a pooled, thread-safe one): - Azure (SW.CloudFiles.AS): same AddTransient bug as S3 had, and worse — the DI extension registered a singleton BlobContainerClient that was never actually used, since CloudFilesService's constructor built its own client instead of taking one as a parameter. In the non-managed- identity auth path that means a synchronous, blocking GetBlobContainers list call on every single construction. Fixed: CloudFilesService now takes BlobContainerClient via constructor injection (reusing the registered singleton), and ICloudFilesService is now AddSingleton. - Oracle (SW.CloudFiles.OC): was AddScoped, so a fresh ObjectStorageClient + UploadManager (and a disk read of the PEM/config file) were built once per HTTP request rather than once per resolution. Changed to AddSingleton so the client is built once for the process lifetime. - Google Cloud (SW.CloudFiles.GC): already correct — ICloudFilesService is AddScoped, but the actual StorageClient/UrlSigner are AddSingleton and injected, so no repeated client construction. No change needed. No known consumer currently uses Azure/GCS/Oracle (only S3 is actively used today), so this is library-quality debt rather than a live incident, unlike the S3 fix. Documented the full audit in CLAUDE.md and added the client-reuse/timeout notes to README.md. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 15 +++++++++++++++ README.md | 10 ++++++++++ .../IServiceCollectionExtensions.cs | 2 +- SW.CloudFiles.AS/CloudFilesService.cs | 4 ++-- .../IServiceCollectionExtensions.cs | 2 +- 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1326247..e2a9348 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,6 +111,21 @@ S3 and GCS also auto-create the bucket if it does not exist. `AddLocalTestsCloudFiles` registers `CloudFilesService` (concrete) as a singleton **in addition to** the `ICloudFilesService` interface alias, so test classes can inject `CloudFilesService` directly and call `Cleanup()` in teardown. Storage root defaults to `Path.GetTempPath()/SW.CloudFiles.LocalTests/{BucketName}` via `LocalTestsCloudFilesOptions.ResolvedStoragePath`. Metadata is persisted as sidecar `.meta.json` files alongside each stored blob. `GetSignedUrl` returns the same `file://` URI as `GetUrl`. `OpenWrite` throws `NotImplementedException`. +## Known Issues / DI Lifetime Audit (2026-08-25) + +A production incident on 2026-08-25 (invoice-attachment and shipment-label uploads hanging ~1 minute then failing) traced back to the S3 provider building a brand-new `AmazonS3Client` on every DI resolution (`ICloudFilesService` was `AddTransient`) with no explicit timeout, so any latency to the storage endpoint compounded through the AWS SDK's default ~100s timeout and several backoff retries. Fixed in 8.1.11 (S3 provider): `ICloudFilesService` is now `AddSingleton`, and `S3CloudFilesOptions.TimeoutSeconds`/`MaxErrorRetry` (defaults 15/2) are configurable via the `CloudFiles` config section or the `AddS3CloudFiles(options => ...)` delegate. + +Prompted by that incident, the other three network-backed providers were audited for the same class of bug (fresh client construction on every resolution instead of reusing a pooled, thread-safe SDK client): + +- **Azure (`SW.CloudFiles.AS`) — same bug, and worse. Fixed alongside S3.** `ICloudFilesService` was also `AddTransient`, and `CloudFilesService`'s constructor called `cloudFilesOptions.CreateClient()` itself rather than using the `BlobContainerClient` the DI extension registered as a singleton — that singleton registration was dead code, never actually injected anywhere. Worse than the S3 case: in the non-managed-identity (shared-key) auth path, `CreateClient()` does a **synchronous, blocking `GetBlobContainers()` list call** plus an existence check — a real network round-trip, not just a fresh TCP/TLS handshake — on every single construction. Fixed: `CloudFilesService` now takes `BlobContainerClient` as a constructor parameter (reusing the singleton), and `ICloudFilesService` is registered `AddSingleton`. +- **Oracle (`SW.CloudFiles.OC`) — milder version, fixed alongside S3.** Was `AddScoped` (so only once per HTTP request rather than per resolution, unlike S3/Azure's `AddTransient`), but the constructor still built a fresh `ObjectStorageClient` + `UploadManager` per request scope, including a `ConfigFileAuthenticationDetailsProvider` re-reading the PEM/config file from disk each time. Fixed: registered `AddSingleton` instead, so the client is built once for the process lifetime. +- **Google Cloud (`SW.CloudFiles.GC`) — already correct, no change needed.** `ICloudFilesService` is `AddScoped`, but the actual `StorageClient`/`UrlSigner` doing the I/O are `AddSingleton` and injected in — no repeated client construction regardless of the wrapper service's lifetime. This is the reference-correct pattern the other providers now follow. +- **LocalTests** — file-based, no network client involved, not applicable. + +None of Azure/GCS/Oracle are referenced by any Traxis service (only the S3 provider is, via `AddS3CloudFiles()` in all 8 backend microservices) — this was pure library-quality debt with no live-incident urgency, unlike the S3 fix. + +Note: `SW.CloudFiles.AS.UnitTest` and `SW.CloudFiles.OC.UnitTest` fail locally regardless of these changes (confirmed by reverting and re-running) — they appear to be integration tests requiring live cloud credentials, consistent with CI's `run-tests: 'false'` in `nuget-publish.yml`. Not a regression from this audit. + ## Key Dependencies - `SimplyWorks.PrimitiveTypes` v8.1.3 — shared interface and base types (all providers) diff --git a/README.md b/README.md index 8fbb69c..765af58 100644 --- a/README.md +++ b/README.md @@ -327,11 +327,20 @@ All four providers support `GetSignedUrl(key, expiry)`: ### S3-Compatible Storage - Works with AWS S3, DigitalOcean Spaces, MinIO, and any S3-compatible service. - Bucket is created automatically if it does not exist. +- The underlying `AmazonS3Client` is built once and reused for the life of the process (`ICloudFilesService` is registered as a singleton) rather than per call. +- `TimeoutSeconds` (default `15`) and `MaxErrorRetry` (default `2`) on `S3CloudFilesOptions` bound the client's per-request timeout and automatic retry count, so a genuine outage on the storage endpoint fails fast instead of hanging on the AWS SDK's much longer defaults: + ```csharp + services.AddS3CloudFiles(o => { + o.TimeoutSeconds = 20; + o.MaxErrorRetry = 1; + }); + ``` ### Azure Blob Storage - Container is created automatically if it does not exist. - **Managed Identity**: Set `Managed = true`. Optionally set `ManagedIdentityClientId` for a user-assigned identity; omit it to use the system-assigned identity or ambient `DefaultAzureCredential`. - **Public URL override**: If you connect via private link (`ServiceUrl`) but need public-facing URLs, set `PublicServiceUrl` to the standard public endpoint (e.g. `https://account.blob.core.windows.net`). +- The underlying `BlobContainerClient` is built once and reused for the life of the process (`ICloudFilesService` is registered as a singleton) rather than per call. ```csharp // Managed Identity example @@ -352,6 +361,7 @@ services.AddAsCloudFiles(o => { ### Oracle Cloud Storage - OCI credentials (`UserId`, `TenantId`, `FingerPrint`, `RSAKey`) are written to temporary files on startup and used to authenticate via `ConfigFileAuthenticationDetailsProvider`. - `GetSignedUrl` creates a Pre-Authenticated Request (PAR) with read-only access. +- The underlying `ObjectStorageClient` is built once and reused for the life of the process (`ICloudFilesService` is registered as a singleton) rather than per request. ### Local Filesystem (Testing / Local Development Only) diff --git a/SW.CloudFiles.AS.Extensions/IServiceCollectionExtensions.cs b/SW.CloudFiles.AS.Extensions/IServiceCollectionExtensions.cs index 5971fde..021f95e 100644 --- a/SW.CloudFiles.AS.Extensions/IServiceCollectionExtensions.cs +++ b/SW.CloudFiles.AS.Extensions/IServiceCollectionExtensions.cs @@ -33,7 +33,7 @@ public static IServiceCollection AddAsCloudFiles(this IServiceCollection service serviceCollection.AddSingleton(blobContainerClient); serviceCollection.AddSingleton(cloudFilesOptions); serviceCollection.AddSingleton(cloudFilesOptions); - serviceCollection.AddTransient(); + serviceCollection.AddSingleton(); return serviceCollection; } } \ No newline at end of file diff --git a/SW.CloudFiles.AS/CloudFilesService.cs b/SW.CloudFiles.AS/CloudFilesService.cs index 12e096d..d8cadc5 100644 --- a/SW.CloudFiles.AS/CloudFilesService.cs +++ b/SW.CloudFiles.AS/CloudFilesService.cs @@ -14,10 +14,10 @@ namespace SW.CloudFiles.AS; /// Azure Blob Storage implementation of . -public class CloudFilesService(AzureCloudFilesOptions cloudFilesOptions) : IDisposable, ICloudFilesService +public class CloudFilesService(AzureCloudFilesOptions cloudFilesOptions, BlobContainerClient blobContainerClient) : IDisposable, ICloudFilesService { private readonly AzureCloudFilesOptions cloudFilesOptions = cloudFilesOptions; - private readonly BlobContainerClient blobContainerClient = cloudFilesOptions.CreateClient(); + private readonly BlobContainerClient blobContainerClient = blobContainerClient; /// public async Task WriteAsync(Stream inputStream, WriteFileSettings settings) diff --git a/SW.CloudFiles.OC.Extensions/IServiceCollectionExtensions.cs b/SW.CloudFiles.OC.Extensions/IServiceCollectionExtensions.cs index 6f36931..ada0983 100644 --- a/SW.CloudFiles.OC.Extensions/IServiceCollectionExtensions.cs +++ b/SW.CloudFiles.OC.Extensions/IServiceCollectionExtensions.cs @@ -51,7 +51,7 @@ public static IServiceCollection AddOracleCloudFiles(this IServiceCollection ser if (!cloudFilesOptions.DisableAutoLifecycle) EnsureLifecycleRules(cloudFilesOptions); - serviceCollection.AddScoped(); + serviceCollection.AddSingleton(); serviceCollection.AddSingleton(cloudFilesOptions); serviceCollection.AddSingleton(cloudFilesOptions); return serviceCollection;