pingcap / pingcap/tidb

session: LazyTxn.Commit may panic when a killed transaction cleans up MemDB

Open
#69,304 2 comments 0 reactions 0 assignees View on GitHub
affects-8.5 component/session may-affects-8.1 report/customer severity/major sig/execution sig/transaction type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

Please answer these questions before submitting your issue. Thanks!

### 1. Minimal reproduce step (Required)

This is a follow-up to #63956 / #64001 / #64185. Those fixed the `LazyTxn.Rollback` path by replacing the transaction memory-footprint hook with a no-op before rollback cleanup. The same panic class is still reproducible on the `LazyTxn.Commit` path.

The following mock unit test reproduces the problem locally. It creates a transaction with MemDB contents, attaches the session memory tracker to a mock session-root tracker, arms `QueryInterrupted`, and then calls `CommitTxn` directly so the kill signal is still present during commit/memdb cleanup.

```go
func TestPanicOnCommitKilledTxn(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t(id int)")
tk.MustExec("begin pessimistic")
tk.MustExec("insert into t values(1);")
tk.MustExec("insert into t select * from t")
tk.MustExec("insert into t select * from t")
tk.MustExec("insert into t select * from t")
tk.MustExec("insert into t select * from t")
tk.MustExec("insert into t select * from t")
tk.MustExec("insert into t select * from t")
mockTracker := memory.NewTracker(-1, -1)
mockTracker.IsRootTrackerOfSess = true
mockTracker.Killer = &sqlkiller.SQLKiller{}
tk.Session().GetSessionVars().MemTracker.AttachTo(mockTracker)
mockTracker.Killer.SendKillSignal(sqlkiller.QueryInterrupted)
var err error
require.NotPanics(t, func() {
err = tk.Session().CommitTxn(context.Background())
})
require.NoError(t, err)
tk.MustQuery("select count(*) from t").Check(testkit.Rows("64"))
}
```

Run:

```bash
./tools/check/failpoint-go-test.sh pkg/session/test/txn -run TestPanicOnCommitKilledTxn -count=1
```

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

`CommitTxn` should not panic while cleaning up MemDB memory tracking after the transaction has been killed. The transaction should either finish normally if the commit has already proceeded, or return an ordinary error through the existing commit path.

In the local mock above, after the proposed fix the test passes and `select count(*) from t` returns `64`.

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

Without the commit-path fix, the test fails with a recovered panic:

```text
Panic value: [executor:1317]Query execution was interrupted
```

The relevant stack is:

```text
github.com/pingcap/tidb/pkg/util/memory.(*Tracker).Consume
pkg/util/memory/tracker.go:540
github.com/pingcap/tidb/pkg/util/memory.(*Tracker).ReplaceBytesUsed
pkg/util/memory/tracker.go:819
github.com/pingcap/tidb/pkg/session.(*session).SetMemoryFootprintChangeHook.func1
pkg/session/session.go:5460
github.com/tikv/client-go/v2/internal/unionstore/art.(*ART).SetMemoryFootprintChangeHook.func1
github.com/tikv/client-go/v2/internal/unionstore/arena.(*MemdbArena).OnMemChange
github.com/tikv/client-go/v2/internal/unionstore/arena.(*MemdbArena).Reset
github.com/tikv/client-go/v2/internal/unionstore/art.(*ART).DiscardValues
github.com/tikv/client-go/v2/txnkv/transaction.(*twoPhaseCommitter).commitTxn
github.com/tikv/client-go/v2/txnkv/transaction.(*KVTxn).Commit
github.com/pingcap/tidb/pkg/store/driver/txn.(*tikvTxn).Commit
github.com/pingcap/tidb/pkg/session.(*LazyTxn).Commit
github.com/pingcap/tidb/pkg/session.(*session).CommitTxn
```

This is the same mechanism as #63956, but through `LazyTxn.Commit` instead of `LazyTxn.Rollback`: the memdb memory-footprint hook calls `MemDBFootprint.ReplaceBytesUsed`, which reaches `Tracker.Consume(bs > 0)`, and `sessionRootTracker.Killer.HandleSignal()` panics with the kill error.

### 4. What is your TiDB version? (Required)

Verified locally on current master during development.

The same commit-path panic shape was also observed from production logs on v8.5.x:

- v8.5.6 Community: `LazyTxn.Commit -> reset/cleanup -> memBuffer.Cleanup -> memory-footprint hook -> Tracker.Consume`, with `[executor:1317]Query execution was interrupted`.
- v8.5.1 Enterprise: related `Tracker.Consume -> SQLKiller.HandleSignal` panic variants with `[executor:3024] maximum statement execution time exceeded`; those executor-worker variants are broader than this specific commit-path fix.

### Additional notes

The current local fix mirrors the rollback fix from #64001/#64185, but applies it to `LazyTxn.Commit` before entering the underlying `Transaction.Commit`:

```go
// When committing a txn, swap with a dummy hook to avoid checking a killed
// session tracker during memdb cleanup.
txn.SetMemoryFootprintChangeHook(func(uint64) {})
err := txn.Transaction.Commit(ctx)
```

This keeps the change narrow and avoids changing the public `Tracker.Consume` signature. It also matches the previous reasoning for rollback: memory tracking during commit/memdb cleanup should not be allowed to panic solely because the session root tracker has already received a kill signal.

Local validation with this fix:

```bash
./tools/check/failpoint-go-test.sh pkg/session/test/txn -run TestPanicOnCommitKilledTxn -count=1
./tools/check/failpoint-go-test.sh pkg/session/test/txn -run 'TestPanicOn(Commit|Rollback)KilledTxn' -count=1
make bazel_prepare
make lint
```

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.