fix(ilp): fix a leaked socket and native memory when an HTTP sender fails to start

[puzpuzpuz]

Found while reviewing questdb/questdb#7434, which made the equivalent sender
construction path exception-safe in the core repository. The ILP HTTP sender
uses this repository's HttpClientFactory, so it needs the same ownership
guarantee here.

Problem

An HTTP sender can fail after acquiring a client but before the sender escapes
to its caller. In particular, newRequest() can throw while writing the request
preamble. The same risk exists when the OIDC token provider is installed and the
factory rebuilds the initial request. In both cases, the client, its socket and
its native buffers were left without an owner that could close them.

HttpHeaderParser also allocated its DirectUtf8Sink in a field initializer.
Because field initializers run before the constructor body, a later header-buffer
allocation failure could strand the sink outside the constructor's cleanup path.

Finally, if closing a staged socket during HttpClient construction failed, that
cleanup failure could replace the exception that explained why construction
failed.

Changes

• Move the parser sink allocation into the constructor's guarded region so both
  native allocations are released if construction fails.
• Preserve the primary HttpClient construction exception and attach any socket
  close failure as suppressed.
• Close the owned client if the sender constructor fails while creating its
  initial request.
• Apply the same rollback when rebuilding the initial request after wiring an
  OIDC token provider.

There is no happy-path behavior change. A client supplied to a sender is now
closed if sender construction fails, matching the existing ownership contract on
the successful path, where closing the sender also closes that client.

[jerrinot]

Review (level 3): approve. Nothing blocking, and no Moderate or Minor findings.

What I checked

• Supplied-client cleanup: the only caller that passes an existing client into the sender constructor is the version-detection path in createLineSender, and it has no cleanup of its own around that call. The new rollback in AbstractLineHttpSender fixes the leak without any double close.
• Token provider: passing it into the constructor reaches the same end state as before (token pending, request stopped after the headers), and drops a redundant second client.newRequest() during setup.
• Other files: HttpHeaderParser, HttpClient, QwpWebSocketSender and SenderPool keep the same cleanup order. Misc.freeSuppressing behaves exactly like the SenderPool.addSuppressed helper it replaces. Java 8 floor respected; no change on the steady-state send path.
• Tests: LineHttpSenderConstructorTest, HttpClientConstructorTest, HttpHeaderParserTest, MiscTest and LineHttpSenderTokenProviderTest pass on head (5b450b7). testHandedInClientIsReleasedWhenRequestPreambleDoesNotFit asserts the socket is closed twice, so it would fail if the rollback were removed.

Scope

• No submodule pointer moves.
• No tandem questdb/questdb-enterprise PR needed: the change only affects cleanup when a sender fails to start, which is not server-observable and is unit-testable here.

[mtopolnik]

[PR Coverage check]

