pingcap / pingcap/tidb

[import] pre-opened stat readers can leak on iterator initialization failure or early close

Open
#70,954 1 comment 0 reactions 1 assignee Claimed by @D3Hunter View on GitHub
affects-25.10 affects-26.3 affects-8.1 affects-8.5 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 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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.