From 8dcf74117e4d9dbf448d39a2a8f9eb8c3a4d89cc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 10:41:37 +0000 Subject: [PATCH 1/3] CRuby Db: page-cache/mmap pragmas + checkpoint flock (#17) Tip already had WAL, pool, write permit, and off-path PASSIVE/RESTART checkpoints for every --target ruby app. Two remaining generalizable gaps: - Apply cache_size=-65536 and mmap_size=256MiB on every pooled open (Spinel's measured knobs; tip CRuby still inherited SQLite defaults). - Non-blocking flock around the background checkpoint tick so only one WEB_CONCURRENCY worker copies the WAL (Campfire once-campfire#319's piece RH did not yet share). Pin with db_sqlite_concurrency tests for the pragmas and exclusive flock. Correct a stale Spinel comment that claimed CRuby left synchronous unset. Co-Authored-By: Cursor Agent Co-authored-by: Thomas Klemm --- runtime/spinel/db.rb | 8 +++--- runtime/spinel/db_cruby.rb | 48 ++++++++++++++++++++++++++++++++++ tests/db_sqlite_concurrency.rs | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 4 deletions(-) diff --git a/runtime/spinel/db.rb b/runtime/spinel/db.rb index ef0c2af67..c2cd0f905 100644 --- a/runtime/spinel/db.rb +++ b/runtime/spinel/db.rb @@ -900,10 +900,10 @@ class DbPool # compile-time default nobody had written down. # # Which is why this is SET rather than left to agree by accident. - # The sibling shims (db_cruby.rb, db_jruby.rb) still set nothing and - # happen to read NORMAL today; a gem rebuilt with a different - # default would move them silently, and that is the shape of bug - # this line exists to remove. + # The sibling shims (db_cruby.rb, db_jruby.rb) also state + # synchronous=NORMAL (and WAL) explicitly; leaving it to the gem's + # bundled default would move them silently on a rebuild, and that + # is the shape of bug this line exists to remove. # # The trade is the standard WAL one and worth stating: NORMAL is # safe across a process crash and risks only the most recent commits diff --git a/runtime/spinel/db_cruby.rb b/runtime/spinel/db_cruby.rb index 5e82206f1..8c449ac1d 100644 --- a/runtime/spinel/db_cruby.rb +++ b/runtime/spinel/db_cruby.rb @@ -41,6 +41,7 @@ # target compiles against. require "sqlite3" +require "fileutils" module Db @pool = nil @@ -121,6 +122,16 @@ def self.open_connection # compile-time accident is not agreement, so each states it. db.execute("PRAGMA journal_mode=WAL") db.execute("PRAGMA synchronous=NORMAL") + # Page cache + mmap — same measured knobs as runtime/spinel/db.rb + # (roundhouse#17 CRuby half). SQLite's default cache is 2 MB; a + # long-lived serving process re-reads the working set from the OS + # on every visit. Spinel measured ~400 pread64s per /top visit at + # the default vs a large cache; tip CRuby still opened at + # cache_size=-2000 / mmap_size=0. Negative cache_size is a KiB + # budget (-65536 = 64 MiB); mmap_size maps the file so hits skip + # the read() copy. Harmless on :memory: (pages are already heap). + db.execute("PRAGMA cache_size=-65536") + db.execute("PRAGMA mmap_size=268435456") # The gem's default is 0: a second writer fails at once with # SQLITE_BUSY. Rails' database.yml says `timeout: 5000`, and so do # the binary's PRAGMAS — the harness's file database (see @@ -567,11 +578,46 @@ def self.start_checkpointer end end + # One checkpointer across WEB_CONCURRENCY / Resque siblings (the bit + # Campfire once-campfire#319 adds on top of the same PASSIVE loop). + # Without a flock every forked worker runs its own 250ms PASSIVE and + # they contend on the WAL; with it, losers skip the tick. Lock file + # sits next to the database so each file-backed DB has its own. + def self.checkpoint_lock_path(path) + File.join(File.dirname(path), ".#{File.basename(path)}.wal_checkpoint.lock") + end + + def self.try_checkpoint_lock(lock_path) + FileUtils.mkdir_p(File.dirname(lock_path)) + file = File.open(lock_path, File::RDWR | File::CREAT, 0644) + if file.flock(File::LOCK_EX | File::LOCK_NB) + file + else + file.close + nil + end + rescue StandardError + nil + end + + def self.release_checkpoint_lock(file) + return if file.nil? + begin + file.flock(File::LOCK_UN) + ensure + file.close + end + rescue StandardError + end + def self.checkpoint_loop(path) conn = SQLite3::Database.new(path) conn.busy_handler_timeout = 100 + lock_path = checkpoint_lock_path(path) loop do sleep CHECKPOINT_INTERVAL + lock = try_checkpoint_lock(lock_path) + next if lock.nil? begin row = conn.execute("PRAGMA wal_checkpoint(PASSIVE)")[0] log_frames = row.nil? ? 0 : row[1].to_i @@ -587,6 +633,8 @@ def self.checkpoint_loop(path) rescue StandardError # A busy or failed checkpoint is retried on the next tick; the # log only grows meanwhile. + ensure + release_checkpoint_lock(lock) end end end diff --git a/tests/db_sqlite_concurrency.rs b/tests/db_sqlite_concurrency.rs index 75ff8768a..fc80827a5 100644 --- a/tests/db_sqlite_concurrency.rs +++ b/tests/db_sqlite_concurrency.rs @@ -182,6 +182,53 @@ check("the abandoned insert rolled back", count("t") == 1) ); } +/// Serving connections pin the measured page-cache / mmap knobs +/// (roundhouse#17 CRuby half). Tip used to inherit SQLite defaults +/// (`cache_size=-2000`, `mmap_size=0`) while the Spinel shim already +/// set the 64 MiB / 256 MiB budget — same working-set win, every +/// RH CRuby app, not Campfire-specific. +#[test] +fn open_connection_sets_cache_and_mmap() { + run( + "pragmas", + r#" +Db.with_connection do + c = Db.current_dbh + check("journal_mode=WAL", c.execute("PRAGMA journal_mode")[0][0].to_s.downcase == "wal") + check("synchronous=NORMAL", c.execute("PRAGMA synchronous")[0][0] == 1) + check("cache_size=-65536", c.execute("PRAGMA cache_size")[0][0] == -65536) + check("mmap_size=256MiB", c.execute("PRAGMA mmap_size")[0][0] == 268435456) +end +"#, + ); +} + +/// Multi-worker checkpointing takes a non-blocking flock so only one +/// process copies the WAL (Campfire once-campfire#319's generalizable +/// bit). A second holder must fail while the first still holds it. +#[test] +fn checkpoint_flock_is_exclusive_across_processes() { + run( + "checkpoint_flock", + r#" +path = Db.instance_variable_get(:@path) +lock_path = Db.checkpoint_lock_path(path) +held = Db.try_checkpoint_lock(lock_path) +check("first holder acquired", !held.nil?) +pid = fork do + other = Db.try_checkpoint_lock(lock_path) + exit(other.nil? ? 0 : 1) +end +_pid, status = Process.wait2(pid) +check("sibling failed to take the flock", status.exitstatus == 0) +Db.release_checkpoint_lock(held) +again = Db.try_checkpoint_lock(lock_path) +check("lock free after release", !again.nil?) +Db.release_checkpoint_lock(again) +"#, + ); +} + /// Once the server asks, serving connections stop checkpointing inside /// COMMIT and a background thread copies the log into the database /// file instead: the file grows without any request checkpointing. From 0c97c9247beb11d450b8072fcf7ca6fa8ab63f88 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 11:17:33 +0000 Subject: [PATCH 2/3] CRuby checkpoint lock: close FD, skip only on contention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit on #548: File.open then a raising flock leaked the descriptor, and try_checkpoint_lock treated mkdir/open errors the same as a held lock — skipping the tick forever after wal_autocheckpoint=0. Close the file on the error path; return :busy only for LOCK_NB contention and nil for setup failure so checkpoint_loop still copies the WAL. Hold the flock for the inner loop (Campfire #319), not per tick. Co-Authored-By: Cursor Agent Co-authored-by: Thomas Klemm --- runtime/spinel/db_cruby.rb | 51 ++++++++++++++++++++++------------ tests/db_sqlite_concurrency.rs | 14 +++++++--- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/runtime/spinel/db_cruby.rb b/runtime/spinel/db_cruby.rb index 8c449ac1d..1718db2dc 100644 --- a/runtime/spinel/db_cruby.rb +++ b/runtime/spinel/db_cruby.rb @@ -587,16 +587,22 @@ def self.checkpoint_lock_path(path) File.join(File.dirname(path), ".#{File.basename(path)}.wal_checkpoint.lock") end + # File on acquire, `:busy` when another process holds the flock, `nil` + # when the lock file cannot be used (mkdir/open/flock error). Callers + # skip only on `:busy`; `nil` still checkpoints so a broken lock path + # cannot disable WAL copy after `wal_autocheckpoint=0`. def self.try_checkpoint_lock(lock_path) + file = nil FileUtils.mkdir_p(File.dirname(lock_path)) file = File.open(lock_path, File::RDWR | File::CREAT, 0644) - if file.flock(File::LOCK_EX | File::LOCK_NB) - file - else - file.close - nil - end + return file if file.flock(File::LOCK_EX | File::LOCK_NB) + file.close + :busy rescue StandardError + begin + file.close if file && !file.closed? + rescue StandardError + end nil end @@ -615,24 +621,33 @@ def self.checkpoint_loop(path) conn.busy_handler_timeout = 100 lock_path = checkpoint_lock_path(path) loop do - sleep CHECKPOINT_INTERVAL + # Hold the flock for the whole inner loop (Campfire #319), not + # per tick: releasing every 250ms lets a sibling overlap a + # PASSIVE with ours. Losers sleep and retry; a dead winner + # drops the flock so another worker takes over. lock = try_checkpoint_lock(lock_path) - next if lock.nil? + if lock == :busy + sleep CHECKPOINT_INTERVAL + next + end begin - row = conn.execute("PRAGMA wal_checkpoint(PASSIVE)")[0] - log_frames = row.nil? ? 0 : row[1].to_i - if log_frames >= CHECKPOINT_RESTART_FRAMES - if acquire_permit - begin - conn.execute("PRAGMA wal_checkpoint(RESTART)") - ensure - release_permit + loop do + sleep CHECKPOINT_INTERVAL + row = conn.execute("PRAGMA wal_checkpoint(PASSIVE)")[0] + log_frames = row.nil? ? 0 : row[1].to_i + if log_frames >= CHECKPOINT_RESTART_FRAMES + if acquire_permit + begin + conn.execute("PRAGMA wal_checkpoint(RESTART)") + ensure + release_permit + end end end end rescue StandardError - # A busy or failed checkpoint is retried on the next tick; the - # log only grows meanwhile. + # A busy or failed checkpoint is retried on the next outer + # pass; the log only grows meanwhile. ensure release_checkpoint_lock(lock) end diff --git a/tests/db_sqlite_concurrency.rs b/tests/db_sqlite_concurrency.rs index fc80827a5..4e9cf0837 100644 --- a/tests/db_sqlite_concurrency.rs +++ b/tests/db_sqlite_concurrency.rs @@ -214,17 +214,23 @@ fn checkpoint_flock_is_exclusive_across_processes() { path = Db.instance_variable_get(:@path) lock_path = Db.checkpoint_lock_path(path) held = Db.try_checkpoint_lock(lock_path) -check("first holder acquired", !held.nil?) +check("first holder acquired", held.is_a?(File)) pid = fork do other = Db.try_checkpoint_lock(lock_path) - exit(other.nil? ? 0 : 1) + exit(other == :busy ? 0 : 1) end _pid, status = Process.wait2(pid) -check("sibling failed to take the flock", status.exitstatus == 0) +check("sibling saw :busy", status.exitstatus == 0) Db.release_checkpoint_lock(held) again = Db.try_checkpoint_lock(lock_path) -check("lock free after release", !again.nil?) +check("lock free after release", again.is_a?(File)) Db.release_checkpoint_lock(again) +# mkdir_p fails when the parent is a file — that is setup failure, not +# contention; the loop must still checkpoint (nil, not :busy). +parent = File.join(File.dirname(path), "not-a-dir") +File.write(parent, "x") +got = Db.try_checkpoint_lock(File.join(parent, "x.lock")) +check("setup failure is nil so checkpoint still runs", got.nil?) "#, ); } From 15befcd87ad0b5dbe55eb44e8bbddbd5b075f944 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 11:58:00 +0000 Subject: [PATCH 3/3] CRuby Db: close inherited checkpoint flock after fork CodeRabbit on #548: if a process forks after acquiring the WAL checkpoint flock, the child keeps the OFD open without a checkpointer thread. Closing that FD in adopt_after_fork (without LOCK_UN) lets a surviving worker take the lock after the parent exits; unlocking would drop the parent's hold. Retain the File on Db; pin with a concurrency test. Co-Authored-By: Cursor Agent Co-authored-by: Thomas Klemm --- runtime/spinel/db_cruby.rb | 25 +++++++++++++++++++++++-- tests/db_sqlite_concurrency.rs | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/runtime/spinel/db_cruby.rb b/runtime/spinel/db_cruby.rb index 1718db2dc..f8702d2aa 100644 --- a/runtime/spinel/db_cruby.rb +++ b/runtime/spinel/db_cruby.rb @@ -64,6 +64,10 @@ module Db # lease. See `start_checkpointer`. @checkpoint_wanted = false @checkpointer_pid = nil + # The process-shared flock File while this process holds it. Retained + # so `adopt_after_fork` can close the child's inherited copy without + # LOCK_UN (parent keeps the lock). Cleared on release. + @checkpoint_lock_file = nil # Failed resources stay reachable, but never re-enter the free list. @quarantined = [] @missing_connections = 0 @@ -179,6 +183,19 @@ def self.adopt_after_fork # Re-checked under the lock: every worker thread in a fresh child # reaches this together on the first request. if @owner_pid != Process.pid + # Drop the inherited checkpoint-lock FD without LOCK_UN. The + # parent may still hold the flock via its own descriptor; if we + # unlocked here we would release the parent's hold. Closing the + # child copy lets a surviving worker acquire after the parent + # exits (Puma preload / clustered fork after checkpointer start). + inherited = @checkpoint_lock_file + if inherited && !inherited.closed? + begin + inherited.close + rescue StandardError + end + end + @checkpoint_lock_file = nil # The parent's handles are simply dropped. They are already # discarded by the gem's fork safety, and closing a descriptor # this process shares with its parent is not ours to do. @@ -595,7 +612,10 @@ def self.try_checkpoint_lock(lock_path) file = nil FileUtils.mkdir_p(File.dirname(lock_path)) file = File.open(lock_path, File::RDWR | File::CREAT, 0644) - return file if file.flock(File::LOCK_EX | File::LOCK_NB) + if file.flock(File::LOCK_EX | File::LOCK_NB) + @checkpoint_lock_file = file + return file + end file.close :busy rescue StandardError @@ -607,11 +627,12 @@ def self.try_checkpoint_lock(lock_path) end def self.release_checkpoint_lock(file) - return if file.nil? + return if file.nil? || !file.is_a?(File) begin file.flock(File::LOCK_UN) ensure file.close + @checkpoint_lock_file = nil if @checkpoint_lock_file.equal?(file) end rescue StandardError end diff --git a/tests/db_sqlite_concurrency.rs b/tests/db_sqlite_concurrency.rs index 4e9cf0837..4bc2be9e0 100644 --- a/tests/db_sqlite_concurrency.rs +++ b/tests/db_sqlite_concurrency.rs @@ -235,6 +235,40 @@ check("setup failure is nil so checkpoint still runs", got.nil?) ); } +/// After fork, the child must close the inherited checkpoint-lock FD +/// without LOCK_UN. Otherwise a surviving worker keeps the OFD open +/// after the parent exits and its new checkpointer stays `:busy` forever +/// while `wal_autocheckpoint=0` (CodeRabbit on #548 / Puma preload). +#[test] +fn adopt_after_fork_closes_inherited_checkpoint_lock() { + run( + "checkpoint_flock_fork", + r#" +path = Db.instance_variable_get(:@path) +lock_path = Db.checkpoint_lock_path(path) +held = Db.try_checkpoint_lock(lock_path) +check("parent acquired", held.is_a?(File)) +check("Db retained lock file", Db.instance_variable_get(:@checkpoint_lock_file).equal?(held)) +r, w = IO.pipe +pid = fork do + w.close + r.read(1) + Db.adopt_after_fork + got = Db.try_checkpoint_lock(lock_path) + exit(got.is_a?(File) ? 0 : 1) +end +r.close +# Drop the parent's FD without unlocking — same as the parent exiting +# while the child still holds an inherited copy of the OFD. +held.close +w.write("x") +w.close +_pid, status = Process.wait2(pid) +check("child acquired after adopt closed inherited FD", status.exitstatus == 0) +"#, + ); +} + /// Once the server asks, serving connections stop checkpointing inside /// COMMIT and a background thread copies the log into the database /// file instead: the file grows without any request checkpointing.