😍 pass : 38 / 39 (97.44%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/std/Misc.java	8	9	88.89%
🔵	io/questdb/client/cutlass/http/client/HttpClient.java	1	1	100.00%
🔵	io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java	1	1	100.00%
🔵	io/questdb/client/cutlass/http/HttpHeaderParser.java	14	14	100.00%
🔵	io/questdb/client/impl/SenderPool.java	1	1	100.00%
🔵	io/questdb/client/cutlass/line/http/AbstractLineHttpSender.java	13	13	100.00%

[bluestreak01]

PR #78 review: HTTP sender and client constructor rollback (level 3)

Verdict: approve with comments. Nothing blocks the merge and the test gate passes. One Moderate item should be fixed before merge: the change removes some released public constructor signatures without keeping the old ones. The rollback fix itself is correct, and I found no issues with it.

Critical

None.

Moderate

M1. Released multi-host sender constructors were changed in place instead of overloaded

• Problem: four constructors from the 1.3.9 release no longer exist. There is no overload with the old parameter list.
• Net impact: any outside code that calls these constructors directly, or subclasses these senders, stops compiling or linking. I found no such callers.
• Evidence:
  • Release 1.3.9 (git show 1.3.9:…) has these 17-argument constructors, ending (…, int currentAddressIndex, Rnd rnd):
    • LineHttpSenderV2 and LineHttpSenderV3: public.
    • AbstractLineHttpSender and LineHttpSenderV1: protected. The classes are public and not final, so subclasses can call them.
  • At head 5b450b7, javap -protected lists only the 18-argument forms, which add HttpTokenProvider at the end.
  • A small caller that uses the old LineHttpSenderV2 constructor compiles against base 916e51f (javac exit 0). Against head it fails with "no suitable constructor found".

Where it is:

• core/src/main/java/io/questdb/client/cutlass/line/http/AbstractLineHttpSender.java:168-187
• LineHttpSenderV1.java:82-99
• LineHttpSenderV2.java:86-105
• LineHttpSenderV3.java:46-65

module-info.java:52 exports io.questdb.client.cutlass.line.http. The repo's own rules apply here:

• ExportedApiCompatibilityTest says: "Adding an overload is fine … retyping or removing one turns it red". It also explains that a break with no known callers was still restored in the past.
• createLineSender keeps a provider-less overload "so callers compiled against the pre-httpTokenProvider signature keep linking".

This PR goes against that. The test doesn't catch it because it only checks createLineSender and connect, not constructors.

Why Moderate, not Critical: I searched the questdb and questdb-enterprise checkouts and found no callers. They only use the 13-argument single-host LineHttpSenderV2 constructor, which is unchanged. The removed constructors also take internal plumbing types (HttpClient, Rnd, currentAddressIndex). So I can't name anyone who is actually affected.

Suggested fix: add the four 17-argument constructors back as overloads that pass null for the provider. Also add the constructor signatures to ExportedApiCompatibilityTest, for example with a constructor variant of assertSignaturesPresent.

Minor

None.

Coverage map

Test gate: pass. No coverage gaps admitted.

Every behaviour change has a local test that fails without the fix. I ran the new tests on JDK 8 at head (all pass) and at base with the tests adapted to compile there (base 916e51f):

New test	Result at base
HttpHeaderParserTest.testConstructorFailureFreesNativeAllocations	fails: 32 B leaked under NATIVE_DIRECT_UTF8_SINK
LineHttpSenderConstructorTest.testHandedInClient…	fails: socket closed once, expected twice
LineHttpSenderConstructorTest.testProviderClient…	fails: leak under NATIVE_DEFAULT
LineHttpSenderConstructorTest.testSelfBuiltClient…	fails: leak under NATIVE_DEFAULT
HttpClientConstructorTest.testBaseConstructorFailurePreservesOriginalFailure…	errors: the socket close failure replaces the original exception

The wider run (*Http*, *LineSender*, *SenderPool*, *TokenProvider*, Misc*) passed at head: 649 tests, 0 failures. The code also compiles and test-compiles on JDK 25.

No tandem PR is needed. The change only affects what happens when a sender fails to start, which a server can't observe and unit tests here can cover. I searched for tandem PRs on the puzpuzpuz_http_ctor_rollback branch: there are none in questdb or questdb-enterprise.

Summary

• Submodules: no submodule pointer moves.
• Scope of the finding: M1 is an in-diff issue with no breakage outside the diff. I checked every caller of the changed symbols across this repo and the local questdb/questdb-enterprise checkouts.
• Rollback fix:
  • The client a sender is handed is closed exactly once. createLineSender has no cleanup of its own around the constructor switch, so there is no double close.
  • Passing the token provider through the constructor leaves the request in the same state as the old rebuild. The only difference is one fewer client.newRequest() call.
  • Misc.freeSuppressing behaves the same as the old SenderPool helper. It is slightly safer in QwpWebSocketSender, where it can no longer throw on self-suppression.
  • Nothing changes on the per-row or flush path.
• Findings: 0 Critical, 1 Moderate (M1), 0 Minor.

Before approving, I'd ask the author to add the constructor overloads from M1.