Throw DataCorruptionError for meaningful duplicate batch detections#27668
Open
dannimad wants to merge 2 commits into
Open
Throw DataCorruptionError for meaningful duplicate batch detections#27668dannimad wants to merge 2 commits into
dannimad wants to merge 2 commits into
Conversation
Updates the Duplicate Batch Detection tests to match the new behavior: processing a duplicate batch now throws a DataCorruptionError when either the incoming or previously-recorded batch has an explicit batchId, instead of always being log-only. Adds a new test covering the still-log-only case where neither batch has an explicit batchId (the known service-outage scenario). Also adds a changeset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| Previously, all detected duplicate batches were only logged via the `DuplicateBatch` telemetry event, and the corresponding `DataCorruptionError` was never thrown. This was a temporary mitigation for a service-side bug that could redeliver batches. | ||
|
|
||
| Now, the error is thrown when either the incoming batch or the previously-seen batch has an explicit `batchId` (i.e. the batch was resubmitted, as opposed to a fresh batch whose `batchId` is derived from `clientId` and `batchStartCsn`). This distinguishes genuine duplicate-batch scenarios (e.g. container forking) from the known service-outage artifact, which only ever produces duplicates without explicit batch ids. Duplicates without an explicit `batchId` on either side continue to be log-only. |
Contributor
There was a problem hiding this comment.
📝 [vale] reported by reviewdog 🐶
[Microsoft.SentenceLength] Try to keep sentences short (< 30 words).
|
|
||
| Previously, all detected duplicate batches were only logged via the `DuplicateBatch` telemetry event, and the corresponding `DataCorruptionError` was never thrown. This was a temporary mitigation for a service-side bug that could redeliver batches. | ||
|
|
||
| Now, the error is thrown when either the incoming batch or the previously-seen batch has an explicit `batchId` (i.e. the batch was resubmitted, as opposed to a fresh batch whose `batchId` is derived from `clientId` and `batchStartCsn`). This distinguishes genuine duplicate-batch scenarios (e.g. container forking) from the known service-outage artifact, which only ever produces duplicates without explicit batch ids. Duplicates without an explicit `batchId` on either side continue to be log-only. |
Contributor
There was a problem hiding this comment.
[Microsoft.Hyphens] 'previously-seen' doesn't need a hyphen.
|
|
||
| Previously, all detected duplicate batches were only logged via the `DuplicateBatch` telemetry event, and the corresponding `DataCorruptionError` was never thrown. This was a temporary mitigation for a service-side bug that could redeliver batches. | ||
|
|
||
| Now, the error is thrown when either the incoming batch or the previously-seen batch has an explicit `batchId` (i.e. the batch was resubmitted, as opposed to a fresh batch whose `batchId` is derived from `clientId` and `batchStartCsn`). This distinguishes genuine duplicate-batch scenarios (e.g. container forking) from the known service-outage artifact, which only ever produces duplicates without explicit batch ids. Duplicates without an explicit `batchId` on either side continue to be log-only. |
Contributor
There was a problem hiding this comment.
🚫 [vale] reported by reviewdog 🐶
[Microsoft.Foreign] Use 'that is' instead of 'i.e. '.
|
|
||
| Previously, all detected duplicate batches were only logged via the `DuplicateBatch` telemetry event, and the corresponding `DataCorruptionError` was never thrown. This was a temporary mitigation for a service-side bug that could redeliver batches. | ||
|
|
||
| Now, the error is thrown when either the incoming batch or the previously-seen batch has an explicit `batchId` (i.e. the batch was resubmitted, as opposed to a fresh batch whose `batchId` is derived from `clientId` and `batchStartCsn`). This distinguishes genuine duplicate-batch scenarios (e.g. container forking) from the known service-outage artifact, which only ever produces duplicates without explicit batch ids. Duplicates without an explicit `batchId` on either side continue to be log-only. |
Contributor
There was a problem hiding this comment.
🚫 [vale] reported by reviewdog 🐶
[Microsoft.Foreign] Use 'for example' instead of 'e.g. '.
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (125 lines, 3 files), I've queued these reviewers:
How this works
|
Contributor
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
markfields
approved these changes
Jul 10, 2026
| ); | ||
|
|
||
| const batchIdExplicit = batchStart.batchId !== undefined; | ||
| const otherBatchIdExplicit = result.otherBatchInfo?.batchIdExplicit ?? false; |
Member
There was a problem hiding this comment.
Consider a comment reminding that otherBatchInfo may not be known (if loaded from a snapshot) and in this corner case we treat it as false to avoid false positives on the error
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Previously, duplicate-batch detection only logged a
DuplicateBatchtelemetry event and never threw theDataCorruptionError— this was a temporary mitigation for a live service bug that could redeliver batches.This PR re-enables throwing, but only when either the incoming batch or the previously-recorded batch has an explicit
batchId(i.e., it came from a resubmit viaPendingStateManager.replayPendingStates), which indicates a genuine duplicate-batch scenario (e.g. container forking) rather than the known service-outage artifact. Duplicates where neither batch has an explicitbatchIdcontinue to be log-only.Updated the
Duplicate Batch Detectiontests incontainerRuntime.spec.tsto assert the new throwing behavior where applicable, and added a new test covering the still-log-only case (no explicitbatchIdon either side).Reviewer Guidance
The review process is outlined on this wiki page.