Repository: benbjohnson/litestream (go)

Issue:
Unclean close leaves Litestream in weird state
I've been using Litestream as a library and tried a couple of things to abuse it since the usage pattern is a bit different than when using it as a separate process.

What I'm seeing is that the final sync at the beginning of Close() will fail due to this because I had sqlite3 cli just writing while closing (and rolling in the WAL): `cannot verify wal state: stat foo/bar.db-wal: no such file or directory`. This might be fine to ignore but the caller doesn't know it and calling Close() again will fail because the underlying database connection has already been closed and will always return an error at that point.

This is very much an edge case of an edge case and it shouldn't ever really happen but the way database Close() is structured it's designed to ignore all errors and just continue but it might be better to return early and allow calling it more than once to retry if it's a passing issue or handle the errors more gracefully. Now because it's designed to ignore errors the caller needs to assume the database is somewhat closed and ignore the returned error and at most log it.

Pull request description:
Always release long-running fd on db close
If Litestream is used as a library and it can be closed and re-opened against the same database this leaking fd can break SQLite database file locking. During normal cli operation this race cannot happen as the whole process will exit after closing databases.

Closing Litestream and opening again right after will leave the unclosed file handle out-of-scope and free to garbage collect. Go tries to be helpful and closes the file handle during GC to prevent it leaking permanently but that will cause a race with POSIX advisory locks.

This order of operations will release the _real_ lock the embedded SQLite of Litestream is holding while leaving it clueless. That will allow the main application to take a new lock on the file and eventually both may be checkpointing the same WAL into the main database file causing permanent corruption.

https://www.sqlite.org/howtocorrupt.html#_posix_advisory_locks_canceled_by_a_separate_thread_doing_close_

Fixes #510

Patch:
diff --git a/db.go b/db.go
index ba914e1..2306676 100644
--- a/db.go
+++ b/db.go
@@ -317,14 +317,11 @@ func (db *DB) Open() (err error) {
 }
 
 // Close flushes outstanding WAL writes to replicas, releases the read lock,
-// and closes the database.
-func (db *DB) Close() (err error) {
+// and closes the database. Takes a context for final sync.
+func (db *DB) Close(ctx context.Context) (err error) {
 	db.cancel()
 	db.wg.Wait()
 
-	// Start a new context for shutdown since we canceled the DB context.
-	ctx := context.Background()
-
 	// Perform a final db sync, if initialized.
 	if db.db != nil {
 		if e := db.Sync(ctx); e != nil && err == nil {
@@ -355,6 +352,12 @@ func (db *DB) Close() (err error) {
 		}
 	}
 
+	if db.f != nil {
+		if e := db.f.Close(); e != nil && err == nil {
+			err = e
+		}
+	}
+
 	return err
 }
 


Test patch:
diff --git a/cmd/litestream/replicate.go b/cmd/litestream/replicate.go
index 898e79d..988407c 100644
--- a/cmd/litestream/replicate.go
+++ b/cmd/litestream/replicate.go
@@ -168,7 +168,7 @@ func (c *ReplicateCommand) Run() (err error) {
 // Close closes all open databases.
 func (c *ReplicateCommand) Close() (err error) {
 	for _, db := range c.DBs {
-		if e := db.Close(); e != nil {
+		if e := db.Close(context.Background()); e != nil {
 			db.Logger.Error("error closing db", "error", e)
 			if err == nil {
 				err = e
diff --git a/db_test.go b/db_test.go
index ef82987..c882858 100644
--- a/db_test.go
+++ b/db_test.go
@@ -264,7 +264,7 @@ func TestDB_Sync(t *testing.T) {
 		// Checkpoint & fully close which should close WAL file.
 		if err := db.Checkpoint(context.Background(), litestream.CheckpointModeTruncate); err != nil {
 			t.Fatal(err)
-		} else if err := db.Close(); err != nil {
+		} else if err := db.Close(context.Background()); err != nil {
 			t.Fatal(err)
 		} else if err := sqldb.Close(); err != nil {
 			t.Fatal(err)
@@ -314,7 +314,7 @@ func TestDB_Sync(t *testing.T) {
 		}
 
 		// Fully close which should close WAL file.
-		if err := db.Close(); err != nil {
+		if err := db.Close(context.Background()); err != nil {
 			t.Fatal(err)
 		} else if err := sqldb.Close(); err != nil {
 			t.Fatal(err)
@@ -367,7 +367,7 @@ func TestDB_Sync(t *testing.T) {
 		pos0, err := db.Pos()
 		if err != nil {
 			t.Fatal(err)
-		} else if err := db.Close(); err != nil {
+		} else if err := db.Close(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
@@ -413,7 +413,7 @@ func TestDB_Sync(t *testing.T) {
 		}
 
 		// Close & truncate shadow WAL to simulate a partial header write.
-		if err := db.Close(); err != nil {
+		if err := db.Close(context.Background()); err != nil {
 			t.Fatal(err)
 		} else if err := os.Truncate(db.ShadowWALPath(pos0.Generation, pos0.Index), litestream.WALHeaderSize-1); err != nil {
 			t.Fatal(err)
@@ -458,7 +458,7 @@ func TestDB_Sync(t *testing.T) {
 		}
 
 		// Close & truncate shadow WAL to simulate a partial frame write.
-		if err := db.Close(); err != nil {
+		if err := db.Close(context.Background()); err != nil {
 			t.Fatal(err)
 		} else if err := os.Truncate(db.ShadowWALPath(pos0.Generation, pos0.Index), fi.Size()-1); err != nil {
 			t.Fatal(err)
@@ -504,7 +504,7 @@ func TestDB_Sync(t *testing.T) {
 		}
 
 		// Close & delete shadow WAL to simulate dir created but not WAL.
-		if err := db.Close(); err != nil {
+		if err := db.Close(context.Background()); err != nil {
 			t.Fatal(err)
 		} else if err := os.Remove(db.ShadowWALPath(pos0.Generation, pos0.Index)); err != nil {
 			t.Fatal(err)
@@ -626,7 +626,7 @@ func MustOpenDBAt(tb testing.TB, path string) *litestream.DB {
 // MustCloseDB closes db and removes its parent directory.
 func MustCloseDB(tb testing.TB, db *litestream.DB) {
 	tb.Helper()
-	if err := db.Close(); err != nil && !strings.Contains(err.Error(), `database is closed`) {
+	if err := db.Close(context.Background()); err != nil && !strings.Contains(err.Error(), `database is closed`) {
 		tb.Fatal(err)
 	} else if err := os.RemoveAll(filepath.Dir(db.Path())); err != nil {
 		tb.Fatal(err)