Fix read-only open during checkpoint from reading inconsistent state

[ericyuanhui]

Summary
This change fixes a bug where opening the database in read-only mode could race with an in-progress checkpoint and end up reading inconsistent on-disk state. In that situation, recovery could observe intermediate checkpoint artifacts and continue loading from partially updated metadata, which could lead to crashes such as segmentation faults.
To address that, the fix makes checkpoint progress explicit and blocks read-only recovery from entering that unsafe window. It adds coordination around checkpoint execution so read-only opens can detect that a checkpoint is underway and fail fast instead of trying to recover from an intermediate state. It also adds defensive validation when reading checkpoint metadata so invalid or out-of-range page information is rejected with a controlled error rather than being used blindly. In addition, it cleans up a related resource-handling issue in file locking so failed lock attempts do not leak OS handles.
In short, the change turns a potentially unsafe concurrent recovery path into a safe, well-defined failure mode, and adds extra validation to prevent corrupted or partial checkpoint state from causing low-level crashes.

What Changed
Added two checkpoint lock files:*.checkpoint.intent.lock
*.checkpoint.apply.lock

Added checkpoint lock acquisition/release in Checkpointeracquire at checkpoint start
release on cleanup and rollback

Added eager creation of checkpoint lock files for non-read-only persistent databases
Added read-only recovery protection in WALReplayerif a checkpoint is in progress, opening in read-only mode now throws a RuntimeException
read-only recovery no longer proceeds through shadow file / checkpoint WAL intermediate states

Added defensive validation when reading checkpoint headersvalidate catalog page range
validate metadata page range
validate dataFileNumPages against actual file size

Fixed file descriptor / handle leaks when file locking fails in LocalFileSystem
Updated WAL testsReadOnlyRecoveryWithShadowFile now expects failure in read-only mode
added ReadOnlyRecoveryWithCheckpointWAL

[adsharma]

Note that DuckDB and SQLite take different approaches here:

https://claude.ai/share/9ce5d5a8-bf34-46cb-87e7-9f1636491c4f

[ericyuanhui]

Note that DuckDB and SQLite take different approaches here:

https://claude.ai/share/9ce5d5a8-bf34-46cb-87e7-9f1636491c4f

ladybug is same like SQlite right?

[adsharma]

Do you use a filesystem that supports snapshots? From a maintainability and complexity perspective, I prefer a solution like this:

https://dev.to/shiviyer/step-by-step-implementation-filesystem-snapshots-in-postgresql-39ci

The SQLite method works even if the reader doesn't have write permissions to the dir where the database resides. Also our internals are different from SQLite and the development methodology is not as tightly controlled. That requires us to operate with simpler abstractions.

[ericyuanhui]

Do you use a filesystem that supports snapshots? From a maintainability and complexity perspective, I prefer a solution like this:

https://dev.to/shiviyer/step-by-step-implementation-filesystem-snapshots-in-postgresql-39ci

The SQLite method works even if the reader doesn't have write permissions to the dir where the database resides. Also our internals are different from SQLite and the development methodology is not as tightly controlled. That requires us to operate with simpler abstractions.

What I'm asking is: is there anything wrong with my approach to this modification and implementation?
Looking at Ladybug's design philosophy — since Ladybug supports both read-write mode and read-only mode, if I need to have one connection accessing the same database file in read-write mode while another connection accesses it in read-only mode, then we'd end up with the intermediate state issue I mentioned earlier. That's why we need to add a locking mechanism to safeguard consistency.

[adsharma]

Let me come back to this after the 0.18.x release. Requires more careful consideration.

[ericyuanhui]

Let me come back to this after the 0.18.x release. Requires more careful consideration.

new version is release. If we can discuss this PR?

[adsharma]

Still looking to get 0.18.1 out with some bug fixes before letting in big PRs. But this is a good time to discuss the main ideas behind the PR and why it works.

[adsharma]

None of these are an objection to the PR. Obviously what it's doing is useful based on feedback from @M0nkeyFl0wer and possibly others.

• What belongs to an embedded database vs what should be outside.

I would argue that network protocol implementation or a distributed consensus should exist in another library. Someone could then combine all 3 libraries to make a fully functional database.

• Value vs complexity trade-off

Certainly it provides value. But it also adds complexity. Which one is greater?

• Is there a strong theoretical basis for what it's doing?

I think the answer is yes, more on it in my next comment below.

• Are there other ways of doing this?

Many filesystems implement zero-copy snapshots. So one can do:

// sqlite
BEGIN EXCLUSIVE TRANSACTION;
zfs snapshot pool_name/dataset@snap_name
ROLLBACK TRANSACTION

or

// rocksdb
db->DisableFileDeletions();
// fs snapshot
db->EnableFileDeletions();

These will keep the complexity in the filesystem and/or orchestration between the fs snapshot and DB blocking of further writes.

The complexity containment is the #1 concern I have around this PR.

[adsharma]

1. SQLite — closest mechanism match

SQLite uses POSIX advisory byte-range locks on the database file itself with a five-state lock state machine:

  UNLOCKED → SHARED → RESERVED → PENDING → EXCLUSIVE

