From 3ebb07a5600218282e6fd19b657bd7441145251b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?coffeegoddd=E2=98=95=EF=B8=8F=E2=9C=A8?= Date: Tue, 27 Jan 2026 13:01:32 -0800 Subject: [PATCH 1/2] /go/{cmd,libraries}: close dbs in no cache mode --- .../dolt/commands/engine/lock_release_test.go | 96 +++++++++++++++++++ go/cmd/dolt/commands/engine/sqlengine.go | 3 + go/libraries/doltcore/env/environment.go | 18 ++-- .../doltcore/sqle/database_provider.go | 79 ++++++++++++++- .../doltcore/sqle/statspro/controller.go | 10 +- .../doltcore/sqle/statspro/listener.go | 28 +++++- 6 files changed, 222 insertions(+), 12 deletions(-) create mode 100644 go/cmd/dolt/commands/engine/lock_release_test.go diff --git a/go/cmd/dolt/commands/engine/lock_release_test.go b/go/cmd/dolt/commands/engine/lock_release_test.go new file mode 100644 index 00000000000..db62563ed82 --- /dev/null +++ b/go/cmd/dolt/commands/engine/lock_release_test.go @@ -0,0 +1,96 @@ +// Copyright 2026 Dolthub, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package engine + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/dolthub/fslock" + "github.com/stretchr/testify/require" + + "github.com/dolthub/dolt/go/libraries/doltcore/dbfactory" + "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" + "github.com/dolthub/dolt/go/libraries/doltcore/env" + "github.com/dolthub/dolt/go/libraries/utils/config" + "github.com/dolthub/dolt/go/libraries/utils/filesys" +) + +// TestCreateDatabase_ReleasesLockOnEngineClose asserts that when embedded callers opt into +// disable_singleton_cache, closing the SQL engine releases the underlying filesystem lock +// for a newly created database so subsequent opens can proceed. +func TestCreateDatabase_ReleasesLockOnEngineClose(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping on windows due to differing file locking semantics") + } + + ctx := context.Background() + root := t.TempDir() + + fs, err := filesys.LocalFS.WithWorkingDir(root) + require.NoError(t, err) + + cfg := config.NewMapConfig(map[string]string{ + config.UserNameKey: "test", + config.UserEmailKey: "test@example.com", + }) + + dbLoadParams := map[string]interface{}{ + dbfactory.DisableSingletonCacheParam: struct{}{}, + dbfactory.FailOnJournalLockTimeoutParam: struct{}{}, + } + + rootEnv := env.LoadWithoutDB(ctx, env.GetCurrentUserHomeDir, fs, doltdb.LocalDirDoltDB, "test") + rootEnv.DBLoadParams = map[string]interface{}{ + dbfactory.DisableSingletonCacheParam: struct{}{}, + dbfactory.FailOnJournalLockTimeoutParam: struct{}{}, + } + mrEnv, err := env.MultiEnvForDirectory(ctx, cfg, fs, "test", rootEnv) + require.NoError(t, err) + + seCfg := &SqlEngineConfig{ + ServerUser: "root", + ServerHost: "localhost", + Autocommit: true, + DBLoadParams: dbLoadParams, + } + + se, err := NewSqlEngine(ctx, mrEnv, seCfg) + require.NoError(t, err) + + sqlCtx, err := se.NewLocalContext(ctx) + require.NoError(t, err) + + _, _, _, err = se.Query(sqlCtx, "CREATE DATABASE IF NOT EXISTS testdb") + require.NoError(t, err) + + err = se.Close() + require.True(t, err == nil || errors.Is(err, context.Canceled), "unexpected close error: %v", err) + + // If the DB is properly closed, we should be able to take the lock quickly. + lockPath := filepath.Join(root, "testdb", ".dolt", "noms", "LOCK") + _, err = os.Stat(lockPath) + require.NoError(t, err, "expected lock file to exist at %s", lockPath) + + lck := fslock.New(lockPath) + err = lck.LockWithTimeout(25 * time.Millisecond) + require.NoError(t, err, "expected lock to be free after engine close (path=%s)", lockPath) + require.NoError(t, lck.Unlock()) +} diff --git a/go/cmd/dolt/commands/engine/sqlengine.go b/go/cmd/dolt/commands/engine/sqlengine.go index 2c139d138d1..34af08bcead 100644 --- a/go/cmd/dolt/commands/engine/sqlengine.go +++ b/go/cmd/dolt/commands/engine/sqlengine.go @@ -186,6 +186,9 @@ func NewSqlEngine( return nil, err } pro = pro.WithRemoteDialer(mrEnv.RemoteDialProvider()) + if config != nil && len(config.DBLoadParams) > 0 { + pro.SetDBLoadParams(config.DBLoadParams) + } config.ClusterController.RegisterStoredProcedures(pro) if config.ClusterController != nil { diff --git a/go/libraries/doltcore/env/environment.go b/go/libraries/doltcore/env/environment.go index 21baf0a8987..88913bfba96 100644 --- a/go/libraries/doltcore/env/environment.go +++ b/go/libraries/doltcore/env/environment.go @@ -127,6 +127,9 @@ func (dEnv *DoltEnv) DoltDB(ctx context.Context) *doltdb.DoltDB { func (dEnv *DoltEnv) LoadDoltDBWithParams(ctx context.Context, nbf *types.NomsBinFormat, urlStr string, fs filesys.Filesys, params map[string]interface{}) error { if dEnv.doltDB == nil { + if nbf == nil { + nbf = types.Format_Default + } // Merge any environment-level DB load params without mutating the caller's map. if len(dEnv.DBLoadParams) > 0 { if params == nil { @@ -136,11 +139,14 @@ func (dEnv *DoltEnv) LoadDoltDBWithParams(ctx context.Context, nbf *types.NomsBi maps.Copy(params, dEnv.DBLoadParams) } } - ddb, err := doltdb.LoadDoltDBWithParams(ctx, types.Format_Default, urlStr, fs, params) + ddb, err := doltdb.LoadDoltDBWithParams(ctx, nbf, urlStr, fs, params) if err != nil { + dEnv.DBLoadError = err return err } dEnv.doltDB = ddb + dEnv.urlStr = urlStr + dEnv.DBLoadError = nil } return nil } @@ -506,9 +512,7 @@ func (dEnv *DoltEnv) InitRepoWithNoData(ctx context.Context, nbf *types.NomsBinF return err } - dEnv.doltDB, err = doltdb.LoadDoltDB(ctx, nbf, dEnv.urlStr, dEnv.FS) - - return err + return dEnv.LoadDoltDBWithParams(ctx, nbf, dEnv.urlStr, dEnv.FS, nil) } var ErrCannotCreateDirDoesNotExist = errors.New("dir does not exist") @@ -640,13 +644,11 @@ func (dEnv *DoltEnv) InitDBWithTime(ctx context.Context, nbf *types.NomsBinForma } func (dEnv *DoltEnv) InitDBWithCommitMetaGenerator(ctx context.Context, nbf *types.NomsBinFormat, branchName string, commitMeta datas.CommitMetaGenerator) error { - var err error - dEnv.doltDB, err = doltdb.LoadDoltDB(ctx, nbf, dEnv.urlStr, dEnv.FS) - if err != nil { + if err := dEnv.LoadDoltDBWithParams(ctx, nbf, dEnv.urlStr, dEnv.FS, nil); err != nil { return err } - err = dEnv.DoltDB(ctx).WriteEmptyRepoWithCommitMetaGenerator(ctx, branchName, commitMeta) + err := dEnv.DoltDB(ctx).WriteEmptyRepoWithCommitMetaGenerator(ctx, branchName, commitMeta) if err != nil { return fmt.Errorf("%w: %v", doltdb.ErrNomsIO, err) } diff --git a/go/libraries/doltcore/sqle/database_provider.go b/go/libraries/doltcore/sqle/database_provider.go index 50eaab1b60a..5b7528b3f55 100644 --- a/go/libraries/doltcore/sqle/database_provider.go +++ b/go/libraries/doltcore/sqle/database_provider.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "maps" "path/filepath" "sort" "strings" @@ -52,6 +53,13 @@ type DoltDatabaseProvider struct { fs filesys.Filesys remoteDialer dbfactory.GRPCDialProvider // TODO: why isn't this a method defined on the remote object + // dbLoadParams are optional parameters to apply when loading local file-backed databases created + // via provider-managed code paths (e.g. CREATE DATABASE / dolt_clone / undrop). These should match + // the params threaded into env.DoltEnv.DBLoadParams by higher layers (e.g. engine.SqlEngineConfig.DBLoadParams). + // + // Note: these params are only effective if they are set before the underlying DoltDB is loaded. + dbLoadParams map[string]interface{} + // dbLocations maps a database name to its file system root dbLocations map[string]filesys.Filesys databases map[string]dsess.SqlDatabase @@ -195,6 +203,31 @@ func (p *DoltDatabaseProvider) WithRemoteDialer(provider dbfactory.GRPCDialProvi return &cp } +// SetDBLoadParams sets optional DB load params for newly created / registered databases. The provided map is cloned. +func (p *DoltDatabaseProvider) SetDBLoadParams(params map[string]interface{}) { + p.mu.Lock() + defer p.mu.Unlock() + if len(params) == 0 { + p.dbLoadParams = nil + return + } + p.dbLoadParams = maps.Clone(params) +} + +func (p *DoltDatabaseProvider) applyDBLoadParamsToEnv(dEnv *env.DoltEnv) { + if dEnv == nil { + return + } + if len(p.dbLoadParams) == 0 { + return + } + if dEnv.DBLoadParams == nil { + dEnv.DBLoadParams = maps.Clone(p.dbLoadParams) + return + } + maps.Copy(dEnv.DBLoadParams, p.dbLoadParams) +} + // AddInitDatabaseHook adds an InitDatabaseHook to this provider. The hook will be invoked // whenever this provider creates a new database. func (p *DoltDatabaseProvider) AddInitDatabaseHook(hook InitDatabaseHook) { @@ -212,9 +245,43 @@ func (p *DoltDatabaseProvider) FileSystem() filesys.Filesys { } func (p *DoltDatabaseProvider) Close() { + p.mu.RLock() + closeDoltDBs := p.dbLoadParams != nil + if closeDoltDBs { + _, closeDoltDBs = p.dbLoadParams[dbfactory.DisableSingletonCacheParam] + } + + // Copy the databases so we can close outside the lock. + dbs := make([]dsess.SqlDatabase, 0, len(p.databases)) for _, db := range p.databases { + dbs = append(dbs, db) + } + p.mu.RUnlock() + + for _, db := range dbs { db.Close() } + + // In embedded / nocache mode, the underlying DoltDBs are not tracked by the singleton cache, so + // the provider is responsible for closing them to release filesystem locks. + if closeDoltDBs { + seen := make(map[*doltdb.DoltDB]struct{}) + for _, db := range dbs { + if db == nil { + continue + } + for _, ddb := range db.DoltDatabases() { + if ddb == nil { + continue + } + if _, ok := seen[ddb]; ok { + continue + } + seen[ddb] = struct{}{} + _ = ddb.Close() + } + } + } } // Installs an InitDatabaseHook which configures new databases--those @@ -512,7 +579,9 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str } // TODO: fill in version appropriately - newEnv := env.Load(ctx, env.GetCurrentUserHomeDir, newFs, p.dbFactoryUrl, "TODO") + // Use LoadWithoutDB so we can apply db-load params before any DB is opened. + newEnv := env.LoadWithoutDB(ctx, env.GetCurrentUserHomeDir, newFs, p.dbFactoryUrl, "TODO") + p.applyDBLoadParamsToEnv(newEnv) newDbStorageFormat := types.Format_Default err = newEnv.InitRepo(ctx, newDbStorageFormat, sess.Username(), sess.Email(), p.defaultBranch) @@ -753,6 +822,7 @@ func (p *DoltDatabaseProvider) cloneDatabaseFromRemote( if err != nil { return err } + p.applyDBLoadParamsToEnv(dEnv) err = actions.CloneRemote(ctx, srcDB, remoteName, branch, false, depth, dEnv) if err != nil { @@ -860,7 +930,9 @@ func (p *DoltDatabaseProvider) UndropDatabase(ctx *sql.Context, name string) (er return err } - newEnv := env.Load(ctx, env.GetCurrentUserHomeDir, newFs, p.dbFactoryUrl, "TODO") + // Use LoadWithoutDB so we can apply db-load params before any DB is opened. + newEnv := env.LoadWithoutDB(ctx, env.GetCurrentUserHomeDir, newFs, p.dbFactoryUrl, "TODO") + p.applyDBLoadParamsToEnv(newEnv) return p.registerNewDatabase(ctx, exactCaseName, newEnv) } @@ -882,6 +954,9 @@ func (p *DoltDatabaseProvider) registerNewDatabase(ctx *sql.Context, name string return fmt.Errorf("unable to register new database without database provider mutex being locked") } + // Ensure any provider-supplied DB load params are applied before any lazy DB load occurs. + p.applyDBLoadParamsToEnv(newEnv) + fkChecks, err := ctx.GetSessionVariable(ctx, "foreign_key_checks") if err != nil { return err diff --git a/go/libraries/doltcore/sqle/statspro/controller.go b/go/libraries/doltcore/sqle/statspro/controller.go index df17e7f1466..4b70bf51608 100644 --- a/go/libraries/doltcore/sqle/statspro/controller.go +++ b/go/libraries/doltcore/sqle/statspro/controller.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "log" + "maps" "path" "path/filepath" "strings" @@ -588,7 +589,11 @@ func (sc *StatsController) initStorage(ctx context.Context, fs filesys.Filesys) return nil, fmt.Errorf("unable to make directory '%s', cause: %s", dbfactory.DoltStatsDir, err.Error()) } - dEnv = env.Load(ctx, sc.hdpEnv.GetUserHomeDir, statsFs, urlPath, "test") + // Use LoadWithoutDB so DB load params can be applied before any DB is opened. + dEnv = env.LoadWithoutDB(ctx, sc.hdpEnv.GetUserHomeDir, statsFs, urlPath, doltversion.Version) + if sc.hdpEnv != nil && len(sc.hdpEnv.DBLoadParams) > 0 { + dEnv.DBLoadParams = maps.Clone(sc.hdpEnv.DBLoadParams) + } err = dEnv.InitRepo(ctx, types.Format_Default, "stats", "stats@stats.com", env.DefaultInitBranch) if err != nil { return nil, err @@ -597,6 +602,9 @@ func (sc *StatsController) initStorage(ctx context.Context, fs filesys.Filesys) return nil, fmt.Errorf("file exists where the dolt stats directory should be") } else { dEnv = env.LoadWithoutDB(ctx, sc.hdpEnv.GetUserHomeDir, statsFs, "", doltversion.Version) + if sc.hdpEnv != nil && len(sc.hdpEnv.DBLoadParams) > 0 { + dEnv.DBLoadParams = maps.Clone(sc.hdpEnv.DBLoadParams) + } } if err := dEnv.LoadDoltDBWithParams(ctx, types.Format_Default, urlPath, statsFs, params); err != nil { diff --git a/go/libraries/doltcore/sqle/statspro/listener.go b/go/libraries/doltcore/sqle/statspro/listener.go index 46caf0bd928..e82043fb007 100644 --- a/go/libraries/doltcore/sqle/statspro/listener.go +++ b/go/libraries/doltcore/sqle/statspro/listener.go @@ -242,12 +242,17 @@ func (sc *StatsController) Gc(ctx *sql.Context) error { } func (sc *StatsController) Close() { + var ( + doneCh chan struct{} + kv StatsKv + ) + sc.mu.Lock() - defer sc.mu.Unlock() // Already closed. select { case <-sc.closed: + sc.mu.Unlock() return default: } @@ -260,5 +265,26 @@ func (sc *StatsController) Close() { sc.signalListener(leStop) close(sc.closed) + doneCh = sc.workerDoneCh + kv = sc.kv + sc.mu.Unlock() + + // Best-effort wait for worker exit to avoid racing a close of underlying storage. + if doneCh != nil { + select { + case <-doneCh: + case <-time.After(1 * time.Second): + } + } + + // If we're using a prolly-backed stats store, it owns a DoltDB with its own filesystem locks. + // Close it best-effort on shutdown so embedded callers can reopen without contention. + if ps, ok := kv.(*prollyStats); ok && ps != nil && ps.destDb != nil { + for _, ddb := range ps.destDb.DoltDatabases() { + if ddb != nil { + _ = ddb.Close() + } + } + } return } From 80225a3ade733d5e077d883c67a60d49dd288895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?coffeegoddd=E2=98=95=EF=B8=8F=E2=9C=A8?= Date: Tue, 27 Jan 2026 13:28:53 -0800 Subject: [PATCH 2/2] /go/libraries/doltcore/sqle/logictest/dolt/doltharness.go: dont close underlying database --- go/libraries/doltcore/sqle/logictest/dolt/doltharness.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/libraries/doltcore/sqle/logictest/dolt/doltharness.go b/go/libraries/doltcore/sqle/logictest/dolt/doltharness.go index 85333a298d9..369a869f133 100644 --- a/go/libraries/doltcore/sqle/logictest/dolt/doltharness.go +++ b/go/libraries/doltcore/sqle/logictest/dolt/doltharness.go @@ -56,7 +56,10 @@ type DoltHarness struct { func (h *DoltHarness) Close() { dbs := h.sess.Provider().AllDatabases(sql.NewEmptyContext()) for _, db := range dbs { - db.(dsess.SqlDatabase).DbData().Ddb.Close() + // Close the sql-layer database resources (global state, background threads, etc). + // Do NOT close the underlying DoltDB here; this harness reuses a shared *env.DoltEnv + // across multiple init/teardown cycles (see doltharness_test.go). + db.(dsess.SqlDatabase).Close() } }