[BUG] Renewal of messages is executed even if the message has been completed

[stewartadam]

Describe the bug
Re-opening an issue for continued discussion of Azure/azure-service-bus-dotnet#591, where @havarnov did a great job describing current and expected behavior for long-running message handlers that explicitly ACK a message early on.

Exception or Stack Trace

Microsoft.Azure.ServiceBus.MessageLockLostException: The lock supplied is invalid. Either the lock expired, or the message has already been removed from the queue. Reference:e3cd64ae-fd49-412b-83bb-342590d58020, TrackingId:43f4e2cd-1bc4-427e-a979-a11fbd8e7e72_B0, SystemTracker:redacted:Queue:provisioning-progress, Timestamp:2019-06-26T23:54:36
   at Microsoft.Azure.ServiceBus.Core.MessageReceiver.OnRenewLockAsync(String lockToken)
   at Microsoft.Azure.ServiceBus.Core.MessageReceiver.<>c__DisplayClass74_0.<<RenewLockAsync>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.Azure.ServiceBus.RetryPolicy.RunOperation(Func`1 operation, TimeSpan operationTimeout)
   at Microsoft.Azure.ServiceBus.RetryPolicy.RunOperation(Func`1 operation, TimeSpan operationTimeout)
   at Microsoft.Azure.ServiceBus.Core.MessageReceiver.RenewLockAsync(String lockToken)
   at Microsoft.Azure.ServiceBus.Core.MessageReceiver.RenewLockAsync(Message message)
   at Microsoft.Azure.ServiceBus.MessageReceivePump.RenewMessageLockTask(Message message, CancellationToken renewLockCancellationToken)

To Reproduce
Steps to reproduce the behavior:

1. Setup message handler (QueueClient.RegisterMessageHandler)
2. Complete the message as soon as it comes in
3. Delay the message handler after completion for a longer time span than the LockDuration
4. ExceptionReceivedHandler will receive a "Microsoft.Azure.ServiceBus.MessageLockLostException" because the library tries to renew an already completed message

Code Snippet
See linked issue

Expected behavior
ExceptionReceivedHandler will not receive any exception as the RenewMessageTask should've been canceled.

Screenshots

Setup (please complete the following information):

• OS: [e.g. iOS] MacOS 10.14.5
• IDE : [e.g. IntelliJ] VS Code
• Version of the Library used: 3.4.0

Additional context
Add any other context about the problem here.

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

• Bug Description Added
• Repro Steps Added
• Setup information Added

[jsquire]

//cc: @nemakam, @binzywu, @AlexGhiondea

[stewartadam]

Here is a simple min viable repro: https://github.com/stewartadam/aspnet-servicebus-hostedservice

Edit appsettings.json for your connection string/queue name, then simply POST https://localhost:5001/api/bus and then observe the log output.

The controller pushes a message on the bus, the IHostedService implementation will read the message on a background thread and complete it immediately, then wait 25 seconds. The long-running message handler completes as expected but Service Bus exceptions are printed about lock renewal failing, which should not happen for a message that was already completed.

[nemakam]

The background renewal doesn't know about the CompleteAsync() done by the client handler. If you are completing the message early on, then why do you need auto-renewal?
I'm closing this issue as it is by-design and the cons of fixing this I believe outweigh the pros.
The message renewal should either way stop renewing for that message once it receives LockLostException for that message.

[stewartadam]

I'm completing it early on just to show what happens in the min viable repro - but as noted above the real-world scenario has a long-running task as the message handler so lock renewals are necessary.

[stewartadam]

and, for clarity - if this is by design, we should document this so that users are aware this exception is expected when auto-renew is enabled.