Repository navigation
Fix Cosmos HTTP client lifecycle across sources, sinks and Operations - #257
Conversation
Return per-client HTTP wrappers over transport-specific connection pools and dispose source/sink clients at operation boundaries. Cover lifecycle, proxy/TLS behavior, pooling and sequential Operations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba9dddb-0a91-4f8c-b304-ef21961f51c6
Inject an offline TokenCredential into all RBAC client-construction tests, including existing diagnostic tests. Assert the SDK eagerly requests the endpoint scope; leave production credential selection unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba9dddb-0a91-4f8c-b304-ef21961f51c6
|
Phil Nachreiner (@philnach) This PR is ready to merge once the required approval is recorded. I reviewed head Today's local Cosmos suite passed 129 tests, including 22 lifecycle cases, with only the two optional local-emulator tests skipped. All ten PR checks are successful at this head; the existing Linux emulator/guard and Windows emulator runs also pass. CI was rechecked, not newly rerun today. The documented limits remain: no live Entra authorization or enterprise proxy negotiation was exercised. This PR shares Cosmos sink/test/docs files with #254 and #255, so preserve the independent fixes and recheck CI as those changes land. |
|
Copilot resolve the merge conflicts in this pull request |
…-http-client-lifecycle # Conflicts: # Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension.UnitTests/CosmosDataSourceExtensionTests.cs # Extensions/Cosmos/README.md Co-authored-by: markjbrown <800166+markjbrown@users.noreply.github.com>
Resolved the conflicts and merged |
There was a problem hiding this comment.
🟡 Changes recommended
The sink documentation still gives contradictory proxy authentication guidance and incorrectly suggests pre-authentication avoids the first challenge.
1 open issue
What changed in this PR
Fixes the Cosmos HTTP client lifecycle issue reported in #223 by separating SDK-owned clients from shared connection pools, allowing sequential Operations to initialize clients safely.
Changes:
- Creates fresh
HttpClientwrappers over shared handlers configured for proxy, credentials, and TLS settings. - Disposes source and sink Cosmos clients at their enumeration or operation boundaries.
- Adds offline lifecycle coverage and sequential emulator migration tests, and clarifies connection-setting documentation.
| File | Description |
|---|---|
| Extensions/Cosmos/README.md | Documents transport ownership, authentication settings, and regression coverage. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension/CosmosExtensionServices.cs | Separates client wrappers from pooled handlers and adds an internal credential-injection seam. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension/CosmosDataSourceExtension.cs | Disposes the Cosmos client when enumeration ends. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension/CosmosDataSinkExtension.cs | Scopes Cosmos client disposal to the write operation. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension.UnitTests/OfflineTokenCredential.cs | Provides a credential fake that rejects token acquisition locally. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension.UnitTests/CosmosHttpClientLifecycleTests.cs | Covers lifecycle isolation, pooling, disposal, proxy routing, and TLS behavior. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension.UnitTests/CosmosDataSourceExtensionTests.cs | Extends emulator coverage with sequential Cosmos-to-Cosmos operations. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension.UnitTests/CertificateConfigurationTests.cs | Disposes test clients and isolates RBAC construction from live credentials. |
Review effort: Balanced
Reuse the two Operations destinations across pre-auth variants so the merged integration suite fits the current emulator's ten partitions. Retain all operations and assertions, and log command failures for diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba9dddb-0a91-4f8c-b304-ef21961f51c6
Keep client disposal and all partition-key assertions while reusing transport pools across repeated sink operations. Retain the separate SDK-managed transport JSON round trip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba9dddb-0a91-4f8c-b304-ef21961f51c6

