[br] BR database restore can report success after a data-file metadata read failure
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
### 1. Minimal reproduce step (Required)
The source-level reproducer is unmodified TiDB master
`05b396fb6636f73b3bc06b09107cf43f2c725c35`.
Save this as `br/pkg/metautil/datafile_terminal_test.go`:
```go
package metautil
import (
"context"
"encoding/json"
"fmt"
"testing"
backuppb "github.com/pingcap/kvproto/pkg/brpb"
"github.com/pingcap/kvproto/pkg/encryptionpb"
"github.com/pingcap/tidb/pkg/meta/model"
"github.com/pingcap/tidb/pkg/objstore"
"github.com/pingcap/tidb/pkg/parser/ast"
"github.com/pingcap/tidb/pkg/tablecodec"
"github.com/stretchr/testify/require"
)
func TestDataFileReadErrorDominatesFileClose(t *testing.T) {
store, err := objstore.NewLocalStorage(t.TempDir())
require.NoError(t, err)
tableID := int64(123)
tableInfo := &model.TableInfo{ID: tableID, Name: ast.NewCIStr("t")}
dbInfo := model.DBInfo{ID: 1, Name: ast.NewCIStr("test")}
dbBytes, err := json.Marshal(dbInfo)
require.NoError(t, err)
tableBytes, err := json.Marshal(tableInfo)
require.NoError(t, err)
files := make([]*backuppb.File, 0, 2048)
for i := range 2048 {
files = append(files, &backuppb.File{
Name: fmt.Sprintf("present-%04d.sst", i),
StartKey: tablecodec.EncodeRowKey(
tableID, fmt.Appendf(nil, "%04d", i)),
EndKey: tablecodec.EncodeRowKey(
tableID, fmt.Appendf(nil, "%04d", i+1)),
})
}
meta := &backuppb.BackupMeta{
Schemas: []*backuppb.Schema{{
Db: dbBytes, Table: tableBytes,
}},
Files: files,
FileIndex: &backuppb.MetaFile{
MetaFiles: []*backuppb.File{{Name: "missing.meta"}},
},
}
cipher := &backuppb.CipherInfo{
CipherType: encryptionpb.EncryptionMethod_PLAINTEXT,
}
var lostErrors int
for range 1000 {
databases, loadErr := LoadBackupTables(
context.Background(),
NewMetaReader(meta, store, cipher),
false,
)
if loadErr == nil {
lostErrors++
require.Len(t,
databases["test"].GetTable("t").FilesOfPhysicals[tableID],
len(files))
}
}
require.Zero(t, lostErrors,
"data-file read errors must never be reported as success")
}
```
Run:
```bash
go test ./br/pkg/metautil \
-run TestDataFileReadErrorDominatesFileClose \
-count=1 -timeout 120s -v
```
Observed:
```text
Error: Should be zero, but was 1
Test: TestDataFileReadErrorDominatesFileClose
Messages: data-file read errors must never be reported as success
```
The product consequence can be reproduced with one TiDB, one PD, real TiKV,
MDL enabled, and a default V2 database backup:
1. Back up a table containing three rows and a secondary index.
2. Copy the backup directory.
3. Replace one referenced `backupmeta.datafile.*` object with invalid bytes.
4. Restore the damaged copy repeatedly.
The natural source test above proves the close-winning schedule is reachable.
For a deterministic product run, add this schedule-only delay immediately
before the `generateFileMapDone` collector loop in
`br/pkg/metautil/metafile.go` and build BR:
```diff
@@
}()
+ time.Sleep(100 * time.Millisecond)
generateFileMapDone:
```
The delay changes no backup data or returned error; it only allows the producer
to publish its terminal error and close its result channel before the consumer
selects.
Run the damaged restore 20 times:
```bash
set -euo pipefail
BR=${BR:-./bin/br}
MYSQL=${MYSQL:-mysql}
PD=${PD:-127.0.0.1:2379}
DB=br_datafile_meta_repro
ROOT=$(mktemp -d /tmp/br-datafile-meta.XXXXXX)
sql() {
"$MYSQL" -N -B -h 127.0.0.1 -P 4000 -u root "$@"
}
sql -e "
DROP DATABASE IF EXISTS $DB;
CREATE DATABASE $DB;
CREATE TABLE $DB.t (
id BIGINT PRIMARY KEY,
v VARCHAR(64),
KEY(v)
);
INSERT INTO $DB.t VALUES
(1,'protected-1'),(2,'protected-2'),(3,'protected-3');"
"$BR" backup db --db "$DB" --pd "$PD" \
--storage "local://$ROOT/intact" --log-file "$ROOT/backup.log"
cp -R "$ROOT/intact" "$ROOT/damaged"
DATA_META=$(find "$ROOT/damaged" -maxdepth 1 -type f \
-name 'backupmeta.datafile.*' -print -quit)
dd if=/dev/zero of="$DATA_META" bs=1048576 count=32 status=none
for n in $(seq 1 20); do
sql -e "DROP DATABASE IF EXISTS $DB;"
set +e
"$BR" restore db --db "$DB" --pd "$PD" \
--storage "local://$ROOT/damaged" \
--log-file "$ROOT/restore-$n.log"
rc=$?
set -e
exists=$(sql -e "
SELECT COUNT(*) FROM information_schema.schemata
WHERE schema_name='$DB';")
rows=NA
if [[ "$exists" = 1 ]]; then
rows=$(sql -e "SELECT COUNT(*) FROM $DB.t;")
fi
printf 'attempt=%s exit=%s database=%s rows=%s\n' \
"$n" "$rc" "$exists" "$rows"
done
```
Observed matrix:
```text
same damaged backup, source helper, no delay:
1000 calls -> 999 errors, 1 lost error
same damaged backup, BR with 100 ms schedule delay:
20 restores -> 10 metadata errors, 10 exit 0 with table present and 0/3 rows
same damaged backup, terminal-join counterfactual, same delay:
20 restores -> 20 metadata errors, 0 databases created
intact backup, unmodified BR:
restored 3/3 rows; forced-index count 3; ADMIN CHECK TABLE passed
```
On a close-winning product run BR prints:
```text
DataBase Restore success summary
total-ranges=0
write-CF-files=0
Size=0
```
The database and table exist, but the restored table contains zero rows.
### 2. What did you expect to see? (Required)
A terminal read or checksum error from any referenced data-file metadata object
must make restore fail. BR should never publish a successful database restore
from a partial file enumeration.
### 3. What did you see instead? (Required)
The collector can select the closed result channel and discard the
already-published metadata error. BR creates the table, imports zero files,
skips checksum validation, prints a success summary, and exits 0.
### 4. What is your TiDB version? (Required)
TiDB master `05b396fb6636f73b3bc06b09107cf43f2c725c35`.
Likely root cause and fix direction
`readDataFiles` sends a terminal error to buffered `fileErrCh` and closes
`fileCh` separately. `ReadSchemasFiles` selects both channels independently.
When both are ready, selecting the closed `fileCh` breaks the loop and drops
the error.
The expected checksum is then derived from the same reduced file map. If no
files were discovered, `ChecksumExists()` is false and validation is skipped,
so it is not an independent completeness oracle.
Use one ordered terminal protocol: drain file results, then join and return the
producer's terminal error before success. The sibling schema parser workers
should also be joined before closing their outer result channel.
Contributor guide
Research direction
Run the provided TestDataFileReadErrorDominatesFileClose in br/pkg/metautil/datafile_terminal_test.go, then read LoadBackupTables and the generateFileMapDone collector in br/pkg/metautil/metafile.go. Trace how readDataFiles publishes its terminal error and how the result channels are consumed. Done means damaged data-file metadata always makes restore fail, while the intact-backup behavior remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100