pingcap / pingcap/tidb

[import] conflict deletion errors can be trapped behind an idle KV input channel

Open
#69,805 0 comments 0 reactions 0 assignees View on GitHub
component/DXF component/global-sort component/import found-by-ai severity/major type/bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.