Refactor external call client ownership and helper split

[angelol]

Summary

This stacked PR moves HTTP client ownership from ExtensionServiceManager into per-extension resources owned by HttpExtensionServiceClient, and splits request construction and response/error mapping into small extension-local helpers.

It remains pre-OAuth and does not change config shape or public extension APIs.

What changed

• added HttpExtensionClientResources, HttpExtensionClientResourcesFactory, and JdkHttpExtensionClientResourcesFactory
• removed the manager-owned shared JDK HttpClient and injected a resources factory/runtime path for tests
• added HttpExtensionRequestBuilder and HttpExtensionResponseMapper
• kept retry orchestration in HttpExtensionServiceClient while moving request and response mechanics into local helpers
• added unit coverage for per-extension resource creation and JDK resource factory settings

Why

This is the next preparatory refactor for the OAuth work. It keeps the responsibility localized to the external-call extension while fixing one real issue in the current structure: connectTimeout and tlsInsecure now apply per extension instead of leaking through a shared manager-owned client.

Tests

Ran:

sb t -Dsbt.log.noformat=true "community-participant/testOnly com.digitalasset.canton.participant.extension.HttpExtensionServiceClientTest com.digitalasset.canton.participant.extension.ExtensionServiceManagerTest com.digitalasset.canton.participant.extension.JdkHttpExtensionClientResourcesFactoryTest com.digitalasset.canton.participant.extension.ExtensionServiceExternalCallHandlerTest"

[greptile-apps]

Greptile Summary

This PR moves HttpClient ownership from the single shared instance in ExtensionServiceManager into a per-extension HttpExtensionClientResources bundle, and splits the monolithic HttpExtensionServiceClient into three focused helpers: HttpExtensionRequestBuilder, HttpExtensionResponseMapper, and a new JdkHttpExtensionClientResourcesFactory. The structural refactor is clean and well-motivated — it correctly fixes the bug where a single shared HttpClient caused connectTimeout and tlsInsecure settings to leak across extensions.\n\nKey points:\n- The public constructor of ExtensionServiceManager and HttpExtensionServiceClient is preserved unchanged; the primary constructor is now private[extension], enabling clean test injection without altering the production API.\n- private lazy val resources in HttpExtensionServiceClient ensures resourcesFactory.create() is called exactly once per client instance — correctly verified by the new tests.\n- The insecure-TLS warning is now emitted lazily inside JdkHttpExtensionClientResourcesFactory.create(). When validateExtensionsOnStartup = false, the warning will not appear at startup and is silently deferred to the first actual HTTP call — a subtle behavioral change from the old eager-at-construction warning.\n- HttpClient instances created per extension have no lifecycle management (close() is never called), which matters on Java 21+ where HttpClient implements AutoCloseable. The per-extension design increases the count from 1 to N leaked instances.\n- Two small code-quality items: a redundant null check on a Scala String field in HttpExtensionResponseMapper, and a logically redundant createdResourceIds.distinct assertion in ExtensionServiceManagerTest.

Confidence Score: 5/5

Safe to merge — all findings are P2 style/cleanup suggestions with no blocking defects

All four findings are P2: a deferred security warning (non-blocking, only affects non-default config), a missing AutoCloseable pattern (pre-existing limitation, not worsened materially), a redundant null check, and a weak test assertion. No P0/P1 issues were found. The core invariant — one HttpClient per extension, created lazily and exactly once — is correctly implemented and well-tested.

HttpExtensionClientSupport.scala (deferred TLS warning + HttpClient lifecycle) and HttpExtensionResponseMapper.scala (null guard) are the files with minor improvements worth considering before the OAuth follow-up

Important Files Changed

