Skip to content

fix(fork): backport k3s-io/kine#795 Migrate connection leak fix - #38

Merged
cbalan merged 1 commit into
release/vcluster/v0.17.2from
fix/engcp-1950-migrate-leak-v0.17.2
Nov 12, 2026
Merged

cbalan merged 1 commit into
release/vcluster/v0.17.2from
fix/engcp-1950-migrate-leak-v0.17.2

Conversation

@cbalan

@cbalan cbalan commented Nov 9, 2026 •

Copy link
Copy Markdown

Backport of upstream k3s-io#795 (commit k3s-io/kine@f805442, "Fix connection leak in Migrate") to the v0.17.2 release line, as a fix(fork) commit per the fork policy. The upstream patch applies cleanly here.

Replay note: upstream v0.17.3 and later already contain this fix and then remove Migrate altogether ("Drop migration support", same upstream PR). Drop this commit when replaying the fork commits onto a release line cut from v0.17.3 or later; it conflicts in pkg/drivers/generic/generic.go there.

Why

On this branch TestCompact fails on cgo SQLite: the post-compaction PRAGMA wal_checkpoint(FULL) waits out the full 30 s _busy_timeout, and the apiserver test cancels the Compact call. This is the Test sqlite failure that #35 merged with.

Root cause: Generic.Migrate opened both COUNT(*) rows up front and returned after the first Scan failed (no key_value table), so the second row was never read and kept its connection checked out. With _stmt_cache_size=20 in DefaultParams, go-sqlite3 steps the cached statement eagerly, so that leaked connection also holds a read transaction on the startup snapshot. FULL waits for every reader to reach the latest snapshot, so it blocks until the busy timeout.

Pinning the golang:1.26-alpine3.23 digest would not fix this: the stall reproduces locally on Go 1.26.5, 1.26.8 and 1.26.10.

Changes

  • pkg/drivers/generic/generic.go: query kine only after the key_value row has been scanned and warrants it.
  • pkg/drivers/sqlite/sqlite_test.go: adds upstream's TestMigrateReleasesItsConnections (1-connection pool, DB must stay usable after Migrate).

Testing

  • Local repro (default DSN, busy timeout lowered to 3 s, one write, then wal_checkpoint(FULL)): before the fix, busy=1 after 3.1 s with 1 connection still in use; after, busy=0 in about 0.25 ms. Same result for cgo (mattn, SQLite 3.53.4) and nocgo (modernc).
  • TestMigrateReleasesItsConnections passes with CGO_ENABLED=1 and CGO_ENABLED=0.
  • CGO_ENABLED=1 go test ./pkg/drivers/... ./pkg/logstructured/... passes.
  • The full apiserver TestCompact suite was not run locally; the SQLite CI jobs on this PR cover it.

Refs: ENGCP-1950

🤖 Generated with Claude Code

@cbalan
cbalan force-pushed the fix/engcp-1950-migrate-leak-v0.17.2 branch from 4c3503f to 6194e8b Compare November 9, 2026 16:30
@cbalan cbalan changed the title fix(fork): close leaked Migrate connection that stalls FULL WAL checkpoint (v0.17.2) fix(fork): close the connection Migrate leaks on a fresh database Nov 9, 2026
@cbalan
cbalan force-pushed the fix/engcp-1950-migrate-leak-v0.17.2 branch from 6194e8b to 09a35c8 Compare November 9, 2026 16:34
@cbalan
cbalan force-pushed the fix/engcp-1950-migrate-leak-v0.17.2 branch from 09a35c8 to a21327e Compare November 10, 2026 15:57
@cbalan cbalan changed the title fix(fork): close the connection Migrate leaks on a fresh database fix(fork): backport k3s-io/kine#795 Migrate connection leak fix Nov 10, 2026

@Piotr1215 Piotr1215 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Verified the upstream patch and the regression test with cgo and nocgo.

@cbalan
cbalan merged commit 770d621 into release/vcluster/v0.17.2 Nov 12, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants