pingcap / pingcap/tidb

[import] Retried conflict collection leaves orphaned conflict-row objects in external storage

Open
#69,802 0 comments 0 reactions 0 assignees View on GitHub
component/DXF component/global-sort component/import found-by-ai severity/moderate 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 a static audit of current master; I have not run a new TiDB-level reproduction. The existing RealTiKV retry test provides a deterministic reproduction strategy:

1. Run a global-sort `IMPORT INTO ... WITH on_duplicate_key='capture'` that produces conflicts in several KV groups and records conflicted rows in external storage.
2. Enable the existing `github.com/pingcap/tidb/pkg/dxf/importinto/afterCollectOneKVGroup` failpoint as `TestGlobalSortRetryOnConflictResolutionStep` does. Let two KV groups finish, then return its retryable injected error.
3. Before the framework retry, list `conflicted-rows//` in the sort-storage bucket and record the first attempt's UUID-qualified object names.
4. Let the collect-conflicts subtask retry and succeed, then compare every object under that task prefix with `CollectConflictsStepMeta.ConflictedRowFilenames`.

The relevant retry timeline is:

1. Attempt A calls `resetForNewSubtask`, starts with an empty result and zero file-size counter, and gives each collector a fresh UUID prefix such as `conflicted-rows//-/data-0001.txt`.
2. Collectors create and write those objects incrementally. After completed KV groups have closed their writers, `afterCollectOneKVGroup` injects an error, so `RunSubtask` returns before `onFinished` stores the filenames in subtask metadata.
3. Attempt B calls `resetForNewSubtask` again, discarding attempt A's in-memory filenames and resetting the 1 GiB recording counter. New collectors receive new UUID prefixes and write another artifact set.
4. Only attempt B's filenames are published in the final successful subtask metadata.

### 2. What did you expect to see? (Required)

Subtask retry should be idempotent with respect to its externally stored conflicted-row artifacts. After a successful retry, there should be one authoritative artifact set and every retained object should be discoverable through the final subtask metadata. TiDB should either reuse stable names or remove failed-attempt prefixes before publishing the successful attempt.

A failed attempt must not leave persistent, unowned objects or receive a fresh recording quota on every retry.

### 3. What did you see instead (Required)

Static analysis shows that attempt A's closed files remain in object storage but are absent from the final metadata. The retry starts from a new result, a reset size counter, and fresh UUIDs, so it creates duplicate data under unrelated prefixes. The normal import cleanup intentionally deletes `/` global-sort data but not `conflicted-rows//`, because successful conflicted-row files are retained for users. It therefore cannot discover or remove failed-attempt prefixes.

This is a storage/resource and artifact-ownership bug, not evidence of TiKV table corruption. One failed attempt can orphan roughly the per-attempt 1 GiB conflicted-row recording cap, and repeated retries can accumulate another quota-sized set each time. Bucket lifecycle or retention rules may eventually reduce storage usage, but operator policy does not restore TiDB's ownership semantics, make retries idempotent, or make the retained artifact set complete and discoverable from job metadata.

### 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

- [`Collector.recordRowToFile` and `switchFile`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/collector.go#L177-L235) create objects as rows are collected and append their names only to the current collector result. The total recording limit is 1 GiB per collect-conflicts subtask execution ([`collector.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/collector.go#L41-L51)).
- [`RunSubtask`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/collect_conflicts.go#L98-L145) publishes `ConflictedRowFilenames` only after every KV group succeeds. Each collector gets a fresh UUID path ([`collect_conflicts.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/collect_conflicts.go#L184-L220)), while retry resets both the result and recording-size counter ([`collect_conflicts.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/collect_conflicts.go#L230-L240)).
- The filename helper deliberately puts conflicted rows outside the normal `/` cleanup prefix so successful artifacts survive ([`collect_conflicts.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/collect_conflicts.go#L264-L270)). Normal cleanup only calls `CleanUpFiles` for `/` ([`clean_up.go`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/clean_up.go#L91-L106)).

### Missing regression assertions

The existing [`TestGlobalSortRetryOnConflictResolutionStep`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/tests/realtikvtest/importintotest4/conflict_resolution_test.go#L623-L653) already injects a failure after two collected groups and proves that the subtask retries successfully. However, it reads only the filenames that the successful metadata references. It does not:

- list all objects under `conflicted-rows//` and compare that set with `ConflictedRowFilenames`;
- assert that no UUID prefix from the failed attempt remains;
- assert that retries do not duplicate conflicted-row content or bypass the intended per-subtask recording cap.

Contributor guide

Open the contributing guide

Research direction

Start with TestGlobalSortRetryOnConflictResolutionStep in tests/realtikvtest/importintotest4/conflict_resolution_test.go, then read collect_conflicts.go, conflictedkv/collector.go, and clean_up.go. Trace retry reset, object naming, metadata publication, and cleanup behavior. Done means a successful retry leaves no failed-attempt objects, and every retained conflicted-row object is represented by final metadata without bypassing the recording cap.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.