Filename	Overview
community/participant/src/main/scala/com/digitalasset/canton/participant/extension/ExtensionServiceManager.scala	Removes the manager-owned shared HttpClient; adds a secondary public constructor for backward compatibility and injects resourcesFactory/runtime into the primary private[extension] constructor
community/participant/src/main/scala/com/digitalasset/canton/participant/extension/HttpExtensionClientSupport.scala	Introduces HttpExtensionClientResources, HttpExtensionClientResourcesFactory, JdkHttpExtensionClientSettings, and JdkHttpExtensionClientResourcesFactory; the insecure-TLS warning is now emitted lazily on first factory.create() call rather than eagerly at manager construction
community/participant/src/main/scala/com/digitalasset/canton/participant/extension/HttpExtensionRequestBuilder.scala	New file extracting request construction (endpoint, headers, JWT) from HttpExtensionServiceClient; no functional changes from moved logic
community/participant/src/main/scala/com/digitalasset/canton/participant/extension/HttpExtensionResponseMapper.scala	New file extracting response and exception mapping; contains a redundant null-guard on a Scala String body field
community/participant/src/main/scala/com/digitalasset/canton/participant/extension/HttpExtensionServiceClient.scala	Replaces the injected transport with a resourcesFactory; delegates to requestBuilder/responseMapper helpers; lazy val resources ensures one HttpClient per client instance
community/participant/src/test/scala/com/digitalasset/canton/participant/extension/ExtensionServiceManagerTest.scala	Adds RecordingResourcesFactory and two new tests confirming per-extension resource isolation and single-creation invariant
community/participant/src/test/scala/com/digitalasset/canton/participant/extension/HttpExtensionServiceClientTest.scala	Migrates all existing tests from transport injection to FakeResourcesFactory; adds a new test verifying the lazy val initializes the transport exactly once
community/participant/src/test/scala/com/digitalasset/canton/participant/extension/JdkHttpExtensionClientResourcesFactoryTest.scala	New test file for settingsFor — validates per-extension connect timeout and insecureTls flag derivation

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
  class ExtensionServiceManager {
    -clients Map~String ExtensionServiceClient~
    +handleExternalCall()
    +validateAllExtensions()
    +onClosed()
  }
  class HttpExtensionServiceClient {
    -resourcesFactory HttpExtensionClientResourcesFactory
    -requestBuilder HttpExtensionRequestBuilder
    -responseMapper HttpExtensionResponseMapper
    -resources HttpExtensionClientResources
    +call()
    +validateConfiguration()
  }
  class HttpExtensionRequestBuilder {
    -config ExtensionServiceConfig
    -endpoint URI
    +buildCallRequest()
    +buildValidationRequest()
  }
  class HttpExtensionResponseMapper {
    +mapResponse()
    +mapException()
    +retryAfter()
  }
  class HttpExtensionClientResourcesFactory {
    +create(config) HttpExtensionClientResources
  }
  class JdkHttpExtensionClientResourcesFactory {
    +create(config) HttpExtensionClientResources
  }
  class HttpExtensionClientResources {
    +resourceTransport HttpExtensionClientTransport
  }
  class HttpExtensionClientTransport {
    +send(request) HttpExtensionClientResponse
  }
  class JdkHttpExtensionClientTransport {
    -httpClient HttpClient
    +send(request) HttpExtensionClientResponse
  }
  ExtensionServiceManager --> HttpExtensionServiceClient : creates N
  HttpExtensionServiceClient --> HttpExtensionClientResourcesFactory : injected
  HttpExtensionServiceClient --> HttpExtensionRequestBuilder : owns
  HttpExtensionServiceClient --> HttpExtensionResponseMapper : owns
  HttpExtensionServiceClient --> HttpExtensionClientResources : lazily owns
  HttpExtensionClientResourcesFactory <|-- JdkHttpExtensionClientResourcesFactory
  HttpExtensionClientResources --> HttpExtensionClientTransport : contains
  HttpExtensionClientTransport <|-- JdkHttpExtensionClientTransport

Loading

Comments Outside Diff (1)

1. community/participant/src/test/scala/com/digitalasset/canton/participant/extension/ExtensionServiceManagerTest.scala, line 738-739 (link)

   createdResourceIds.distinct assertion is redundant

   createdResourceIds stores incrementing 1-based integers (1, 2, …), so every element is already unique. distinct is a no-op here, making the assertion equivalent to createCalls should have size 2, which is already asserted on the line above.

   To actually verify that each extension receives a different HttpExtensionClientResources instance (the real intent), consider comparing the transport objects or using object identity:

   val transports = resourcesFactory.createCalls.map(c => s"response-for-${c.name}")
   transports.distinct should have size 2

_{Reviews (1): Last reviewed commit: "Refactor external call client ownership" | Re-trigger Greptile}

[greptile-apps]

HttpClient instances have no lifecycle management

Each call to JdkHttpExtensionClientResourcesFactory.create() allocates a new java.net.http.HttpClient (which owns an internal thread pool and selector). Since Java 21, HttpClient implements AutoCloseable, and omitting close() prevents orderly shutdown of those background threads.

With the new per-extension design, ExtensionServiceManager.onClosed() now silently abandons N HttpClient instances instead of one. Consider having HttpExtensionClientResources (or HttpExtensionServiceClient) implement AutoCloseable / FlagCloseable so the manager can propagate onClosed() to each resource bundle:

private[extension] final class HttpExtensionClientResources(
    resourceTransport: HttpExtensionClientTransport
) extends AutoCloseable {
  override def close(): Unit = resourceTransport match {
    case c: AutoCloseable => c.close()
    case _ => ()
  }
}

[angelol]

Good point. We should track this, but I do not want to solve it inside this refactor with a Java-17-specific workaround. This repo is compiled with --release 17, and on that baseline java.net.http.HttpClient does not expose the Java-21 close() / AutoCloseable API shape, so there is no clean source-level full-client shutdown path here. The previous shared-client design also had no explicit HttpClient shutdown.

I have opened a follow-up ticket to handle this separately and narrowly: ZEN-21. The likely Java-17-compatible direction is to own and shut down any custom ExecutorService we introduce for per-extension clients, propagate that shutdown from ExtensionServiceManager.onClosed(), and document the remaining limitation that Java 17 still does not expose a public full HttpClient close API.

[angelol]

Addressed the Greptile summary note about the weak assertion in ExtensionServiceManagerTest: the fake resources factory now records the actual created resources and the test asserts that the created transports are distinct, so it checks per-extension isolation rather than a synthetic counter.