Database connections carried across fork() will not be fully closed

Sqlite is not fork-safe and closing the database connection in the
child process can lead to corruption.

Emit a warning when this happens so developers know when they're doing
something that's not supported by sqlite.

See adr/2024-09-fork-safety.md for a full explanation.

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## next / unreleased
 
+### Changed
+
+- Any database connections carried across a `fork()` will not be fully closed to help protect database files against corruption. Using a database connection in a child process that was created in a parent process is unsafe and may corrupt the database file. If an inherited connection is closed then a warning will be emitted and some reserved memory will be lost to the child process permanently. See the README "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558] @flavorjones
+
+
 ### Improved
 
 - Use `sqlite3_close_v2` to close databases in a deferred manner if there are unclosed prepared statements. Previously closing a database while statements were open resulted in a `BusyException`. See https://www.sqlite.org/c3ref/close.html for more context. [#557] @flavorjones

README.md

@@ -148,6 +148,23 @@ It is generally recommended that if applications want to share a database among
 threads, they _only_ share the database instance object.  Other objects are
 fine to share, but may require manual locking for thread safety.
 
+
+## Fork Safety
+
+[Sqlite is not fork
+safe](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_)
+and you should not carry an open database connection across a `fork()`. Using an inherited
+connection in the child may corrupt your database, leak memory, or cause other undefined behavior.
+
+Instead, whenever possible, close connections in the parent before forking.
+
+If that's not possible or convenient, then immediately close any inherited connections in the child
+after forking, before opening any new connections. This will incur a small one-time memory leak per
+connection, but that's preferable to potentially corrupting your database.
+
+See [./adr/2024-09-fork-safety.md](./adr/2024-09-fork-safety.md) for more information and context.
+
+
 ## Support
 
 ### Installation or database extensions

adr/2024-09-fork-safety.md

