Conversation
adreed-msft
requested review from
dphulkar-msft,
gapra-msft,
seanmcc-msft,
shnayak-msft,
souravgupta-msft,
tanyasethi-msft,
vibhansa-msft and
wonwuakpa-msft
as code owners
There was a problem hiding this comment.
Pull request overview
Fixes an order-of-operations issue in blockBlobSenderBase.Epilogue() so InvalidBlockList diagnostics can run before FailActiveSend cancels the transfer context, enabling collection of block list delta details for troubleshooting.
Changes:
- Defers
jptm.FailActiveSend(...)until after InvalidBlockList diagnostic block list retrieval/logging.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fail _afterwards_, since this is the action that cancels our context. Unfortunately, doing this would cause us to fail to send the request that'd get us details on the block list. | ||
| jptm.FailActiveSend(common.Iff(blobTags != nil, "Committing block list (with tags)", "Committing block list"), err) | ||
|
|
There was a problem hiding this comment.
Now that FailActiveSend is delayed until after the InvalidBlockList diagnostics, any latency/hang in the GetBlockList call (or subsequent formatting) will delay marking the transfer failed/canceling the context. Consider running the diagnostic GetBlockList under a short, bounded timeout context (best-effort) so this debug path can’t stall epilogue/failure propagation.
| // Fail _afterwards_, since this is the action that cancels our context. Unfortunately, doing this would cause us to fail to send the request that'd get us details on the block list. | ||
| jptm.FailActiveSend(common.Iff(blobTags != nil, "Committing block list (with tags)", "Committing block list"), err) | ||
|
|
There was a problem hiding this comment.
With this change, the InvalidBlockList diagnostic logging will actually execute before failing. That path currently logs the full blockIDs slice and builds block ID lists via repeated string concatenation, which can become extremely large/slow (comment notes up to 50k blocks) and can bloat log files. Consider limiting/summarizing at error level (counts + a small sample) and only emitting full lists when debug logging is enabled, and use a builder-based formatter to avoid O(n^2) concatenation costs.
Description
Type of Change
How Has This Been Tested?
Testing with eDiscovery PG to validate, since we have a reliable repro of the bug that causes this.