From 946e2b83a1f6c6ae119427c09e0a5802940b82ff Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Mon, 3 Aug 2026 23:36:05 +0200 Subject: [PATCH] fix(db, model): better handle db connections, puller concurrency, avoid deadlock (fixes #10841) (#10842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is a danger of deadlock wherever we have database operations inside a database iterator. The iterator itself pins a connection, so the nested operations need another connection; if we've reached maxOpenConns then it blocks until a connection is released. If all connections are consumed by such iterators then none can make progress. Luckily, we have limited concurrency for most such iterator loops. They are part of scanning, pulling, reverting, etc where there is only ever one such routine per folder. The exception is block reuse in the copier routine, which is limited by the `Copiers` setting per folder. This could in practice deadlock since you could set copiers to eight and end up with six `AllLocalBlocksWithHash` iterators when `maxDBConns=6`, all of which need to make additional database calls inside the loop. This PR fixes the problem twice; - The problematic loop does not need to be reentrant. The set of blocks that may be returned by the iterator is finite so we can easily just collect them to a slice before we start processing them. This avoids the problem entirely. - We do not need to limit database connections as strictly as we currently do. Increase the maximum allowed, while reducing the number of held-open idle connections slightly. This is not an exact science, but ideally we want "most" operations to be able to use the pinned connections to avoid cache churn. Most operations are short lived queries, or single-goroutine iterators with short lived queries inside, so four connections seems like it should usually be enough. 🤷 - Additionally, set a cap on Copiers. Currently you could set it to an arbitrarily large number, which is not advantageous. Limit it to 2*NumCPU which scales somewhat with system performance. Signed-off-by: Jakob Borg --- internal/db/sqlite/basedb.go | 6 +++--- internal/db/sqlite/db_open.go | 20 +++++++++++++++++--- internal/db/sqlite/folderdb_open.go | 4 ++-- lib/model/folder_sendrecv.go | 19 +++++++++++-------- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/internal/db/sqlite/basedb.go b/internal/db/sqlite/basedb.go index 168fb5c11..71ca3105b 100644 --- a/internal/db/sqlite/basedb.go +++ b/internal/db/sqlite/basedb.go @@ -49,7 +49,7 @@ type baseDB struct { tplInput map[string]any } -func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScripts []string) (*baseDB, error) { +func openBase(path string, maxOpenConns, maxIdleConns int, pragmas, schemaScripts, migrationScripts []string) (*baseDB, error) { // Open the database with options to enable foreign keys and recursive // triggers (needed for the delete+insert triggers on row replace). pathURL := url.URL{ @@ -62,8 +62,8 @@ func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScript return nil, wrap(err) } - sqlDB.SetMaxOpenConns(maxConns) - sqlDB.SetMaxIdleConns(maxConns) + sqlDB.SetMaxOpenConns(maxOpenConns) + sqlDB.SetMaxIdleConns(maxIdleConns) for _, pragma := range pragmas { if _, err := sqlDB.Exec("PRAGMA " + pragma); err != nil { diff --git a/internal/db/sqlite/db_open.go b/internal/db/sqlite/db_open.go index 0f55282cb..000ba005d 100644 --- a/internal/db/sqlite/db_open.go +++ b/internal/db/sqlite/db_open.go @@ -11,6 +11,7 @@ import ( "log/slog" "os" "path/filepath" + "runtime" "sync" "time" @@ -20,10 +21,23 @@ import ( ) const ( - maxDBConns = 6 + // maxIdleConns is set so that most db operations will usually fit + // within one of those connections and hence use their persistent page + // cache. Additional connections on top of these will allocate their own + // page cache (same as any other), but it will be deallocated on + // connection close. + maxIdleConns = 4 + minDeleteRetention = 24 * time.Hour ) +// maxOpenConns is sized for handling spikes. The primary driver is the +// Copiers folder option which may result in up to 2*NumCPU concurrent +// iterations. We reserve additional space on top of this to serve +// additional operations, some of which may be reentrant (queries within +// iterators) without deadlock. +var maxOpenConns = max(16, 4*runtime.NumCPU()) + type DB struct { *baseDB @@ -69,7 +83,7 @@ func Open(path string, opts ...Option) (*DB, error) { initTmpDir(path) mainPath := filepath.Join(path, "main.db") - mainBase, err := openBase(mainPath, maxDBConns, pragmas, schemas, migrations) + mainBase, err := openBase(mainPath, maxOpenConns, maxIdleConns, pragmas, schemas, migrations) if err != nil { return nil, err } @@ -120,7 +134,7 @@ func OpenForMigration(path string) (*DB, error) { initTmpDir(path) mainPath := filepath.Join(path, "main.db") - mainBase, err := openBase(mainPath, 1, pragmas, schemas, migrations) + mainBase, err := openBase(mainPath, 1, 1, pragmas, schemas, migrations) if err != nil { return nil, err } diff --git a/internal/db/sqlite/folderdb_open.go b/internal/db/sqlite/folderdb_open.go index b1b4ec95d..6b5da5704 100644 --- a/internal/db/sqlite/folderdb_open.go +++ b/internal/db/sqlite/folderdb_open.go @@ -38,7 +38,7 @@ func openFolderDB(folder, path string, deleteRetention time.Duration) (*folderDB "sql/migrations/folder/*", } - base, err := openBase(path, maxDBConns, pragmas, schemas, migrations) + base, err := openBase(path, maxOpenConns, maxIdleConns, pragmas, schemas, migrations) if err != nil { return nil, err } @@ -75,7 +75,7 @@ func openFolderDBForMigration(folder, path string, deleteRetention time.Duration "sql/schema/folder/*", } - base, err := openBase(path, 1, pragmas, schemas, nil) + base, err := openBase(path, 1, 1, pragmas, schemas, nil) if err != nil { return nil, err } diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index e86011982..316cb8bf7 100644 --- a/lib/model/folder_sendrecv.go +++ b/lib/model/folder_sendrecv.go @@ -15,6 +15,7 @@ import ( "io" "log/slog" "path/filepath" + "runtime" "slices" "strconv" "strings" @@ -148,6 +149,8 @@ func newSendReceiveFolder(model *model, ignores *ignore.Matcher, cfg config.Fold // copy step. TODO: Rename this config option at some point. f.Copiers = defaultCopiers } + // Cap the copiers at 2*NumCPU, so that we have a known upper bound. + f.Copiers = min(f.Copiers, 2*runtime.NumCPU()) // If the configured max amount of pending data is zero, we use the // default. If it's configured to something non-zero but less than the @@ -1438,15 +1441,15 @@ func (f *sendReceiveFolder) copyBlock(ctx context.Context, block protocol.BlockI // Returns true when the block was successfully copied. // The passed buffer must be large enough to accommodate the block. func (f *sendReceiveFolder) copyBlockFromFolder(ctx context.Context, folderID string, block protocol.BlockInfo, state copyBlocksState, ffs fs.Filesystem, buf []byte) bool { - for e, err := range itererr.Zip(f.model.sdb.AllLocalBlocksWithHash(folderID, block.Hash)) { - if err != nil { - // We just ignore this and continue pulling instead (though - // there's a good chance that will fail too, if the DB is - // unhealthy). - f.sl.DebugContext(ctx, "Failed to get block information from database", "blockHash", block.Hash, slogutil.FilePath(state.file.Name), slogutil.Error(err)) - return false - } + candidates, err := itererr.Collect(f.model.sdb.AllLocalBlocksWithHash(folderID, block.Hash)) + if err != nil { + // We just ignore this and continue pulling instead (though there's + // a good chance that will fail too, if the DB is unhealthy). + f.sl.DebugContext(ctx, "Failed to get block information from database", "blockHash", block.Hash, slogutil.FilePath(state.file.Name), slogutil.Error(err)) + return false + } + for _, e := range candidates { if !f.copyBlockFromFile(ctx, e.FileName, e.Offset, state, ffs, block, buf) { if state.failed() != nil { return false