Skip to content

fix(consumer): close resources on constructor-failure paths#209

Merged
eschizoid merged 1 commit into
mainfrom
fix/resource-leak-construction-paths
Jun 20, 2026
Merged

fix(consumer): close resources on constructor-failure paths#209
eschizoid merged 1 commit into
mainfrom
fix/resource-leak-construction-paths

Conversation

@eschizoid

Copy link
Copy Markdown
Owner

What

Fixes three resource-leak risks surfaced by the resource-leak audit, all on construction paths where a partial init would otherwise strand already-opened resources for the JVM lifetime.

1. (MEDIUM) KPipeConsumer constructor leaks resources if a later field init throws

The constructor opens, in order: the Kafka consumer, the dispatcher (PARALLEL owns a virtual-thread executor), an optional built-in DLQ KPipeProducer, an optional scheduler, the batch wrappers, and a JVM shutdown hook. If anything after the first open threw — e.g. addShutdownHook while the JVM is already shutting down (IllegalStateException), or a user-supplied ConsumerMetrics/Tracer/CircuitBreakerController whose construction work throws — every already-opened resource leaked.

The constructor body is now wrapped so any throwable triggers closePartiallyConstructed(t), which closes whatever was opened in reverse of the open order (shutdown hook → scheduler → producer → batch wrappers → dispatcher → Kafka consumer), suppressing any cleanup failure onto the original throwable, then rethrows.

Ownership mirrors the existing teardown (releaseConstructedResources / the consumer-thread finally): the Kafka consumer and the DLQ producer are closed regardless of whether they were supplied via withConsumer(...) / withKafkaProducer(...) or built internally, because once handed to the consumer they are consumer-owned. The helper reads fields off this (a partially-constructed instance leaves not-yet-assigned fields at their default null), so every step is null-guarded. The whitespace-ignoring diff for this file is +71 lines (the try, the catch, and the helper); the rest of the large diff is pure re-indentation of the wrapped body.

This is the load-bearing fix.

2. (LOW) HttpHealthServer constructor leaks bound socket if createContext throws

HttpServer.create(addr, 0) binds the socket; if the subsequent createContext(path, ...) throws, the socket leaked. Now wrapped: try { createContext(...) } catch (RuntimeException e) { server.stop(0); throw e; }.

3. (LOW) HttpHealthServer.fromEnv Javadoc said "started" but never called start()

The Javadoc promised "a started HttpHealthServer instance", but fromEnv never started the server. The only caller is a test exercising the disabled path, so nothing started it on the caller side — the doc claim was a real bug. fromEnv now calls server.start() before returning, matching the documented contract (no double-start: start() is idempotent via a CAS guard).

Deferred

Scheduler awaitTermination finding (#4 in the audit) was considered and deliberately deferred. It is purely defensive — all scheduled tasks are cancelled before shutdownNow() today, so there is nothing to await — and adding an awaitTermination would only add shutdown latency for no behavioral gain.

Tests

  • KPipeConsumerMockingTest.constructorClosesKafkaConsumerWhenLaterInitStepThrows — supplies a real MockConsumer via withConsumer(...) plus a withOffsetManagerProvider(...) that throws (the provider runs after both the Kafka consumer and the dispatcher are open), asserts the thrown exception propagates unchanged, and asserts mockConsumer.closed() — proving the load-bearing Kafka-consumer close.
  • HttpHealthServerTest.shouldReturnStartedServerFromEnv — mocks the HealthConfig env reads to an ephemeral port and hits the endpoint over HTTP without calling start(), proving fromEnv started it.

No test was added for #2 (createContext failure): triggering it cleanly requires spying/subclassing HttpServer since createContext only throws on a duplicate/invalid path that normalizePath already prevents — a brittle test for a small, reviewable fix. Relying on review there.

Verification

./gradlew :lib:kpipe-consumer:test — 265 passing; the only 2 failures are the known DockerClientProviderStrategy Testcontainers initialization errors (environmental, no Docker in this sandbox).

The KPipeConsumer constructor opens, in order: the Kafka consumer, the
dispatcher (PARALLEL owns a virtual-thread executor), an optional built-in
DLQ producer, an optional scheduler, the batch wrappers, and a JVM shutdown
hook. If any step after the first open threw — e.g. addShutdownHook while the
JVM is already shutting down, or a user-supplied metrics/tracer/circuit-breaker
whose construction work throws — every already-opened resource leaked for the
JVM lifetime.

Wrap the constructor body so any throwable triggers reverse-order cleanup of
whatever was opened (shutdown hook, scheduler, producer, batch wrappers,
dispatcher, Kafka consumer), then rethrow with cleanup failures suppressed onto
the original. Ownership mirrors the existing teardown path: the Kafka consumer
and DLQ producer are closed regardless of whether they were supplied or built
internally, because once handed to the consumer they are consumer-owned.

Also harden HttpHealthServer:
- createContext failure now closes the already-bound socket before propagating.
- fromEnv now starts the returned server, matching its Javadoc contract (no
  caller started it, so the doc claim of "a started instance" was a real bug).

Tests: constructorClosesKafkaConsumerWhenLaterInitStepThrows drives a throwing
offset-manager provider through build() and asserts the Kafka consumer was
closed; shouldReturnStartedServerFromEnv hits the fromEnv endpoint without
calling start() to prove it was started.
@eschizoid

Copy link
Copy Markdown
Owner Author

@copilot please review

@codecov

codecov Bot commented Jun 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.63768% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.36%. Comparing base (4ccac98) to head (099b25e).
⚠️ Report is 16 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...github/eschizoid/kpipe/consumer/KPipeConsumer.java 75.38% 21 Missing and 11 partials ⚠️
...ithub/eschizoid/kpipe/health/HttpHealthServer.java 62.50% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #209      +/-   ##
============================================
+ Coverage     77.82%   78.36%   +0.54%     
- Complexity      732      755      +23     
============================================
  Files            68       66       -2     
  Lines          2800     2866      +66     
  Branches        346      366      +20     
============================================
+ Hits           2179     2246      +67     
+ Misses          462      454       -8     
- Partials        159      166       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

eschizoid added a commit that referenced this pull request Jun 20, 2026
Four documentation hardenings from the R1 convergence review of the
audit-fix PRs (#205-#210). All comment/javadoc only — zero behavior
change.

- AvroFormat.deserializeStatic: explain why the catch is broadened to
  RuntimeException (Avro surfaces malformed bytes as AvroRuntimeException,
  not IOException) and that the original cause is always attached so a
  genuine programming error stays visible — not swallowed. (#207)
- KPipeConsumer.closePartiallyConstructed: explain why reading the final
  fields compiles — the cleanup runs in a separate method, so
  definite-assignment analysis doesn't guard the read and an unreached
  blank-final reads its default null. Warns against "fixing" the
  apparent unassigned-final read by dropping final. (#209)
- MessageProcessorRegistry: document the one unbounded-cardinality case
  for the unrouted-key dedup set (dynamically-generated keys) and the
  remedy (cap / bounded LRU) if it ever becomes real. (#210)
- OffsetManagerBoxingBenchmark: drop the JVM-version-specific
  object-header byte estimate; point the reader at the measured
  gc.alloc.rate.norm delta instead (prove, don't assert). (#205)
@eschizoid eschizoid deleted the fix/resource-leak-construction-paths branch June 22, 2026 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant