[import] pre-opened stat readers can leak on iterator initialization failure or early close
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
### 1. Minimal reproduce step (Required)
This issue was found by AI-assisted static analysis while reviewing #70942. It covers two ownership failures in the asynchronous stat-reader pre-open pipeline used by `newMergePropBaseIter`.
**Initialization-error path**
1. Build a `MultipleFilesStat` with more than `2 * limit` stat files so `newMergePropBaseIter` starts asynchronous pre-opens and can fill `preOpenCh`.
2. Make one of the first `limit` synchronous `NewStatsReader` calls fail while asynchronous opens for later files succeed.
3. `newLimitSizeMergeIter` returns an error, and `newMergePropBaseIter` returns a half-built iterator together with that error.
4. `NewMergePropIter` discards the iterator without closing `closeCh`. The producer can remain blocked on `preOpenCh`, and successfully pre-opened readers have no remaining owner.
**Early-close path after the producer exits**
1. Build a `MultipleFilesStat` with `len(Filenames) <= 2 * limit` and make every stat-reader open succeed.
2. Wait until the producer has put all asynchronous task channels into `preOpenCh` and exited, without consuming those tasks through the iterator.
3. Call `mergePropBaseIter.close()`.
4. `close()` waits for the finished producer and closes readers owned by `m.iter`, but it never drains the successful readers still buffered behind `preOpenCh`.
Relevant review findings:
- https://github.com/pingcap/tidb/pull/70942#discussion_r3957158973
- https://github.com/pingcap/tidb/pull/70942#discussion_r3957158982
The relevant producer, constructor return, and close logic predates #70942 and was introduced by #49832.
### 2. What did you expect to see? (Required)
Reader ownership should be resolved on every constructor and shutdown path. If iterator construction fails or the iterator closes before consuming all pre-opened tasks, all producer goroutines should terminate and every successfully opened `StatsReader` should be closed exactly once.
### 3. What did you see instead (Required)
On an initialization error, the half-built iterator is discarded without signaling `closeCh`. Depending on the number of files, the producer either blocks indefinitely after filling `preOpenCh` or exits while successful reader results remain unconsumed. In both cases, pre-opened readers can leak.
On early close after the producer has already exited, the only `preOpenCh` drain is no longer reachable. `mergePropBaseIter.close()` does not drain the remaining tasks, so their readers stay open together with their storage connections and prefetch buffers.
### 4. What is your TiDB version? (Required)
Confirmed by source inspection on PR #70942's base commit `10f06594e7e60c2238a951d9bc151132b6f5f0c0`. The ownership structure traces back to commit `8a79c0d56c6f7f9570ba6d4697ace1f26ce46b45` from December 2023.
Not dynamically reproduced against a released TiDB version or real S3/GCS service yet.
### Suggested fix
- Make `newMergePropBaseIter` self-cleaning when `newLimitSizeMergeIter` returns an error, and return `nil, err` rather than an unusable half-built iterator.
- Give `close()` unconditional ownership of draining any task channels left in `preOpenCh` after the producer stops, while ensuring a reader is closed exactly once.
- Consider `ctx.Done()` as an additional producer exit signal, but do not rely on cancellation alone to release readers that were already opened.
- Add deterministic tests that track open-reader references for both an initialization failure with a full channel and an early close after successful pre-opens.
### Parent issue
- #69798
Contributor guide
Assessment
This issue has not been assessed yet.