feat: Database#discard prevent corruption after fork()

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

Author: flavorjones

adr/2024-09-fork-safety.md

@@ -0,0 +1,57 @@
+
+# 2024-09 Implement `Database#discard` to mitigate sqlite's lack of fork safety
+
+## Status
+
+Proposed, seeking feedback and suggestions for other alternative solutions.
+
+
+## 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 (which will result in a memory leak in the child process). Neither of these options are great, see below.
+
+
+## Decision
+
+The sqlite3-ruby gem will introduce support for `Database#discard` in v2.1.0, which can be called by frameworks like Rails that have made the decision to allow forking while connections are open. This method will close the underlying file descriptors, but will not free any of the memory or call `sqlite3_close` to clean up internal data structures.
+
+
+## Consequences
+
+The positive consequence is that frameworks like Rails can prevent database file corruption as a result of allowing forks by simply `discard`ing the inherited connections in the child process.
+
+The negative consequence is that, when choosing this option, 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 `Database#discard` turns out to be a large source of pain.
+
+
+## References
+
+- [Database#discard 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

@@ -155,6 +155,43 @@ closed_p(VALUE self)
     return Qfalse;
 }
 
+/* call-seq: db.discard
+ *
+ * ⚠ Do not use this method unless you know what you are doing.
+ *
+ * This method is intended to be used by frameworks like Rails after a process with open databases
+ * has forked. It will leak the memory associated with the incompletely-closed connection.
+ *
+ * See adr/2024-09-fork-safety.md for more information on why this method exists.
+ */
+static VALUE
+discard(VALUE self)
+{
+  sqlite3RubyPtr ctx;
+  sqlite3_file *sfile;
+  int status;
+
+  TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx);
+
+  if (ctx->db) {
+    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;
+
+    rb_iv_set(self, "-aggregators", Qnil);
+  }
+
+  return self;
+}
+
 /* call-seq: total_changes
  *
  * Returns the total number of changes made to this database instance
@@ -890,6 +927,7 @@ init_sqlite3_database(void)
     rb_define_method(cSqlite3Database, "collation", collation, 2);
     rb_define_method(cSqlite3Database, "close", sqlite3_rb_close, 0);
     rb_define_method(cSqlite3Database, "closed?", closed_p, 0);
+    rb_define_method(cSqlite3Database, "discard", discard, 0);
     rb_define_method(cSqlite3Database, "total_changes", total_changes, 0);
     rb_define_method(cSqlite3Database, "trace", trace, -1);
     rb_define_method(cSqlite3Database, "last_insert_row_id", last_insert_row_id, 0);

test/helper.rb

@@ -20,5 +20,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

@@ -719,5 +719,36 @@ def test_transaction_returns_block_result
       result = @db.transaction { :foo }
       assert_equal :foo, result
     end
+
+    def test_discard_a_connection
+      skip("Database#discard leaks memory") if i_am_running_in_valgrind
+
+      begin
+        db = SQLite3::Database.new("test.db")
+        result = db.discard
+
+        assert_predicate(db, :closed?)
+        assert_same(db, result)
+
+        assert_nothing_raised do
+          db.close
+        end
+      ensure
+        FileUtils.rm_f("test.db")
+      end
+    end
+
+    def test_discard_a_closed_connection
+      db = SQLite3::Database.new("test.db")
+      db.close
+
+      result = nil
+      assert_nothing_raised do
+        result = db.discard
+      end
+      assert_same(db, result)
+    ensure
+      FileUtils.rm_f("test.db")
+    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