Summary
Closes #223.
Return a fresh
HttpClientfor each Cosmos SDK initialization instead of reusing a process-wide mutable wrapper. Reuse process-ownedSocketsHttpHandlerconnection pools keyed by proxy URL, proxy credentials, pre-authentication and TLS validation settings. SDK-owned wrappers usedisposeHandler: false; source and sink Cosmos clients are disposed at their operation/enumeration boundary, including encryption wrappers.The custom handler explicitly applies proxy and TLS options: SDK 3.49 rejects
WebProxytogether withHttpClientFactoryand skips its HTTP-handler certificate setup when a factory is supplied. Default certificate validation remains enabled; the opt-out warning and Direct-mode callback remain intact. Default network credentials are distinct from proxy credentials and not required for RBAC. No production auth-flow, retry or partition-key rewrite is included.Review fixes and emulator investigation — October 31
Preserved the contributor's main-branch merge (
335ce22). Corrected the remaining sink proxy/pre-authentication guidance and resolved the Copilot review thread.The merged integration suite allocated twelve containers against the current Linux emulator's ten-partition limit; the original failed run returned 503/1007 on container creation. The Operations fixture now reuses two destinations across both pre-authentication variants, keeping the suite at ten containers. All sequential operations and read-back assertions remain; upsert ensures both variants perform real writes. Command failures now log their underlying errors instead of only reporting exit code 1.
A separate native crash then occurred during the earlier partition-key tests on three attempts of head
418c450:cosmosdb-emulatorreported a fatal error insqlpal.dll, exited with code 1, and subsequent requests failed with connection refused. Docker did not report an OOM kill. The repeated partition migration fixture was exercising fresh SDK-owned connection pools for each disposed sink client.Commit
2186d16runs that repeated migration fixture through this PR's shared HTTP transport (UseDefaultCredentials=true) while retaining scoped CosmosClient disposal and every partition-key assertion. With the same emulator image and partition configuration, the full current Linux suite now passes on its first attempt. This supports connection-pool teardown/churn as a trigger; it does not identify or repair the emulator's internal native defect. The separate JSON migration still exercises SDK-managed transport without default credentials, and offline lifecycle tests retain no-custom-factory controls. No test cases, assertions, required checks, or failure guards were removed or weakened.Current exact-head validation
All PR checks and the separate Windows emulator run passed on
2186d16888217d937a79c3cad1dfa8ef03a424eb.Downloaded emulator TRX artifacts confirm these counts and successful Operations coverage. Local validation before the final two-line fixture adjustment passed 284 tests with seven emulator tests skipped; the final-head build and emulator runs above compiled and exercised that adjustment.
Root cause and ownership evidence
The SDK invokes the factory and assigns
HttpClient.Timeoutand default headers during construction. Reusing a started wrapper causes the reportedInvalidOperationException, with either RBAC or key authentication. The SDK also disposes factory-returned clients.Pools survive individual client disposal; connections recycle after five minutes for DNS refresh. Cookies are disabled to avoid cross-client state. Keys/tokens and HTTP headers are not cached in handler configuration.
Regression coverage
HttpClient.set_Timeout -> CosmosHttpClientCore.CreateHelper -> DMT.CreateClientexception after a pre-cancelled loopback request.Operationswith both pre-authentication settings.Historical validation on pre-main-merge head
480e3097f291abf1e80ff8035854047d398a39a6: all eleven jobs passed, with 131/131 emulator tests on each platform: build/tests, packaging, Linux, Windows.Limits and merge context
These tests do not validate live Entra roles/token acquisition or enterprise proxy credential negotiation. Emulator migrations use keys; offline RBAC tests validate construction/transport lifetime, not live authorization. Bulk execution is not exercised by the emulator round trips. The existing Cosmos Encryption 2.0.4 / Cosmos SDK 3.49 NU1608 warning remains. Native current-emulator stability under rapid SDK-managed pool creation/disposal remains a limitation, not a claimed vendor-engine fix.
Upstream retry and partition-key changes incorporated by the contributor's main merge are preserved. No cloud provisioning, credential-state changes, release/deployment/publishing, approval, merge or manual issue-closure actions were performed.