Fix Paho client cleanup after failed startup

[nkokitkar]

Force-close internally owned MQTT v3 and v5 clients after startup or shutdown failures while preserving shared-client ownership. Keep the primary lifecycle exception and suppress any close failure.

Description

Target

• I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

• If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

• I checked that each commit in the pull request has a meaningful subject line and body.

• I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

AI-assisted contributions

• If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., Co-authored-by trailers) and the PR description identifies the AI tool used.

[davsclaus]

Claude Code on behalf of davsclaus

Thanks for the contribution — the client leak on startup failure is a real bug and the fix pattern (try-catch-finally with addSuppressed) is well done. Good test coverage too.

Findings

1. Missing JIRA ticket (Medium)

Per project conventions, bug fixes should have a JIRA issue filed. This is a non-trivial fix across 6 files — please create a JIRA ticket and reference it in the PR title/commits (e.g. CAMEL-XXXXX: Fix Paho client cleanup after failed startup).

2. Producer classes have the same bug (Medium)

PahoProducer and PahoMqtt5Producer have the identical lifecycle leak — if client.connect() throws in doStart(), the MqttClient instance leaks its thread pool and file-based persistence. Was the scope intentionally limited to consumers? If so, please file a follow-up JIRA to track the producer fix.

3. catchThrowableOfType parameter order (Low)

The catchThrowableOfType(callable, type) two-argument overload was deprecated in AssertJ 3.22+ in favor of catchThrowableOfType(type, callable) (swapped parameter order). Consider using the non-deprecated form or assertThatThrownBy(...) instead.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

[davsclaus]

The catchThrowableOfType(callable, type) overload was deprecated in AssertJ 3.22+. Consider using the swapped parameter order:

Suggested change

	MqttException thrown = catchThrowableOfType(MqttException.class, consumer::doStart);

This applies to the other catchThrowableOfType calls in this file and in PahoMqtt5ConsumerLifecycleTest as well.

[atiaomar1978-hub]

Review (Grok + Bugbot)

AI-generated review on behalf of Omar Atie

Reviewed PR #25508 — Fix Paho client cleanup after failed startup (nkokitkar). Ran the new lifecycle tests locally on both camel-paho and camel-paho-mqtt5; all pass. CI has not reported checks on this branch yet. Builds on davsclaus's earlier review with a Bugbot-style pass over the diff.

---

Summary

This fixes a real resource leak in PahoConsumer and PahoMqtt5Consumer: when Camel creates its own MqttClient and startup fails (connect, callback, subscribe), the client was never closed, leaving Paho's thread pool and file-based persistence behind.

The fix is sound:

• stopClient = client == null cleanly separates owned vs shared clients
• try/catch on doStart() force-closes owned clients on failure
• doStop() uses try/catch/finally with addSuppressed for close failures
• createClient() is overridable for unit tests (good pattern)
• 6 focused mock tests per module cover the main scenarios

---

Bugbot findings

No critical bugs in the consumer fix

The lifecycle logic is correct. Shared clients are never closed; owned clients are always closed in finally, including when disconnected (previously leaked).

Medium — same leak still exists in producers

PahoProducer and PahoMqtt5Producer still create and connect clients in doStart() with no cleanup on failure, and neither calls close() on stop. Either extend this PR with the same pattern for producers, or file a follow-up JIRA (as davsclaus suggested). Not a blocker if scope is intentionally consumer-only, but worth tracking.

Medium — missing JIRA + incomplete PR template

• No CAMEL-XXXX in title/commits
• Template checkboxes unchecked (target branch, JIRA, build verification)
• Copilot co-authorship is present in the commit; AI checkbox in the template is unchecked

Per Camel conventions, non-trivial bug fixes need a JIRA reference.

Low — AssertJ deprecated API

Tests use catchThrowableOfType(consumer::doStart, MqttException.class). AssertJ 3.22+ prefers swapped order:

catchThrowableOfType(MqttException.class, consumer::doStart)

Or assertThatThrownBy(consumer::doStart).isSameAs(connectException).

Low — missing test for subscribe failure on start

Tests cover connect failure and stop paths, but not subscribe/callback failure after a successful connect. Code path should still cleanup (same catch block), but an explicit test would lock that in.

Low — behavior change on normal stop (intentional improvement)

Old doStop() only called disconnect() and never close(). New code always calls close(true) for owned clients. That is the right fix for Paho resource cleanup and is tested (stopForceClosesOwnedClientWhenDisconnected), but it is slightly broader than "failed startup" — worth mentioning in the PR description.

---

Grok assessment

Area	Verdict
Root cause	Valid — Paho MqttClient must be closed even when connect/subscribe fails
Fix design	Good — ownership flag, suppressed exceptions, shared-client preservation
Test quality	Strong for a unit-test approach; mock-based, fast, covers key paths
Scope	Consumer-only; producers still affected
Conventions	Missing JIRA; deprecated AssertJ usage; template incomplete
Risk	Low — both Paho modules are @Deprecated(since = "4.21"), but fix is still worth merging

---

Verdict

Approve with minor changes requested (not blocking on code logic):

1. Create/link a JIRA ticket and update title/commits
2. Fill in the PR template / test plan
3. Optionally fix AssertJ deprecation
4. Confirm producer scope (same PR vs follow-up JIRA)

The consumer lifecycle fix itself looks ready to merge once process items are addressed.