[import] conflict deletion errors can be trapped behind an idle KV input channel
- 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 static analysis of current master. The code-level wait below is deterministic, but this is **not** a claim that an end-to-end production import was observed hanging forever.
Add a package-local unit test in `pkg/dxf/importinto/conflictedkv` that uses:
1. An open, idle `pairCh := make(chan *simplesst.KVPair)` that is not closed until test cleanup.
2. A minimal `BaseHandler` as `Deleter.handler`; it will wait for input without needing a table or encoder.
3. An embedded `kv.Storage` test wrapper whose `Begin` method signals `deleteAttempted` and returns a sentinel nonretryable error.
4. A buffered `Deleter.keysCh` preloaded with one nonempty key batch, so `deleteLoop` immediately reaches the injected `Begin` error and cancels the inner error-group context.
5. A bounded `select` on the result of `Deleter.Run` before closing `pairCh`.
The essential test shape is:
```go
pairCh := make(chan *simplesst.KVPair) // intentionally open and idle
injectedErr := errors.New("injected nonretryable delete error")
deleteAttempted := make(chan struct{})
d := &Deleter{
handler: NewBaseHandler(nil, "", nil, nil, nil, zap.NewNop()),
keysCh: make(chan []kv.Key, 1),
store: &beginErrorStorage{
Storage: mockStore,
err: injectedErr,
beginCalled: deleteAttempted,
},
logger: zap.NewNop(),
}
d.keysCh <- []kv.Key{kv.Key("k")}
done := make(chan error, 1)
go func() { done <- d.Run(context.Background(), pairCh) }()
<-deleteAttempted // Begin closes this immediately before returning injectedErr
select {
case err := <-done:
require.ErrorIs(t, err, injectedErr) // expected after the bug is fixed
case <-time.After(100 * time.Millisecond):
close(pairCh) // cleanup: this is what finally unblocks current code
require.ErrorIs(t, <-done, injectedErr)
t.Fatal("Deleter.Run did not propagate the deletion error while pairCh was idle")
}
```
`beginErrorStorage` can embed the normal mock storage and override only `Begin` to close `beginCalled` and return `injectedErr`. Waiting for `deleteAttempted` removes scheduling ambiguity: the test starts its bounded assertion only after the delete path has reached the injected failure. A sentinel `errors.New` value is nonretryable, so this does not depend on retry timing. The same underlying defect can also be isolated by running `BaseHandler.Run` on an idle channel, canceling its context, and observing that it does not return until the channel is closed.
Production control flow has two nested error-group scopes:
1. [`resolveConflictsOfKVGroup`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflict_resolution.go#L139-L149) puts `ReadKVFilesAsync` and each `Deleter.Run` in the outer conflict-resolution error group.
2. [`Deleter.Run`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/deleter.go#L99-L135) creates an inner error group for `deleteLoop` and the handler. If deletion exhausts retries or returns a nonretryable error, only the inner context is canceled at that point.
3. [`BaseHandler.Run`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L124-L133) uses `for kvPair := range pairCh` and never selects on `ctx.Done()`. With no next pair and no channel close, it remains blocked, so the inner `eg.Wait()` and `Deleter.Run` cannot return the deletion error to the outer group.
4. [`ReadKVFilesAsync`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/ingestor/globalsort/reader.go#L238-L279) is the outer sibling that owns `pairCh` closure. If it is idle in an object-storage read, the outer context has not yet been canceled because the deletion error is still trapped in `Deleter.Run`.
The deterministic test proves delayed error propagation and a code-level wait with no local bound while the input channel stays idle. In production, whether this becomes an indefinite job stall or a long delay depends on the object-storage client's own request deadlines and cancellation behavior; this report does not assume those backend operations wait forever.
### 2. What did you expect to see? (Required)
When `deleteLoop` returns an error, cancellation of the inner error-group context should promptly wake the handler even if `pairCh` is open and idle. `Deleter.Run` should return the deletion error, allowing the outer conflict-resolution error group to cancel its reader sibling and fail the import job promptly.
The deterministic regression test should receive `injectedErr` from `Deleter.Run` without sending a pair or closing `pairCh`.
### 3. What did you see instead (Required)
`BaseHandler.Run` waits only for a pair or channel closure. Inner-context cancellation is ignored while it is blocked on the channel receive. Therefore `Deleter.Run` cannot finish, the deletion error is not propagated to the outer error group, and the outer reader is not canceled by that error.
In the deterministic regression, `Deleter.Run` remains blocked for the entire observation window and returns `injectedErr` only after the test closes `pairCh`. In production, propagation is delayed until the reader sends another pair, closes the channel, or some other event cancels/unblocks the outer operation. The resulting delay is unbounded by the handler/deleter code itself; backend timeout behavior determines the practical upper bound, if any.
Existing coverage does not exercise this topology. [`TestDeleter`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/deleter_test.go#L107-L137) sends all input and always closes `pairCh`; there is no regression combining an idle open input channel with a delete-loop failure or inner cancellation.
### 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 analysis.
Contributor guide
Research direction
Start with pkg/dxf/importinto/conflictedkv/handler.go and deleter.go, especially BaseHandler.Run and Deleter.Run, then review the existing TestDeleter in deleter_test.go. Add the described regression using an idle open pairCh and injected Begin error; done means Deleter.Run returns that sentinel error without closing pairCh, with the focused package tests passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100