@@ -0,0 +1,65 @@
+
+# 2024-09 Discard database connections when carried across fork()of fork safety
+
+## Status
+
+Accepted, but we can revisit more complex solutions if we learn something that indicates that effort is worth it.
+
+
+## Context
+
+In August 2024, Andy Croll opened an issue[^issue] describing sqlite file corruption related to solid queue. After investigation, we were able to reproduce corruption under certain circumstances when forking a process with open sqlite databases.[^repro]
+
+SQLite is known to not be fork-safe[^howto], so this was not entirely surprising though it was the first time your author had personally seen corruption in the wild. The corruption became much more likely after the sqlite3-ruby gem improved its memory management with respect to open statements[^gemleak] in v2.0.0.
+
+Advice from upstream contributors[^advice] is, essentially: don't fork if you have open database connections. Or, if you have forked, don't call `sqlite3_close` on those connections and thereby leak some amount of memory in the child process. Neither of these options are ideal, see below.
+
+
+## Decision
+
+Open database connections carried across a `fork()` will not be fully closed in the child process, to avoid the risk of corrupting the database file.
+
+The sqlite3-ruby gem will track the ID of the process that opened each database connection. If, when the database is closed (either explicitly with `Database#close` or implicitly via GC) the current process ID is different from the original process, then we "discard" the connection.
+
+"Discard" here means:
+
+- The `Database` object acts "closed", including returning `true` from `#closed?`.
+- `sqlite3_close_v2` is not called on the object, because it is unsafe to do so per sqlite instructions[^howto]. As a result, some memory will be lost permanently (a one-time "memory leak").
+- Open file descriptors associated with the database are closed.
+
+
+## Consequences
+
+The positive consequence is that we remove a potential cause of database corruption for applications that fork with active sqlite database connections.
+
+The negative consequence is that, for each discarded connection, some memory will be permanently lost (leaked) in the child process.
+
+
+## Alternatives considered.
+
+### 1. Require applications to close database connections before forking.
+
+This is the advice[^advice] given by the upstream maintainers of sqlite, and so was the first thing we tried to implement in Rails in [rails/rails#52931](https://github.com/rails/rails/pull/52931)[^before_fork]. That first simple implementation was not thread safe, however, and in order to make it thread-safe it would be necessary to pause all sqlite database activity, close the open connections, and then fork. At least one Rails core team member was not happy that this would interfere with database connections in the parent, and the complexity of a thread-safe solution seemed high, so this work was paused.
+
+### 2. Memory arena
+
+Sqlite offers a configuration option to specify custom memory functions for malloc et al. It seems possible that the sqlite3-ruby gem could implement a custom arena that would be used by sqlite so that in a new process, after forking, all the memory underlying the sqlite Ruby objects could be discarded in a single operation.
+
+I think this approach is promising, but complex and risky. Sqlite is a complex library and uses shared memory in addition to the traditional heap. Would throwing away the heap memory (the arena) result in a segfault or other undefined behaviors or corruption? Determining the answer to that question feels expensive in and of itself, and any solution along these lines would not be supported by the sqlite authors. We can explore this space if the memory leak from discarded connections turns out to be a large source of pain.
+
+
+## References
+
+- [Database connections carried across fork() will not be fully closed by flavorjones · Pull Request #558 · sparklemotion/sqlite3-ruby](https://github.com/sparklemotion/sqlite3-ruby/pull/558)
+- TODO rails pr implementing sqlite3adapter discard
+
+
+## Footnotes
+
+[^issue]: [SQLite queue database corruption · Issue #324 · rails/solid_queue](https://github.com/rails/solid_queue/issues/324)
+[^repro]: [flavorjones/2024-09-13-sqlite-corruption: Temporary repo, reproduction of sqlite database corruption.](https://github.com/flavorjones/2024-09-13-sqlite-corruption)
+[^howto]: [How To Corrupt An SQLite Database File: §2.6 Carrying an open database connection across a fork()](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_)
+[^gemleak]: [Always call sqlite3_finalize in deallocate func by haileys · Pull Request #392 · sparklemotion/sqlite3-ruby](https://github.com/sparklemotion/sqlite3-ruby/pull/392)
+[^advice]: [SQLite Forum: Correct way of carrying connections over forked processes](https://sqlite.org/forum/forumpost/1fa07728204567a0a136f442cb1c59e3117da96898b7fa3290b0063ae7f6f012)
+[^before_fork]: [SQLite3Adapter: Ensure fork-safety by flavorjones · Pull Request #52931 · rails/rails](https://github.com/rails/rails/pull/52931#issuecomment-2351365601)
+[^config]: [SQlite3 Configuration Options](https://www.sqlite.org/c3ref/c_config_covering_index_scan.html)

ext/sqlite3/database.c

@@ -12,6 +12,39 @@
 
 VALUE cSqlite3Database;
 
+static void
+close_or_discard_db(sqlite3RubyPtr ctx)
+{
+    if (ctx->db) {
+        if (ctx->owner == getpid()) {
+            // Ordinary close.
+            sqlite3_close_v2(ctx->db);
+        } else {
+            // This is an open connection carried across a fork().
+            // "Discard" it. See adr/2024-09-fork-safety.md
+            sqlite3_file *sfile;
+            int status;
+
+            rb_warning("An open sqlite database connection was inherited from a forked process and "
+                       "is being discarded. This is a memory leak. If possible, please close all sqlite "
+                       "database connections before forking.");
+
+            // close the open file descriptors
+            status = sqlite3_file_control(ctx->db, NULL, SQLITE_FCNTL_FILE_POINTER, &sfile);
+            if (status == 0 && sfile->pMethods != NULL) {
+                sfile->pMethods->xClose(sfile);
+            }
+
+            status = sqlite3_file_control(ctx->db, NULL, SQLITE_FCNTL_JOURNAL_POINTER, &sfile);
+            if (status == 0 && sfile->pMethods != NULL) {
+                sfile->pMethods->xClose(sfile);
+            }
+        }
+        ctx->db = NULL;
+    }
+}
+
+
 static void
 database_mark(void *ctx)
 {
@@ -22,11 +55,8 @@ database_mark(void *ctx)
 static void
 deallocate(void *ctx)
 {
-    sqlite3RubyPtr c = (sqlite3RubyPtr)ctx;
-    sqlite3 *db     = c->db;
-
-    if (db) { sqlite3_close_v2(db); }
-    xfree(c);
+    close_or_discard_db((sqlite3RubyPtr)ctx);
+    xfree(ctx);
 }
 
 static size_t
@@ -51,7 +81,9 @@ static VALUE
 allocate(VALUE klass)
 {
     sqlite3RubyPtr ctx;
-    return TypedData_Make_Struct(klass, sqlite3Ruby, &database_type, ctx);
+    VALUE object = TypedData_Make_Struct(klass, sqlite3Ruby, &database_type, ctx);
+    ctx->owner = getpid();
+    return object;
 }
 
 static char *
@@ -62,8 +94,6 @@ utf16_string_value_ptr(VALUE str)
     return RSTRING_PTR(str);
 }
 
-static VALUE sqlite3_rb_close(VALUE self);
-
 sqlite3RubyPtr
 sqlite3_database_unwrap(VALUE database)
 {
@@ -119,21 +149,22 @@ rb_sqlite3_disable_quirk_mode(VALUE self)
 #endif
 }
 
-/* call-seq: db.close
+/*
+ *  Close the database and release all associated resources.
  *
- * Closes this database.
+ *  ⚠ If the process that created the database forks a child process, and this method is called
+ *  from the child process, then this method will _not_ free memory resources and instead will
+ *  call discard. This is a memory leak, but is safer than risking database corruption.
+ *
+ *  See adr/2024-09-fork-safety.md for more information on fork safety.
  */
 static VALUE
 sqlite3_rb_close(VALUE self)
 {
     sqlite3RubyPtr ctx;
-    sqlite3 *db;
     TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx);
 
-    db = ctx->db;
-    CHECK(db, sqlite3_close_v2(ctx->db));
-
-    ctx->db = NULL;
+    close_or_discard_db(ctx);
 
     rb_iv_set(self, "-aggregators", Qnil);
 

ext/sqlite3/database.h

@@ -8,6 +8,7 @@ struct _sqlite3Ruby {
     VALUE busy_handler;
     int stmt_timeout;
     struct timespec stmt_deadline;
+    rb_pid_t owner;
 };
 
 typedef struct _sqlite3Ruby sqlite3Ruby;

test/helper.rb

@@ -1,10 +1,6 @@
 require "sqlite3"
 require "minitest/autorun"
 
-if ENV["GITHUB_ACTIONS"] == "true" || ENV["CI"]
-  $VERBOSE = nil
-end
-
 puts "info: ruby version: #{RUBY_DESCRIPTION}"
 puts "info: gem version: #{SQLite3::VERSION}"
 puts "info: sqlite version: #{SQLite3::SQLITE_VERSION}/#{SQLite3::SQLITE_LOADED_VERSION}"
@@ -20,5 +16,10 @@ class TestCase < Minitest::Test
     def assert_nothing_raised
       yield
     end
+
+    def i_am_running_in_valgrind
+      # https://stackoverflow.com/questions/365458/how-can-i-detect-if-a-program-is-running-from-within-valgrind/62364698#62364698
+      ENV["LD_PRELOAD"] =~ /valgrind|vgpreload/
+    end
   end
 end

test/test_database.rb

@@ -721,5 +721,43 @@ def test_transaction_returns_block_result
       result = @db.transaction { :foo }
       assert_equal :foo, result
     end
+
+    def test_discard_a_connection
+      skip("interpreter doesn't support fork") unless Process.respond_to?(:fork)
+      skip("valgrind doesn't handle forking") if i_am_running_in_valgrind
+
+      begin
+        read, write = IO.pipe
+
+        db = SQLite3::Database.new("test.db")
+        Process.fork do
+          $stderr = StringIO.new
+
+          result = db.close
+
+          write.write((result == db) ? "ok\n" : "fail\n")
+          write.write(db.closed? ? "ok\n" : "fail\n")
+          write.write($stderr.string)
+
+          write.close
+          read.close
+          exit!
+        end
+
+        assert_equal("ok", read.readline.chomp, "return value was not the database")
+        assert_equal("ok", read.readline.chomp, "closed? did not return true")
+        assert_match(
+          /warning: An open sqlite database connection was inherited from a forked process/,
+          read.readline,
+          "expected warning was not emitted"
+        )
+
+        write.close
+        read.close
+      ensure
+        db.close
+        FileUtils.rm_f("test.db")
+      end
+    end
   end
 end

test/test_resource_cleanup.rb

@@ -17,11 +17,31 @@ def test_cleanup_unclosed_statement_object
       end
     end
 
+    # # this leaks the result set
     # def test_cleanup_unclosed_resultset_object
     #   db = SQLite3::Database.new(':memory:')
     #   db.execute('create table foo(text BLOB)')
     #   stmt = db.prepare('select * from foo')
     #   stmt.execute
     # end
+
+    # # this leaks the incompletely-closed connection
+    # def test_cleanup_discarded_connections
+    #   FileUtils.rm_f "test.db"
+    #   db = SQLite3::Database.new("test.db")
+    #   db.execute("create table posts (title text)")
+    #   db.execute("insert into posts (title) values ('hello')")
+    #   db.close
+    #   100.times do
+    #     db = SQLite3::Database.new("test.db")
+    #     db.execute("select * from posts limit 1")
+    #     stmt = db.prepare("select * from posts")
+    #     stmt.execute
+    #     stmt.close
+    #     db.discard
+    #   end
+    # ensure
+    #   FileUtils.rm_f "test.db"
+    # end
   end
 end