atiaomar1978-hub commented on PR #25531: URL: https://github.com/apache/camel/pull/25531#issuecomment-5332250748
## AI Review Summary (Bugbot + Grok) _AI-generated on behalf of atiaomar1978-hub_ Both reviews were run against this changeset. Findings below, with status after follow-up commits in this PR. --- ### Bugbot findings | Severity | Finding | Status | |----------|---------|--------| | **High** | `setReplyTo` was called before generation confirmation; a concurrent `scheduleRefresh()` could publish then delete a queue, or return a deleted queue | **Addressed** — `setReplyTo` now runs only after `refreshGeneration == generationToHandle`; stale attempts return `null` | | **High** | Lock-free fast path could return a cached queue after a concurrent arm | **Addressed** — resolver logic is fully under `destinationLock`; no unsynchronized fast path | | **Medium** | Recovery task could stop re-scheduling if container is not running after failed stop/start | **Partially addressed** — `finally` re-schedules when still pending; Simple-container stop/start path remains less exercised in tests | --- ### Grok review (cursor-grok-4.6-high-fast) **Verdict:** request changes → several items addressed in latest commit; a few gaps remain for human review. #### What looks correct - Generation-based refresh (`refreshGeneration` / `publishedGeneration`) is the right fix for **Scenario B** (flag cleared before queue published) and **Scenario C** (lost update on naive reorder-only fix). - Builds on CAMEL-24074 locking without reverting it. - Unit tests with failing `TemporaryQueueResolver` cover the B-shaped retry path well. #### Addressed in this PR 1. **Recovery races Spring DMLC** — recovery is deferred by `recoveryInterval`, skips while `isRecovering()`, and `recoverReplyDestinationAfterRefresh()` guards with `!isRecovering()`. 2. **Publish ordering** — `replyTo` published only after generation match. 3. **Shutdown** — listener stopped first (`super.doStop()`), then `shutdownNow` on recovery executor; loop checks `isStopping()`. 4. **Generation discard during resolve** — covered by `shouldDiscardPublishWhenRefreshGenerationChangesDuringResolve`. #### Remaining gaps / human review items 1. **Scenario A not fully reproduced in tests** — the DMLC invoker-pool late-arm path (CACHE_CONSUMER + collapsed pool) is not covered by unit/IT tests here; the external reproducer lab remains the authoritative proof. Consider follow-up IT or acceptance note in JIRA. 2. **`JmsTemporaryReplyToRequestReplyIT`** — sanity test for consecutive InOut only; Artemis VM `restart()` is a no-op, so this does not prove post-broker-fault recovery. 3. **Unrelated generated file in diff** — `core/camel-core-model/.../model.properties` appears unrelated to CAMEL-24401 and should be dropped from this PR before merge. 4. **Simple `replyToConsumerType`** — stop/start recovery path for `SimpleMessageListenerContainer` is untested. 5. **`camel-sjms`** — same defect family noted in JIRA/reproducer; out of scope here but worth a separate ticket. #### Style / conventions - Production `Thread.sleep` in recovery loop uses endpoint `recoveryInterval` (acceptable for background recovery, but worth committer opinion). - New public method `recoverReplyDestinationAfterRefresh()` on `DefaultJmsMessageListenerContainer` — thin hook, but it is public API on a released type. --- ### Recommendation Approach is sound for B/C and a reasonable mitigation for A. Before merge: - Remove unrelated `model.properties` change - Human reviewer to confirm deferred recovery timing is acceptable vs. always triggering immediately on arm - Acknowledge Scenario A validation relies on reporter reproducer + unit coverage of generation protocol Backport: https://github.com/apache/camel/pull/25530 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
