Repository navigation
perf: O(1) pack checkpoint validation; fast read-only opens - #17
Merged
Merged
Conversation
…y cases These fail before the validation change: opening a checkpointed store scans every object row, queues behind the pack lock, and an unwatched folder opens the catalog to say so. The crash-recovery cases pin the state recovery must reach on both sides of the change.
…r settled packs Opening a store counted every object row in the active pack to validate its checkpoint, so each open, including read-only status, scaled with the store. A pack whose rows all end at or before the verified length is the same condition, read from the objects_pack_end index. A pack that already ends at its validated checkpoint is no longer recovered under the catalog and pack locks. Asking about a folder the store never watched is answered from the workspace folders before the catalog is opened.
…fix umask in portable test
… write A registered workspace is looked up before it is inserted, legacy file rows are shared only for workspaces that still own some, and deferred reference logs write the catalog only when one needed repair. The umask-dependent mode in the portable exchange test is now set explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Opening a store validated the active pack's checkpoint with two
COUNT(*)scans over every object row in the pack (1.55 s on a 1.1 M-row pack), on every open, including read-onlystatusand the unwatched-folder answer.Design it twice for the check:
pack_high_water(pack) <= verified_lenplus the existing last-object check. No schema change, no new state to keep consistent.Why A is as sound as the counts it replaces. The catalog is one SQLite database, so a crash leaves a prefix of its commits. Rows are inserted before the checkpoint is advanced, and GC and reset change rows and checkpoint in one transaction. So a crash can leave rows ahead of the checkpoint but never the checkpoint ahead of its rows. "No row ends past
verified_len" covers both things the old counts guarded: rows past the checkpoint, and a row straddling it (its end is pastverified_len). What is dropped is the equalityrows in prefix == object_count, which can only fail through outside tampering with the catalog; the last-object lookup and header read still catch an emptied or rebuilt index. Any failed check still takes the old path: reset the index and re-verify the pack from byte 0.Second change: if the pack file length equals
verified_lenand the checkpoint validates (read from one snapshot, no locks), open skips the catalog write lock and pack lock. A writer mid-append makes the length differ, so it falls through to the locked recovery as before. Any frontier (group commit) disagreement also falls through.Third: the rest of open stops asking for the catalog write lock when it has nothing to write. A registered workspace is looked up before the insert; legacy file rows are shared only for workspaces that still own some (one indexed lookup each); deferred reference logs write the catalog only when a log needed repair. A busy catalog on workspace registration is reported as the same writer-wait error as before (the stalled-writer test caught this).
Fourth: status of a watched folder is a pure read. It created
retention.log/retention.lockfor a workspace that had never pinned; it now reads an absent log as the initial state. Test: the directory tree is identical before and afterstatus.Fifth: the dropped prefix-count check is kept as an explicit, off-critical-path check.
A damaged record fails the rebuild and rolls back, so the index is left as it was. The daemon check and read heal use the count check only (about 2 s on the 1.09 M-object copy), never the hashing rebuild.
Sixth: for a timeline-less open,
ensure_watchedanswers "this repository is not watched" from the workspace folders before the store is opened. A missing store directory is no longer created by that question.Evidence
Tests are seams at the public open path (
ObjectStore::open,FurrowRepository::open_timeline_in). Work is counted in SQLite VM steps through the progress handler (dev-dependency featurehooks), not wall-clock. Red commit first (76ef4c3), fix after.Before (red, 4 of 9 fail; the 5 recovery cases pass both sides):
After: all 9 pass. Recovery cases pinned through
open, each ending with every object readable, row count and checkpoint equal to the pack: rows ahead of a stale checkpoint, pack ahead of a lost catalog commit, torn final record (catalog ahead of pack), checkpoint inside a record, dropped index.Spark, copy of the real 1.1 M-row store (36 GB),
furrow --repo <dir> --json status:More red-then-green tests:
a_settled_store_opens_while_another_process_holds_the_catalog_write_lockanda_watched_folder_opens_while_another_process_holds_the_catalog_write_lock(open andstatusfinish while another connection holdsBEGIN IMMEDIATE);legacy_file_rows_move_to_their_folder_once_and_later_opens_leave_thempins the migration that is now conditional.portable.rsnow sets the mode it asserts, so it no longer depends on the runner's umask (it failed on unmodified main under fleet).Timings at the final sha, same store copy: watched
status0.08 - 0.09 s, unwatched 0.02 s (main: 3 - 4 s warm). With another process holding the catalog write lock for 25 s, watchedstatusstill answers in 0.13 s.furrow verifyon the 1.09 M-object copy takes 2.05 s and reports the index sound.Red-then-green for this round:
verify_restores_rows_missing_from_the_middle_of_the_verified_prefix,a_read_that_finds_a_row_missing_rescans_the_pack_once, two deep-verify tests,verify_reports_a_sound_store_and_rebuilds_a_damaged_index(CLI),a_quiet_writer_rebuilds_an_index_that_lost_a_row(daemon),status_of_a_watched_folder_creates_nothing.CI commands on Spark at
fd92172:cargo fmt --all -- --checkandcargo clippy --workspace --all-targets --locked -- -D warningsclean;cargo test --workspace --locked --no-fail-fastvia fleet job20261026-052109-002459: everything passes exceptportable.rsw3_linux_exchange_preserves_bytes_paths_and_recorded_modes, which fails the same way on unmodified main under fleet's umask. That is fixed separately in #18 and deliberately not in this PR.Merge Danger
Door: two-way. No catalog schema or on-disk format change; revert restores the old scan. Not a one-way door.
This touches crash recovery. Open trusts the checkpoint when the pack length equals
verified_len, the high water is at or below it, and the last object's row, location and header agree. Rows missing from the middle of the verified prefix are not seen by open; they are found byfurrow verify, by the daemon's idle check, and by the first failed read of a session, each of which rebuilds the index from the pack. Reads keep their own checksum check.find_workspacestill walks the workspace folders (about 3,800 on the real store; inside the 0.02 s unwatched answer, growing linearly). A catalog lookup cannot replace it without copying thedetachedandt-rules or trusting a table that older stores may not fully populate; a positive hit could be verified against the files, but the unwatched answer is the case that matters and it would still need the walk.Blast Radius: every store open (CLI verbs, daemon, cell processes). A bug would show as a needless full re-verify of the pack (slow, safe) or, in the worst case, a stale index accepted until the next verify. Opens that used to wait on the catalog write lock now proceed, so anything that relied on an open serialising behind a writer does not get that for free; writers still serialise among themselves. The daemon's idle check and the read heal can each run a full rescan of the active pack, once per hour and once per session at most.