feat: make LLMClient extend AutoCloseable for resource safety

[krrish175-byte]

What does this PR do?

LLMClient has a close() method but doesn't implement AutoCloseable, preventing idiomatic resource management via scala.util.Using. This PR makes LLMClient extend AutoCloseable and ensures all provider clients actually shut down their underlying HTTP clients on close(), preventing resource leaks (thread pools, connections).

Changes:

• LLMClient now extends AutoCloseable (backward-compatible - close(): Unit already existed)

• Created HttpClientCloser utility that uses reflection to call HttpClient.close() on JDK 21+ and gracefully no-ops on older JDKs (project currently runs on JDK 20)

• Updated all 8 provider close() methods:

  1. 6 JDK HttpClient providers (Gemini, DeepSeek, Cohere, Ollama, OpenRouter, Zai) now call HttpClientCloser.tryClose(httpClient)
  2. AnthropicClient now calls client.close() on its OkHttpClient
  3. OpenAIClient unchanged (Azure SDK has no closeable contract)

• Added LLMClientAutoCloseableSpec (4 tests) and HttpClientCloserSpec (2 tests)

##Related issue:

Fixes resource leak: LLMClient instances could not be used with scala.util.Using or try-with-resources, and provider close() methods were no-ops that didn't release underlying HTTP connections.

How was this tested?

sbt "core/testOnly *LLMClientAutoCloseableSpec* *HttpClientCloserSpec*"
# 6 tests passed
sbt "core/testOnly *LLMClientRetrySpec* *ClientStatusSpec*"
# 24 existing tests passed (regression check)
sbt core/compile
# Clean compilation, zero errors

Checklist:

• I have read the Contributing Guide
• PR is small and focused - one change, one reason
• sbt scalafmtAll - code is formatted
• sbt +test - tests pass on both Scala 2.13 and 3.x
• New code includes tests
• No unrelated changes included (branched from main, not from another PR)
• Commit messages explain the "why"

Closing issue:

closes issue #685

[github-actions]

🔒 Claude Code Review Status

Thank you for your contribution! This PR is from an external repository, so automated Claude review is disabled for security reasons.

For maintainers: To get Claude review for this PR, comment @claude and I'll trigger a manual review.

PR Summary:

• Files changed: 11
• Additions: +124
• Deletions: -14
• Author: @krrish175-byte

[codecov]

Codecov Report

❌ Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...a/org/llm4s/llmconnect/provider/CohereClient.scala	50.00%	1 Missing ⚠️
...org/llm4s/llmconnect/provider/DeepSeekClient.scala	50.00%	1 Missing ⚠️
...a/org/llm4s/llmconnect/provider/GeminiClient.scala	0.00%	1 Missing ⚠️
...g/llm4s/llmconnect/provider/OpenRouterClient.scala	50.00%	1 Missing ⚠️
...cala/org/llm4s/llmconnect/provider/ZaiClient.scala	50.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[rorygraves]

The core direction is right — LLMClient extends AutoCloseable is a clean, backward-compatible improvement that enables scala.util.Using, and the Using-based tests in LLMClientAutoCloseableSpec are well-written. Two issues to fix before merge.

Must Fix

1. HttpClientCloser uses reflection — banned by project convention (#505)

httpClient.getClass.getMethod("close").invoke(httpClient) is reflection. The project CI already targets JDK 21, and java.net.http.HttpClient implements java.io.Closeable natively since JDK 21 — no reflection needed.

Replace HttpClientCloser with a direct call in each provider:

// Direct call — works on JDK 21+ (our CI target):
override def close(): Unit =
  if (closed.compareAndSet(false, true)) {
    httpClient.close()
  }

Or, if backward compatibility with pre-21 JDKs is a goal, a safe runtime cast avoids reflection entirely:

httpClient match {
  case c: java.io.Closeable => c.close()
  case _                    => ()
}

Either approach eliminates the HttpClientCloser utility and the HttpClientCloserSpec (which only tested the reflection path). The .recover { case _: NoSuchMethodException => () } also silently swallows InvocationTargetException and IllegalAccessException — a further reason to remove the reflection path entirely.

2. Unsafe result.get in LLMClientAutoCloseableSpec

result.get shouldBe Right(stubCompletion)   // .get throws on Failure

result is a scala.util.Try — calling .get on a Failure throws, giving an unhelpful exception rather than a clear test failure. Use result.value (mix in TryValues) or pattern-match instead.

---

Should Fix

3. "idempotent close" test checks stub behaviour, not real contract

it should "support idempotent close (multiple calls)" in {
  stub.close()
  stub.close()
  stub.closeCount shouldBe 2   // stub increments twice
}

All real provider implementations guard with compareAndSet(false, true) — they release the underlying resource exactly once no matter how many times close() is called. The stub has no such guard, so this test is verifying stub wiring, not the contract implied by the name. Either test a real client, or rename to something like "close() can be called multiple times without error".

4. OpenAIClient.close() remains a silent no-op

After this PR, users who wrap an OpenAIClient in scala.util.Using will silently get no resource cleanup. Worth adding a comment explaining why (Azure SDK has no Closeable contract) so it's clear this is intentional, not an oversight.

---

The fix for issue 1 is a small mechanical change (replace HttpClientCloser.tryClose(httpClient) with httpClient.close() in 6 providers and delete the utility). Once that's done and result.get is fixed, this should be ready to merge.

[rorygraves]

PR Review: #686 - feat: make LLMClient extend AutoCloseable (Round 2 Follow-up)

Status: Approved — Ready to Merge

All Round 1 issues have been addressed:

Issue	Severity	Status
HttpClientCloser uses reflection	Must Fix	✅ Replaced with safe (httpClient: Any) match { case c: AutoCloseable => c.close() ... }
Unsafe result.get in tests	Must Fix	✅ Changed to result shouldBe Success(Right(stubCompletion))
"idempotent close" test name misleading	Should Fix	✅ Renamed to "be safe to call close() multiple times"
OpenAIClient silent no-op undocumented	Should Fix	✅ Added comment explaining Azure SDK has no Closeable contract

The pattern-match approach for the 6 JDK HttpClient providers is a clean solution: widening to Any suppresses unchecked warnings while the runtime check correctly calls close() on JDK 21+ (where HttpClient implements AutoCloseable) and no-ops on earlier JDKs. All 7 functional CI checks pass.