atiaomar1978-hub commented on PR #25001: URL: https://github.com/apache/camel/pull/25001#issuecomment-5048675429
## Converting to draft — this PR is likely unnecessary, and the current approach cannot work as designed Thank you @gnodet and @davsclaus for the careful reviews. After digging into the Azure Service Bus SDK (`azure-messaging-servicebus` 7.17.16), I've confirmed @gnodet's `CHANGES_REQUESTED` finding is correct on both counts, so I'm converting this to draft rather than pushing a patch that wouldn't address the fundamental issue. Full explanation below. ### 1. The `acceptSession()` approach cannot work while the session processor holds the session Azure Service Bus enforces **exclusive** session locks — only one receiver may hold a session's lock at a time ([Microsoft docs](https://learn.microsoft.com/en-us/azure/service-bus-messaging/message-sessions)). In this PR, the `ServiceBusProcessorClient` (built via `sessionProcessor()`) already holds the lock for each active session. When `SessionLockRenewer.acquireSessionReceiver()` then calls `sessionRenewalClient.acceptSession(sessionId).block()` for that **same** session, the broker rejects it with `SessionCannotBeLockedException` after exhausting the retry policy (~4 minutes with the default `AmqpRetryOptions`). The `catch` block swallows that exception and returns `null`, so `add()` skips renewal registration entirely — meaning **the feature would silently do nothing in production**. The unit tests pass only because they mock `acceptSession()` to return immediately: ```java when(sessionRenewalClient.acceptSession(any())).thenReturn(Mono.just(sessionBoundReceiver)); ``` So the tests validate the bookkeeping, not the one behaviour that actually determines whether the feature works. ### 2. The feature is very likely unnecessary — the SDK already renews session locks in the background This is the key difference from CAMEL-23937. For **per-message** locks, the SDK's auto-renewal is driven by the message-processing pipeline, so async Camel routes (which return from `processMessage` immediately) lose renewal mid-processing — that was the real bug CAMEL-23937 fixed. **Session** locks behave differently. The SDK begins session-lock renewal at **session-acquisition time**, not per message: - `SessionsMessagePump` creates a `ServiceBusSessionReactorReceiver` for each rolling session (passing `maxSessionLockRenew`). - That receiver's constructor calls `session.beginLockRenew(tracer, maxSessionLockRenew)` — a **background** renewal task tied to the session's lifetime, independent of any `processMessage` callback duration. - `maxSessionLockRenew` is fed from `ServiceBusSessionProcessorClientBuilder.maxAutoLockRenewDuration(...)`, which Camel **already** sets in `ServiceBusClientFactory.createServiceBusSessionProcessorClient()`. Microsoft's docs for the session processor builder confirm this: *"`maxAutoLockRenewDuration` controls how long the background renewal task runs ... the client will have to continuously renew the session lock before its expiration."* It is a background task, not one gated on callback return. In other words, for session-enabled consumers the session lock is already kept alive for `maxAutoLockRenewDuration` regardless of how quickly `processMessage` returns — so the gap CAMEL-23937 closed for message locks does not appear to exist for session locks. ### Why I'm not just patching the minor items The other review points (per-session vs per-exchange `lockedUntil` tracking, missing `block()` timeout, `cancel()` not clearing the `entries` map) are all trivially fixable — but none of them change conclusions (1) or (2). There's no small change that makes a second `acceptSession()` succeed against a session the processor already owns. ### Proposed path forward I believe **CAMEL-23938 should be closed as "Not a Problem"**: the SDK's session processor already renews session locks in the background via `maxAutoLockRenewDuration`, which Camel configures. If a real-world case shows session locks still expiring for async session consumers, the correct fix would be a redesign that switches session consumers from `ServiceBusProcessorClient` to a lower-level `ServiceBusSessionReceiverAsyncClient` so Camel owns the session lifecycle directly — a much larger change that would need validation against a live session-enabled queue. @davsclaus @gnodet — could you confirm this reasoning before I close the JIRA? I've moved the PR to draft in the meantime. Happy to close it entirely once you agree. --- *Claude Opus 4.8 on behalf of atiaomar1978-hub. This analysis was produced by an AI agent and may contain inaccuracies; please verify before acting.* -- 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]