• SHARED — multiple readers hold it concurrently
• RESERVED — one writer, signaling intent to write; readers can still hold SHARED
• PENDING — writer asks all current readers to leave; blocks new readers
• EXCLUSIVE — full write lock, no readers remain

The PR's intent.lock / apply.lock pair is conceptually the same two-phase idea as RESERVED → PENDING → EXCLUSIVE, just expressed as two separate files instead of byte-range offsets inside one file. Both designs explicitly model "first announce intent, then escalate to the apply/mutate phase." SQLite's lock state machine is documented in docs/lockvar.tex and src/pager.c in the SQLite source.

2. Intention locks (Gray, 1975)

The "intention lock" terminology comes from Jim Gray's paper "Granularity of Locks and Degrees of Consistency in a Shared Database" (1975/1976), which defined the hierarchical lock modes that every textbook DBMS teaches:

• IS — Intention Shared
• IX — Intention Exclusive
• S — Shared
• X — Exclusive
• SIX — Shared Intention Exclusive

The whole point of an "intention" lock is exactly what this PR does: signal at a coarse level that you intend to take a
finer-grained exclusive lock soon, so other coarse-level lockers can coordinate without having to descend into the
fine-grained tree. The PR doesn't descend into a hierarchy, but the naming and intent is straight out of that paper.

3. ARIES (IBM) — two-phase prepare/commit

The ARIES recovery algorithm (used in IBM Db2, PostgreSQL, SQL Server, and many others) has an explicit "prepare" phase (write intent record to log, acquire locks, flush) and a separate "commit" phase (write commit record, release locks). The "intent lock" terminology shows up in the ARIES/2PL implementations of these systems. The PR's "intent" and "apply" naming is reminiscent of that two-phase prepare/apply dance.

4. PostgreSQL exclusive backup

pg_basebackup's exclusive mode uses an exclusive lock on the database plus a backup_label file. A reader doing PITR/recovery detects the backup_label and refuses to start up (you have to remove it or use the non-exclusive mode). This is the same defensive "fail closed if intermediate state is on disk" idea the PR implements, but at the file-label level rather than a separate lock file.

[adsharma]

Races the PR does not cover

1. Long-lived read-only connection vs. later writer activity (the biggest gap)

   • The reader's lock files are released the moment WALReplayer::replay() returns. From that point on, the reader runs queries with no synchronization against the writer.
   • The writer can then run a checkpoint that calls reclaimTailPagesIfNeeded(currentHeader->dataFileNumPages), shrinking the data file. The reader's buffer manager may have cached pages that are now beyond EOF, or it may read pages the writer has just overwritten.
   • The PR provides no MVCC / snapshot isolation for read-only — it only protects the open moment, not the lifetime.

2. Reader opening the DB while a writer is appending to the active (non-frozen) WAL (no checkpoint involved)

   • WALReplayer::replay reads the active WAL via openWALFile() with READ_ONLY. If the writer appends a new record concurrently, the reader can read a torn or partial record. The PR's throwIfReadOnlyCheckpointState does not trigger because the WAL is the active one, not the frozen one.
   • There is no lock file dedicated to "the writer is currently appending".

3. Same-process reader and writer (e.g., background threads, or two Database instances in one process)

   • POSIX fcntl locks are per-process, not per-file-descriptor. The writer's WRITE_LOCK and the reader's READ_LOCK on the same file will not contend in the same process.
   • This is a real concern for any architecture that puts a read-only view and a writer in the same process (replication followers, sidecar readers, embedded use).

4. Stale lock files after a writer crash (false positive, but worth flagging)

   • If the writer process is killed (SIGKILL, OOM, power loss) between acquiring the locks and postCheckpointCleanup, the lock files are released by the OS (good) but the shadow file / frozen WAL may be left behind. The next read-only open will hit throwIfReadOnlyCheckpointState and throw — a false positive that requires user retry. Safe, but not ideal.
   • There is no recovery path that distinguishes "writer crashed mid-checkpoint" from "writer is currently checkpointing" other than waiting and retrying.

5. The data file is opened with O_PERSISTENT_FILE_READ_ONLY and without O_LOCKED_PERSISTENT_FILE in read-only mode (storage_manager.cpp:61-66).

   • The reader can co-exist on the same data file as a writer that has the file locked. This is fine for file content (read-only is non-destructive) but it means the only mutual-exclusion channel is the lock files, which (a) only protect the checkpoint window and (b) are advisory on POSIX.

6. No protection if the lock files themselves are deleted or corrupted

   • Nothing prevents a user (or a misbehaving tool) from rm‑ing *.checkpoint.intent.lock mid-checkpoint. After that, the protection is gone with no warning.

7. External tools that bypass WALReplayer

   • Any tool that opens the data file directly (an offline analyzer, a backup tool, etc.) gets no protection. The lock files are only consulted by the in-process WALReplayer.

