From d491954aad2b01e3d054a7a43530a28d4174c148 Mon Sep 17 00:00:00 2001 From: vchaindz Date: Thu, 30 Jul 2026 15:56:52 +0200 Subject: [PATCH 1/2] fix(server): keep the ongoing tx when a statement fails before execution A statement rejected before the SQL engine runs it - a parse error being the common case - returns no transaction while leaving the caller's ongoing one open and uncancelled. Both callers that hold the only reference to that transaction overwrote it with the returned nil, orphaning an OngoingTx that still held its tbtree snapshots. Snapshots are released solely by Commit and Cancel, and tbtree.snapshots only shrinks on Snapshot.Close, so each failure leaked one permanently. After maxActiveSnapshots (default 100) failures every new transaction failed with "tbtree: max active snapshots limit reached" until the server was restarted. The session path made this unrecoverable: with sqlTx nil, IsClosed reports true, so TxSQLExec removed the transaction from the session and neither the client's Rollback nor session close and expiry could reach it any more. That is also why the reported client signature shows Rollback answering "no transaction found". Guard both assignment sites on what actually happened to the transaction rather than on the error: adopt the returned transaction only when the engine produced one, or when the previous one is genuinely closed. Execution-stage errors are unaffected because the engine cancels the transaction itself, so the reference is still replaced and the session still drops it. Note the leak did not originate in Engine.Exec: the session path parses in db.SQLExec (pkg/database/sql.go) and returns before the engine is reached. Other pre-execution returns leak the same way - empty statement lists, parameter normalisation failures, replica rejections - so the fix belongs in the callers that own the reference, not in the individual early returns. Fixes #2127 --- pkg/pgsql/server/query_machine.go | 8 ++- .../internal/transactions/transactions.go | 12 +++- .../transactions/transactions_test.go | 66 +++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/pkg/pgsql/server/query_machine.go b/pkg/pgsql/server/query_machine.go index c073a59c82..7046d908c3 100644 --- a/pkg/pgsql/server/query_machine.go +++ b/pkg/pgsql/server/query_machine.go @@ -1202,7 +1202,13 @@ func (s *session) exec(st sql.SQLStmt, namedParams []*schema.NamedParam, resultC } ntx, _, err := s.db.SQLExecPrepared(s.ctx, tx, []sql.SQLStmt{st}, params) - s.tx = ntx + + // Same orphaning hazard as the session transaction path: a statement rejected + // before execution returns no transaction while leaving the ongoing one open, + // so replacing the reference would leak its snapshots. + if ntx != nil || tx == nil || tx.Closed() { + s.tx = ntx + } return err } diff --git a/pkg/server/sessions/internal/transactions/transactions.go b/pkg/server/sessions/internal/transactions/transactions.go index 4fc673c482..2af75e8827 100644 --- a/pkg/server/sessions/internal/transactions/transactions.go +++ b/pkg/server/sessions/internal/transactions/transactions.go @@ -121,7 +121,17 @@ func (tx *transaction) SQLExec(ctx context.Context, request *schema.SQLExecReque return sql.ErrNoOngoingTx } - tx.sqlTx, _, err = tx.db.SQLExec(ctx, tx.sqlTx, request) + ntx, _, err := tx.db.SQLExec(ctx, tx.sqlTx, request) + + // A statement rejected before the engine executes it - a parse error, for + // instance - returns no transaction while leaving the ongoing one open and + // uncancelled. Overwriting the reference in that case orphans it along with + // the snapshots it holds, which are then never released. On execution errors + // the engine cancels the transaction itself, so the reference is replaced as + // usual and the session drops it. + if ntx != nil || tx.sqlTx.Closed() { + tx.sqlTx = ntx + } return err } diff --git a/pkg/server/sessions/internal/transactions/transactions_test.go b/pkg/server/sessions/internal/transactions/transactions_test.go index 17c30c2513..a626173f8b 100644 --- a/pkg/server/sessions/internal/transactions/transactions_test.go +++ b/pkg/server/sessions/internal/transactions/transactions_test.go @@ -23,6 +23,8 @@ import ( "github.com/codenotary/immudb/embedded/logger" "github.com/codenotary/immudb/embedded/sql" + "github.com/codenotary/immudb/embedded/store" + "github.com/codenotary/immudb/pkg/api/schema" "github.com/codenotary/immudb/pkg/database" "github.com/stretchr/testify/require" ) @@ -55,3 +57,67 @@ func TestNewTx(t *testing.T) { _, err = tx.Commit(context.Background()) require.ErrorIs(t, err, sql.ErrNoOngoingTx) } + +// A statement that fails before reaching the engine (a parse error, for example) +// must leave the ongoing transaction untouched: the engine never got a chance to +// cancel it, so dropping the reference here would orphan it together with the +// snapshots it holds. See issue #2127. +func TestSQLExecPreExecutionErrorKeepsTxOpen(t *testing.T) { + db, err := database.NewDB("db1", nil, database.DefaultOptions().WithDBRootPath(t.TempDir()), logger.NewSimpleLogger("logger", os.Stdout)) + require.NoError(t, err) + + tx, err := NewTransaction(context.Background(), sql.DefaultTxOptions(), db, "session1") + require.NoError(t, err) + + err = tx.SQLExec(context.Background(), &schema.SQLExecRequest{Sql: "THIS IS NOT SQL"}) + require.Error(t, err) + + require.False(t, tx.IsClosed(), "a parse error must not close the transaction") + + // the client can still roll back, which is what releases the snapshots + require.NoError(t, tx.Rollback()) + require.True(t, tx.IsClosed()) +} + +// An execution-stage error is cancelled by the engine itself, so the transaction +// is expected to end up closed. Pinned here so the fix for #2127 does not quietly +// change this path. +func TestSQLExecExecutionErrorClosesTx(t *testing.T) { + db, err := database.NewDB("db1", nil, database.DefaultOptions().WithDBRootPath(t.TempDir()), logger.NewSimpleLogger("logger", os.Stdout)) + require.NoError(t, err) + + tx, err := NewTransaction(context.Background(), sql.DefaultTxOptions(), db, "session1") + require.NoError(t, err) + + err = tx.SQLExec(context.Background(), &schema.SQLExecRequest{Sql: "INSERT INTO nonexistent(id) VALUES (1);"}) + require.Error(t, err) + + require.True(t, tx.IsClosed(), "the engine cancels the tx on execution errors") + require.ErrorIs(t, tx.Rollback(), sql.ErrNoOngoingTx) +} + +// Repeated pre-execution failures used to leak one tbtree snapshot each, so after +// maxActiveSnapshots of them no further transaction could be opened until the +// server was restarted. See issue #2127. +func TestSQLExecPreExecutionErrorDoesNotLeakSnapshots(t *testing.T) { + const maxActiveSnapshots = 4 + + storeOpts := store.DefaultOptions(). + WithIndexOptions(store.DefaultIndexOptions().WithMaxActiveSnapshots(maxActiveSnapshots)) + + db, err := database.NewDB("db1", nil, + database.DefaultOptions().WithDBRootPath(t.TempDir()).WithStoreOptions(storeOpts), + logger.NewSimpleLogger("logger", os.Stdout)) + require.NoError(t, err) + + // mirrors the reported production cycle: the client submits an invalid + // statement, attempts a rollback, and carries on regardless of its outcome + for i := 0; i < maxActiveSnapshots*2; i++ { + tx, err := NewTransaction(context.Background(), sql.DefaultTxOptions(), db, "session1") + require.NoErrorf(t, err, "opening a transaction failed on iteration %d: snapshots are leaking", i) + + require.Error(t, tx.SQLExec(context.Background(), &schema.SQLExecRequest{Sql: "THIS IS NOT SQL"})) + + _ = tx.Rollback() + } +} From 314e3057dba7e06d692b06c682d3dd359f2fc6d6 Mon Sep 17 00:00:00 2001 From: vchaindz Date: Fri, 31 Jul 2026 20:48:32 +0200 Subject: [PATCH 2/2] test(stdlib): expect the tx to survive a parse error TestTx_Errors asserted the symptom that d491954a removes: a parse error used to leave sqlTx nil, so IsClosed reported true, the session dropped the transaction and the next statement answered "no transaction found". The transaction now stays open, so that statement reaches the parser and returns its own syntax error instead. Assert that error, and roll back at the end - on the old code the rollback is exactly what failed, so it pins the fix through the full client and session path rather than only at the transactions package. The sessions import goes with the ErrTransactionNotFound reference. --- pkg/stdlib/tx_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/stdlib/tx_test.go b/pkg/stdlib/tx_test.go index 1a0ebc2867..c881d305af 100644 --- a/pkg/stdlib/tx_test.go +++ b/pkg/stdlib/tx_test.go @@ -24,7 +24,6 @@ import ( "google.golang.org/grpc/status" - "github.com/codenotary/immudb/pkg/server/sessions" "github.com/stretchr/testify/require" ) @@ -105,5 +104,8 @@ func TestTx_Errors(t *testing.T) { require.ErrorContains(t, err, "syntax error: unexpected IDENTIFIER at position 4") _, err = tx.QueryContext(context.Background(), "this is also very wrong") - require.ErrorIs(t, err, sessions.ErrTransactionNotFound) + require.ErrorContains(t, err, "syntax error: unexpected IDENTIFIER at position 4") + + // the tx survived both parse errors and can still be rolled back (#2127) + require.NoError(t, tx.Rollback()) }