perf(fullmap): upgrade redb 2→4, open fullmap readers with shared locks - #68
Merged
Conversation
…locks Fullmap is the sole redb user. redb 2.x takes an EXCLUSIVE flock on every open — even pure reads — so only one opener per fullmap file could exist system-wide: the agent supervisor, its code-executor subprocesses, and parallel agent runs serialized on that lock (masked by the Python-side lock-retry backoff), and a killed-mid-build subprocess could strand it. redb ≥ 3 added ReadOnlyDatabase (shared lock); 4.1 additionally speeds up concurrent multi-threaded reads (~15% on upstream benchmarks) and general write performance (~1.5x on upstream write benchmarks), which benefits the build's redb write phase. This change: - Bumps redb 2.6 → 4.1 (deps + dev-deps). - Switches the ENTIRE read path (DB cache, open_cached/open_cached_shard, schema validation, shard fan-out lookups, CURIE/dim hydration, and the post-build cache prime) to ReadOnlyDatabase — shared locks, so any number of processes read concurrently; only a build-fullmap rebuild (writer) briefly blocks readers. - Maps UpgradeRequired/RepairAborted at read-only open to the actionable rebuild hint (a read-only open can neither upgrade the old v2 file format nor repair a crash-damaged file). - Write path keeps the exclusive-lock Database; set_durability now returns Result (redb ≥ 3). The 16-shard concurrent-writer scheme is unchanged — it remains redb's maximum write parallelism (single WriteTransaction per file by design). - Bumps the schema tag to tablassert.fullmap.v5 (layout unchanged) so the rebuild is explicit and a downgraded extension rejects new files loudly. BREAKING: redb ≥ 3 dropped the v2 file format; existing fullmap files must be rebuilt once via 'tablassert build-fullmap' (BABEL downloads stay cached, so the rebuild is cheap).
Follow-up comment/docstring updates for the redb 4 shared-lock read path (no behavior change): - fullmap.py: the lock-retry comment now describes the reader-vs-writer (rebuild) window — readers no longer contend with each other under the shared lock; note in _db_cache_key that read-only opens never touch the mtime, so the mtime-keyed lookup caches are stable across lookups. - agent.py: derive_coverage derivations no longer 'serialize on the fullmap lock across processes' (shared locks run concurrently; only a rebuild blocks them); a killed executor no longer strands an EXCLUSIVE lock (at most a shared one, released on process death) — the execution_timeout rationale is reworded accordingly.
CODE_REVIEWER findings on the redb 4 shared-lock change (behavior was already correct; these fix invariant-bearing comments and harden tests): - DB_CACHE doc: stop claiming a shared lock blocks an external rebuild. Rebuilds remove+recreate the files on fresh inodes (unlink needs no lock), so a stale cross-process handle reads the unlinked old file as a consistent snapshot — state that as the real invariant. - lookup_terms doc: opening the primary once is a cache choice, not a lock constraint (read-only opens coexist now). - set_durability doc: it fails only on PersistentSavepointModified; this crate never uses savepoints, so the Result is unreachable (not 'once the transaction has been used'). - Rename build_fullmap_db_writes_schema_v4_sharded_layout → build_fullmap_db_writes_sharded_layout (it writes/asserts v5 now). - Byte-patch tests: assert the raw DatabaseError variant (UpgradeRequired(2) / RepairAborted) BEFORE the mapped rebuild hint, so they pin which variant fired instead of both passing on the shared match arm.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe fullmap storage layer upgrades redb from 2 to 4, introduces schema v5, and uses shared read-only handles for lookups. Rebuilds use exclusive access, outdated databases return rebuild guidance, and generation-aware caching keeps primary and shard reads consistent. ChangesFullmap redb 4 migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LookupProcess
participant ReadOnlyDatabase
participant FullmapShards
participant RebuildWriter
LookupProcess->>ReadOnlyDatabase: open primary read-only handle
ReadOnlyDatabase->>FullmapShards: open shard handles
LookupProcess->>FullmapShards: validate matching build IDs
RebuildWriter->>ReadOnlyDatabase: acquire exclusive rebuild lock
RebuildWriter->>FullmapShards: write new primary and shard generation
LookupProcess->>ReadOnlyDatabase: retry changed generation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/fullmap.rs (1)
2333-2343: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPin each fullmap lookup to one primary-plus-shards generation.
open_cachedandopen_cached_shardcache handles by path only. Afterbuild_fullmap_dbunlinks and recreates the files, a lookup can retain an old primary while opening new shards. The primary providesMETA, dimensions, and CURIE IDs; the shards provide record IDs. Rebuilds can change these IDs and shard routing, which can return incorrect CURIEs, misses, or missing-ID errors.
rust/src/fullmap.rs:2333-2392,2595-2607: Coordinate the primary and shard handles as one generation. Use an atomic bundle publication or a generation token with device/inode checks before and after opening all files, then retry when the generation changes.rust/src/fullmap.rs:3756-3801: Add a replacement-during-lookup test. Verify that one lookup never mixes generations and that the next lookup opens the replacement.- Update the cache comments and Python mtime-based caches to use the same generation boundary.
- Qualify the concurrent-reader claims in
docs/fullmap.md,src/tablassert/fullmap.py, andsrc/tablassert/agent.pyuntil rebuild and lookup coordination covers the complete primary-plus-shards bundle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/fullmap.rs` around lines 2333 - 2343, Make fullmap lookups generation-consistent: update open_cached and open_cached_shard in rust/src/fullmap.rs:2333-2392,2595-2607 to publish or validate a shared primary-plus-shards generation using device/inode checks before and after opening all files, retrying when it changes; update cache comments at rust/src/fullmap.rs:70-83 and add replacement-during-lookup coverage at rust/src/fullmap.rs:3756-3801. Align Python mtime-based caches at src/tablassert/fullmap.py:25-29 and src/tablassert/agent.py:2316-2319 with that generation boundary, and qualify concurrent-reader claims in docs/fullmap.md:125-127 until complete bundle coordination is guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 8: Update the CHANGELOG entry for rebuilding older fullmap databases to
remove the claim that the rebuild is cheap. Retain the note that cached BABEL
downloads avoid re-downloading inputs, while accurately stating that the rebuild
reprocesses inputs and rewrites every fullmap file.
---
Outside diff comments:
In `@rust/src/fullmap.rs`:
- Around line 2333-2343: Make fullmap lookups generation-consistent: update
open_cached and open_cached_shard in rust/src/fullmap.rs:2333-2392,2595-2607 to
publish or validate a shared primary-plus-shards generation using device/inode
checks before and after opening all files, retrying when it changes; update
cache comments at rust/src/fullmap.rs:70-83 and add replacement-during-lookup
coverage at rust/src/fullmap.rs:3756-3801. Align Python mtime-based caches at
src/tablassert/fullmap.py:25-29 and src/tablassert/agent.py:2316-2319 with that
generation boundary, and qualify concurrent-reader claims in
docs/fullmap.md:125-127 until complete bundle coordination is guaranteed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b8a4315-0c47-41c2-bf37-ec825c2c3e2f
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGELOG.mddocs/fullmap.mdrust/Cargo.tomlrust/examples/count_tables.rsrust/src/fullmap.rsrust/tests/build_golden.rssrc/tablassert/agent.pysrc/tablassert/fullmap.py
The rebuild still reprocesses the input and rewrites every fullmap file; only the BABEL downloads stay cached. CodeRabbit wording fix on PR #68.
A rebuild replaces the fullmap files on fresh inodes (unlink + recreate), but DB_CACHE keyed handles by canonical path only — so a cache hit could serve an old-generation handle next to new-generation ones (in-process: cache_database swapped only the primary entry; cross-process: stale handles read the unlinked files forever). Record (st_dev, st_ino) via MetadataExt for every cached handle (cache_database included) and validate it on every cache hit in open_cached / open_cached_shard: a mismatch evicts the entry and reopens the replacement (the primary re-runs validate_schema). A hit whose path is absent (mid-rebuild window) keeps serving the old snapshot, preserving the cross-process reader behavior; on a miss the generation is stat'ed before the open so a racing rebuild costs at most one extra reopen. open_cached_shards now pins one primary-plus-shards generation: it captures the primary's (dev,ino) before opening shards and re-stats after the last one, retrying the whole bundle up to 5 times before raising PyRuntimeError, so a lookup never mixes files from two builds.
… lookups Deterministic rename-over swaps (no sleeps/races): replacing a cached primary or shard file makes the next open see the new content; a full lookup after a rebuild-at-same-path returns only the new generation (old term gone, new term hydrated against the new primary); an absent path serves the cached old snapshot until the replacement appears.
…oundary Add the missing docstring to the nested get_fullmap in make_tools (the only undocumented changed function behind CodeRabbit's docstring-coverage warning) and qualify the concurrent-reader claims with the generation boundary — lookups pin one primary-plus-shards generation; readers follow a rebuild on the next lookup — in the make_tools docstring, the fullmap lock-comment block, the _db_cache_key Note, and docs/fullmap.md.
…window A rebuild commits the new primary BEFORE the shard files are created, so mid-build the primary path holds the new inode while every shard path is absent. A warm-cache reader could then mix the new primary with old shard snapshots: each shard hit took the absent-path arm while the stable new primary passed the post-stat generation check. - open_cached_path now reports HOW it served: Served::Current vs Served::StaleAbsent, plumbed through open_cached_shard to open_cached_shards; only io::ErrorKind::NotFound serves the stale snapshot — any other metadata error propagates. - open_cached_shards accepts a bundle only when generationally consistent: ALL Current with a stable primary generation, or ALL StaleAbsent (the documented old-snapshot arm); MIXED retries the whole bundle, then raises the "changing generation" exhaustion error. - "changing generation" joins Python's _LOCK_RETRY_TOKENS so _call_with_lock_retry retries the exhaustion; the primary-appears-first invariant is cross-referenced at the up-front unlink and drop(database). - Tests: mixed window exhausts then follows the new generation; all-absent serves the old snapshot; exhaustion message carries the retry token.
Generate one build_id per fullmap build and write it to the primary META plus every shard META. Shard META is safe here because v5 already mandates a rebuild, so no shard files without META exist in the wild. Cache each handle's build_id and require primary/shard build_id equality before serving any bundle, including all-stale cached snapshots. Retry mixed or diverged bundles with a short delay so slow shard creation does not burn all attempts instantly.
Owner
Author
|
All review feedback addressed: Rebuild wording (inline comment) — d464291 applies the proposed CHANGELOG wording: BABEL downloads stay cached, but the command rebuilds the fullmap files. Generation pinning (Major, outside-diff) — implemented as a generation token, in three layers:
Tests cover: cached handles following a replacement, absent-path snapshot serving, mixed-window retry-then-follow, diverged-cache rejection, the reverse unlink-phase shape, and rebuild-at-same-path lookup consistency. Docstring coverage — Known boundary, deliberately out of scope: |
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
1 participant
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.
What
Upgrades the fullmap entity-resolution store's embedded database engine from redb 2.6 → 4.1 and switches the entire fullmap read path from an exclusive-lock
redb::Databaseopen to a shared-lockredb::ReadOnlyDatabaseopen.Why
redb 2.x takes an exclusive
flockon every open — even pure reads — so only one opener per fullmap file could exist system-wide. The agent supervisor, its code-executor subprocesses, and parallelagent runprocesses all serialized on that lock (masked only by the Python-side_call_with_lock_retrybackoff), and a killed-mid-build subprocess could strand it.redb ≥ 3 added
ReadOnlyDatabase(shared lock), so any number of processes read concurrently; only abuild-fullmaprebuild (exclusive writer) briefly blocks readers. redb 4.1 additionally speeds up concurrent multi-threaded reads (~15% on upstream benchmarks) and general write performance (~1.5x on upstream write benchmarks), which benefits the build's redb write phase.Changes
redb2 → 4 (deps + dev-deps);ReadableDatabasetrait import wherebegin_read()moved.DB_CACHE,open_cached/open_cached_shard, schema validation, shard fan-out lookups, CURIE/dim hydration, post-build cache prime) now usesReadOnlyDatabase.DatabaseError::UpgradeRequired(_)/RepairAbortedat read-only open to the actionable rebuild hint.Database;set_durabilityhandled asResult. The 16-shard concurrent-writer scheme is unchanged (still redb's maximum write parallelism).tablassert.fullmap.v5(layout unchanged) to make the rebuild explicit.UpgradeRequired/RepairAborted→ rebuild-hint mapping.redb ≥ 3 dropped the v2 file format, so existing
fullmap.redb(and siblingfullmap.s*.redb) files must be rebuilt once viatablassert build-fullmapafter upgrading (BABEL downloads stay cached, so the rebuild is cheap). Until rebuilt, lookups fail with the actionable error:fullmap DB is outdated or needs repair; rebuild with 'tablassert build-fullmap'.Verification
make check(local gate): ✅ All checks passed!--all-targets -- -D warnings: cleanEnd-to-end proof of the fix: built a small fullmap and ran concurrent
lookup_fullmap_termsfrom two separate processes — both succeeded (this is exactly what fails under redb 2's exclusive lock).Code review:
CODE_REVIEWERsubagent, 2 rounds. Round 1 returned 2 should-fixes (incorrect lock-semantics comments) + 3 nits; all fixed inb175c55. Round 2: approve, zero findings.Summary by CodeRabbit
Improvements
Breaking Changes