8. Race against the catalog/metadata pages being rewritten even outside checkpoint

   • DDL or metadata operations that update the catalog page range (e.g., addProperty, createIndex) write new catalog pages via the page manager. A reader that has already finished replay and is querying through cached buffers could observe torn writes. The PR adds validation only in readCheckpoint, not on the read path.

[M0nkeyFl0wer]

Do you use a filesystem that supports snapshots?

Data point from the deployment that produced the #642 repro: no. Plain ext4 on
Ubuntu 24.04 — no ZFS, no btrfs, no LVM thin pools. I'd guess that's the modal
environment for an embedded database's users: laptops, small VMs, default
distro filesystems. A solution that assumes zero-copy filesystem snapshots is
a solution most embedded users can't reach without reformatting.

What I actually deployed is the userland equivalent you're describing: the
writer owns the live file and, after each checkpoint drain (still holding its
write lock), refreshes a read-only snapshot copy that readers open exclusively.
It works — zero SIGSEGVs since — but the costs are real and worth putting on
the record for the complexity discussion:

• A full file copy per write cycle. My graph is 1.2 GB, so every drain
  cycle pays a 1.2 GB copy on ext4. Zero-copy would fix this, but see above.
• Read staleness between refreshes.
• A failure mode the engine can't see. The snapshot path, the atomic
  rename, the "never fall back to the live file" discipline — all of that
  lives in my application code, enforced by convention. Every new reader
  code path is a chance to reintroduce the crash. The engine can't help,
  because the invariant lives outside it.

That last point is my honest answer to the value-vs-complexity question. The
current contract is "read_only open racing a checkpoint is undefined behavior,
mitigate in application code," and the mitigation is subtle enough that I got
it wrong twice before getting it right: first with writer-side locking (which
can't protect a reader the engine lets open a mid-checkpoint file), then with
a snapshot scheme whose readers could silently fall back to the live file.
The version that works had to make the fallback a hard error — enforcing, in
application code, an invariant only the engine can actually see. Even if the full reader/writer
coordination story stays out of scope (your races 1–8 are a fair list, and
gap #1 in particular means this PR shouldn't be sold as general reader/writer
safety), there's a smaller contract worth buying: a failed open never takes
down the process. That's what #615 delivers on the open path, and it's the
difference between "retry loop" and "supervisor restarts my API."

On the "does this belong in the engine or outside" framing: SQLite's answer to
this exact question is instructive precisely because it put the locking state
machine inside — and that's a large part of why nobody rolls their own
concurrent-access layer around SQLite. The library that made "embedded" and
"safe concurrent access" compatible is the one that absorbed the complexity.

The #642 repro harness validates any revision of this fix in about a minute
(3/3 reader SIGSEGVs on stock 0.18.0, 0/6 with the #615 build). Happy to keep
running it against whatever shape the fix ends up taking, including a
minimal-scope version that only hardens the open path.

Empirical check of race #1 (long-lived reader vs. writer checkpoints)

Since race #1 was flagged as the biggest gap, I tested it on stock 0.18.0
(PyPI): 3 reader processes each open the DB read_only once, hold the
connection for the whole run, and query in a tight loop, while a writer
process churns the same file. Five variants over a 200k-row seed, ~80 s each,
including a delete-heavy writer (every original row deleted and re-inserted,
~3.7M rows churned in one variant) and readers running full data scans with a
16 MiB buffer pool (far smaller than the file, forcing disk re-reads on every
scan).

Result: it did not reproduce — across ~1,700 writer checkpoints and
~780k reader queries, zero crashes, zero errors, zero wrong results. Every
long-lived reader served a perfectly consistent open-time snapshot for its
entire lifetime and exited 0.

The named mechanism also appears unreachable in 0.18.0: the data file never
shrank in any run. Reading the source, reclaimTailPagesIfNeeded
(src/storage/page_manager.cpp:71) adds tail pages to the FreeSpaceManager
for reuse but nothing truncates the main data file (only the WAL is
truncated), so "reader's cached pages beyond EOF" can't happen on this
version. Caveat: this is a null result, not a proof — the overwritten-page
variant of the hazard could still exist under free-page reuse patterns my
writers didn't generate (the file grew rather than recycling the reader's
exact pages).

Two side observations from the same runs, possibly worth their own issues:
(1) checkpoint never returns disk space in 0.18.0 — delete-heavy workloads
only grow the file; (2) a held read-only connection is frozen at its
open-time snapshot forever — it never observes any writer commit, so
"long-lived reader" also means "arbitrarily stale reader." If that's the
intended semantics it would be worth documenting, because it means the
lifetime-coordination problem in race #1 is currently more about staleness
than memory safety.

If useful, I can file the race-#1 lifetime question as a separate tracking
issue so this PR's scope can stay cleanly "make the open race fail closed."

[adsharma]

A solution that assumes zero-copy filesystem snapshots is
a solution most embedded users can't reach without reformatting.

Agreed. I found other arguments you make in the comment also compelling.

This fixes a real problem. As long as we don't sell it as a complete solution for reader-writer synchronization, we're good to go.

Yes, please open an issue for tracking race #1.

[adsharma]

I'll merge this after the 0.18.1 release is made.