[import] IndexKVHandler replays successful callbacks after a partial batch failure
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
### 1. Minimal reproduce step (Required)
This report is based on a static audit of current master. I have not run a TiDB-level reproduction.
A deterministic unit regression can extend `TestHandler/test index kv handler` in `pkg/dxf/importinto/conflictedkv/handler_test.go`:
1. Insert two source rows and build two unique-index conflict KV pairs whose row handles are fetched by the same `IndexKVHandler` batch.
2. Set `BufferedHandleLimit = 2` so the second `Handle` call enters `handleBufferedHandles`.
3. Use a mock `EncodedRowHandler` that records per-handle invocation counts. Return `nil` on callback invocation 1, an injected error on invocation 2, and `nil` on later invocations. Recording the first successful handle makes the test independent of Go map iteration order.
4. Feed the two index KV pairs to `Run` and assert that `Run` returns the injected error.
5. Call `Close`, as the production collector and deleter lifecycles do.
6. Assert that the first successful handle's callback count is still 1.
The key callback shape is:
```go
var (
callbackCalls int
firstSuccessfulKey string
perHandleCalls = make(map[string]int)
)
mockEncodedKVHdl := mockHandleEncodedRowFn(func(
_ context.Context, handle tidbkv.Handle, _ []types.Datum, _ *kv.Pairs,
) error {
callbackCalls++
key := handle.String()
perHandleCalls[key]++
if callbackCalls == 1 {
firstSuccessfulKey = key
return nil
}
if callbackCalls == 2 {
return errors.New("injected callback failure")
}
return nil
})
// Run flushes the two-handle batch and returns the injected error.
require.Error(t, indexKVHdl.Run(ctx, ch))
require.NoError(t, indexKVHdl.Close(ctx))
require.Equal(t, 1, perHandleCalls[firstSuccessfulKey])
```
On the audited code, the final assertion fails with 2: `Close` fetches and processes the entire two-handle buffer again.
### 2. What did you expect to see? (Required)
Once an individual fetched row has been decoded, encoded, and accepted by `HandleEncodedRow`, a later failure in the same batch must not cause that successful callback to run again during `Close`.
Batch error handling should either consume successful handles incrementally or otherwise ensure that `Close` retries only work that has not already completed. The original error should continue to fail the current subtask.
### 3. What did you see instead (Required)
`IndexKVHandler.handleBufferedHandles` clears `bufferedHandles` only after every fetched row and callback succeeds. If callback 1 succeeds and callback 2 fails, the function returns with the full buffer intact. Both production call paths invoke `Close` after `Run` errors, and `IndexKVHandler.Close` calls `handleBufferedHandles` again, so callback 1 is replayed.
The replay can duplicate externally visible or resource-consuming side effects:
- Collector callbacks can write the same row again to conflict-row object storage and repeat row-count, checksum, and handle-set accounting. This can worsen failed-attempt artifact leakage.
- Deleter callbacks can repeat snapshot reads, key gathering, channel sends, delete transactions, and metering traffic accounting, depending on where the later failure occurs and when buffered keys are flushed.
- The repeated `BatchGet` itself adds read traffic and metering again.
Severity is qualified as moderate: the original processing error still fails the subtask, and deleting the same keys again is idempotent. This report does **not** claim successful table corruption. The confirmed defect is non-idempotent cleanup after a partial batch failure, with duplicate conflict artifacts, redundant storage/transaction work, and inaccurate traffic or result accounting on the failed attempt.
### 4. What is your TiDB version? (Required)
Current master audited at commit `59f6e85cd01756d53f799c33afba0a185d956d3f`.
No runtime `SELECT tidb_version()` output is available because this report is based on static code analysis.
### Analysis
- [`handleBufferedHandles`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L283-L306) builds a batch from all buffered handles, returns immediately on any per-row error, and truncates the buffer only on total success. [`IndexKVHandler.Close`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L309-L315) unconditionally calls it again.
- The handler contract requires `Close` regardless of `PreRun`/`Run` outcome ([`handler.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L57-L67)). Conflict collection follows that contract in a defer ([`collect_conflicts.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/collect_conflicts.go#L207-L219)), as does deletion ([`deleter.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/deleter.go#L98-L125)).
- Collector side effects occur inside [`HandleEncodedRow`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/collector.go#L154-L174) and its object writer. Deleter side effects begin in [`HandleEncodedRow`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/deleter.go#L182-L203), and repeated fetched-row reads are metered in [`LazyRefreshedSnapshot.BatchGet`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L356-L376).
### Missing regression coverage
The existing index-handler cases in `pkg/dxf/importinto/conflictedkv/handler_test.go` use callbacks that always return `nil`. They verify successful full batches and the final `Close` flush, but do not inject a failure after one callback has already succeeded and do not assert that `Close` avoids replaying successful callbacks.
Contributor guide
Research direction
Start with handleBufferedHandles and Close in pkg/dxf/importinto/conflictedkv/handler.go, then read the existing index-handler cases in pkg/dxf/importinto/conflictedkv/handler_test.go. Add the partial-failure regression using the described mock callback and run the focused handler tests. Done means Run returns the injected error and Close does not invoke the already successful handle again.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 64/100