chore: #1780 PROMOTE FORK: fork to new primary via the restore/PITR path#1920
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesPROMOTE FORK command
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SQLParser
participant Runtime
participant OperationalManifest
participant FileSystem
Client->>SQLParser: PROMOTE FORK name
SQLParser->>SQLParser: parse_promote_fork_command
SQLParser-->>Runtime: QueryExpr::PromoteFork
Runtime->>Runtime: execute_promote_fork (check DDL write access, flush state)
Runtime->>OperationalManifest: promote_fork(name)
OperationalManifest->>FileSystem: rename fork to staging
OperationalManifest->>FileSystem: rename primary to archived_parent
OperationalManifest->>FileSystem: rename staging to primary
OperationalManifest-->>Runtime: PromoteForkOutcome
Runtime-->>Client: success message with fork LSN and archived parent
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/reddb-file/src/operational_manifest.rs (1)
700-763: 🧹 Nitpick | 🔵 TrivialArchived parent stores are never garbage collected.
Each successful promotion leaves a
.retired-by-promote-{name}sibling directory holding a full copy of the superseded primary's data, with no visible cleanup path. Over repeated promotions this accumulates unbounded disk usage. Consider documenting a retention/GC policy (or exposing an explicitDROP ARCHIVED <name>-style command) for these retired roots.🤖 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 `@crates/reddb-file/src/operational_manifest.rs` around lines 700 - 763, The promote_fork flow creates a .retired-by-promote-{name} archived parent via archived_parent_handle and never provides any cleanup path, so repeated promotions will accumulate disk usage. Update promote_fork/PromoteForkOutcome handling to define and expose a retention or garbage-collection policy for these retired roots, and add an explicit removal path (for example through a dedicated drop/cleanup operation) so callers can delete the archived parent when it is no longer needed.crates/reddb-server/tests/store_fork_profile.rs (1)
91-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid integration test for the promote path.
Approving. As an optional enhancement, consider also verifying the promoted data is readable via a real SQL
SELECTagainstusers(post-promotion), not just via raw byte comparison — this would more directly validate the "all fork-side writes durable" acceptance criterion from the linked issue, beyond the directory-rename mechanics.🤖 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 `@crates/reddb-server/tests/store_fork_profile.rs` around lines 91 - 130, The promote-fork integration test currently validates durability only by comparing raw collection bytes and checking fork cleanup; add a real post-promotion SQL readback to cover the acceptance criterion more directly. In promote_fork_sql_installs_fork_as_primary_and_archives_parent, after PROMOTE FORK experiment, execute a SELECT against users via RedDBRuntime and assert the fork-side write is returned through normal SQL access, using the existing runtime and manifest setup to confirm the promoted primary is readable end-to-end.
🤖 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 `@crates/reddb-file/src/operational_manifest.rs`:
- Around line 700-763: promote_fork currently leaves the store non-restartable
if a crash happens after renaming self.root to the retired parent but before
moving staging back to self.root. Update promote_fork and the open/recovery path
to detect and resume an in-flight promotion by checking the
promoting_fork_handle and archived_parent_handle siblings, mirroring the resume
behavior used by detach_fork, or introduce a promotion marker/sweep that
restores the staged manifest instead of treating the missing live path as a
fresh store.
In `@crates/reddb-server/src/runtime/impl_ddl.rs`:
- Around line 1439-1466: The PROMOTE FORK flow in execute_promote_fork only
updates the on-disk manifest and leaves the current RedDB instance using stale
open handles and caches. Add a post-promotion reopen/reload step in
execute_promote_fork (or document/enforce a restart-only contract) so the
runtime switches to the promoted store immediately; use the existing flush(),
OperationalManifest::promote_fork, and RuntimeQueryResult::ok_message path to
locate the update point.
---
Nitpick comments:
In `@crates/reddb-file/src/operational_manifest.rs`:
- Around line 700-763: The promote_fork flow creates a
.retired-by-promote-{name} archived parent via archived_parent_handle and never
provides any cleanup path, so repeated promotions will accumulate disk usage.
Update promote_fork/PromoteForkOutcome handling to define and expose a retention
or garbage-collection policy for these retired roots, and add an explicit
removal path (for example through a dedicated drop/cleanup operation) so callers
can delete the archived parent when it is no longer needed.
In `@crates/reddb-server/tests/store_fork_profile.rs`:
- Around line 91-130: The promote-fork integration test currently validates
durability only by comparing raw collection bytes and checking fork cleanup; add
a real post-promotion SQL readback to cover the acceptance criterion more
directly. In promote_fork_sql_installs_fork_as_primary_and_archives_parent,
after PROMOTE FORK experiment, execute a SELECT against users via RedDBRuntime
and assert the fork-side write is returned through normal SQL access, using the
existing runtime and manifest setup to confirm the promoted primary is readable
end-to-end.
🪄 Autofix (Beta)
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: 413e4563-8702-401b-a202-8878f28bc179
📒 Files selected for processing (17)
crates/reddb-file/src/operational_manifest.rscrates/reddb-rql/src/builders.rscrates/reddb-rql/src/core.rscrates/reddb-rql/src/modes/detect.rscrates/reddb-rql/src/planner/optimizer.rscrates/reddb-rql/src/planner/rewriter.rscrates/reddb-rql/src/sql.rscrates/reddb-server/src/runtime.rscrates/reddb-server/src/runtime/impl_core.rscrates/reddb-server/src/runtime/impl_ddl.rscrates/reddb-server/src/runtime/statement_frame.rscrates/reddb-server/src/storage/query/executors/vector.rscrates/reddb-server/src/storage/query/planner/cost.rscrates/reddb-server/src/storage/query/planner/logical.rscrates/reddb-server/src/storage/query/planner/logical_helpers.rscrates/reddb-server/src/storage/query/unified/executor.rscrates/reddb-server/tests/store_fork_profile.rs
| /// Promote a store fork to the primary operational root. | ||
| /// | ||
| /// The promoted fork is first hydrated through the same materialization path | ||
| /// used by restore/fork detach. The superseded primary is then moved to a | ||
| /// deterministic retired root, so its disposition is explicit and cannot be | ||
| /// mistaken for the active store. | ||
| pub fn promote_fork(&self, name: &str) -> io::Result<Option<PromoteForkOutcome>> { | ||
| let fork = self.fork_handle(name); | ||
| if !fork.root.exists() { | ||
| return Ok(None); | ||
| } | ||
| let origin = fork | ||
| .fork_origin()? | ||
| .ok_or_else(|| invalid_data(format!("store fork is missing origin: {name}")))?; | ||
| if origin.parent_store != self.store_identity() { | ||
| return Err(invalid_data(format!( | ||
| "store fork {name} belongs to {}, not {}", | ||
| origin.parent_store, | ||
| self.store_identity() | ||
| ))); | ||
| } | ||
|
|
||
| let staging = self.promoting_fork_handle(name); | ||
| if staging.root.exists() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::AlreadyExists, | ||
| format!("store fork promotion staging path already exists: {name}"), | ||
| )); | ||
| } | ||
| let archived_parent = self.archived_parent_handle(name); | ||
| if archived_parent.root.exists() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::AlreadyExists, | ||
| format!("retired parent store already exists for promoted fork: {name}"), | ||
| )); | ||
| } | ||
|
|
||
| fork.hydrate_shared_collections()?; | ||
| if let Some(parent) = staging.root.parent() { | ||
| fs::create_dir_all(parent)?; | ||
| } | ||
| fs::rename(&fork.root, &staging.root)?; | ||
| sync_dir(&self.forks_dir())?; | ||
| if let Some(parent) = staging.root.parent() { | ||
| sync_dir(parent)?; | ||
| } | ||
|
|
||
| fs::rename(&self.root, &archived_parent.root)?; | ||
| if let Some(parent) = self.root.parent() { | ||
| sync_dir(parent)?; | ||
| } | ||
| fs::rename(&staging.root, &self.root)?; | ||
| if let Some(parent) = self.root.parent() { | ||
| sync_dir(parent)?; | ||
| } | ||
| self.clear_fork_origin()?; | ||
|
|
||
| Ok(Some(PromoteForkOutcome { | ||
| name: origin.name, | ||
| fork_lsn: origin.fork_lsn, | ||
| archived_parent, | ||
| })) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Rust std::fs::rename atomicity crash safety multiple renames durability
💡 Result:
In Rust, std::fs::rename provides an atomic operation to rename or replace a file [1][2]. However, "atomicity" and "durability" are distinct concepts in crash-consistent systems [3]. While std::fs::rename is generally atomic at the filesystem level, ensuring your data survives a system crash requires additional steps [3][4]. Atomicity On Unix-like platforms, rename(2) is defined by POSIX to be atomic [5][6]. If the destination path exists, it is replaced atomically, ensuring that other processes will never see the path in an incomplete state [5]. On Windows, Rust's implementation uses modern APIs (FileRenameInfoEx with POSIX semantics where available) to mimic this behavior [7]. Recent improvements in Rust have worked to resolve historical inconsistencies on Windows, such as access-denied errors during concurrent operations [8][7][9]. Durability and Crash Safety Atomicity does not guarantee that your data is written to stable storage; it only ensures the name switch is atomic [3][4]. To achieve a crash-consistent "atomic save" (where the file is replaced by new, fully-persisted content), you must follow this pattern [3][4]: 1. Write your data to a temporary file located on the same filesystem as the destination [3][4]. 2. Call sync_all (which invokes fsync) on the temporary file to ensure the data contents are persisted [3][4]. 3. Call std::fs::rename to move the temporary file to the final destination [3][4]. 4. Open the parent directory and call sync_all on it [3]. This is the critical, often-missed step that ensures the directory entry update (the rename itself) is persisted to stable storage [10][3][6]. Failure to sync the parent directory may result in the rename being reverted or lost after a power loss or kernel crash, even if the file content itself was successfully synced [10][3][6]. Note that "durability" is ultimately limited by the filesystem and hardware; on some systems, even these steps may not prevent data loss under extreme conditions, though this pattern is the standard for POSIX-compliant crash consistency [3][6].
Citations:
- 1: https://doc.rust-lang.org/stable/std/fs/fn.rename.html
- 2: https://github.com/rust-lang/rust/blob/7fc048f0712ba515ca11fac204921b9ad8a0c5a3/library/std/src/fs.rs
- 3: https://0xkiire.com/crash-consistency-fsync-rename/
- 4: https://users.rust-lang.org/t/how-to-write-replace-files-atomically/42821
- 5: https://man7.org/linux/man-pages/man2/rename.2.html
- 6: https://stackoverflow.com/questions/3764822/how-to-durably-rename-a-file-in-posix
- 7: Win: Use POSIX rename semantics for
std::fs::renameif available rust-lang/rust#131072 - 8:
std::fs::renamesometimes fails on Windows due to missingFILE_RENAME_POSIX_SEMANTICSrust-lang/rust#123985 - 9: Win: Fix std::fs::rename failing on Windows Server by attempting the non-atomic rename first rust-lang/rust#138133
- 10: https://docs.rs/durability/latest/durability/storage/fn.sync_parent_dir.html
Make promote_fork restartable across the rename sequence
A crash between self.root -> *.retired-by-promote-* and *.promoting-* -> self.root leaves no manifest at the live path, and the normal open path can treat that as a fresh store instead of resuming the in-flight promotion. Mirror detach_fork’s resume logic here or add a promotion marker/sweep path so staged and archived siblings are recovered instead of orphaned.
🤖 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 `@crates/reddb-file/src/operational_manifest.rs` around lines 700 - 763,
promote_fork currently leaves the store non-restartable if a crash happens after
renaming self.root to the retired parent but before moving staging back to
self.root. Update promote_fork and the open/recovery path to detect and resume
an in-flight promotion by checking the promoting_fork_handle and
archived_parent_handle siblings, mirroring the resume behavior used by
detach_fork, or introduce a promotion marker/sweep that restores the staged
manifest instead of treating the missing live path as a fresh store.
| pub fn execute_promote_fork( | ||
| &self, | ||
| raw_query: &str, | ||
| query: &PromoteForkQuery, | ||
| ) -> RedDBResult<RuntimeQueryResult> { | ||
| self.check_write(crate::runtime::write_gate::WriteKind::Ddl)?; | ||
| let path = self.inner.db.path().ok_or_else(|| { | ||
| RedDBError::Query("PROMOTE FORK requires a persistent store".to_string()) | ||
| })?; | ||
| self.flush()?; | ||
| let manifest = reddb_file::OperationalManifest::for_db_path(path); | ||
| let outcome = manifest | ||
| .promote_fork(&query.name) | ||
| .map_err(|err| RedDBError::Query(format!("failed to promote store fork: {err}")))?; | ||
| let outcome = | ||
| outcome.ok_or_else(|| RedDBError::NotFound(format!("store fork '{}'", query.name)))?; | ||
| Ok(RuntimeQueryResult::ok_message( | ||
| raw_query.to_string(), | ||
| &format!( | ||
| "store fork '{}' promoted at LSN {}; retired parent archived at {}", | ||
| outcome.name, | ||
| outcome.fork_lsn, | ||
| outcome.archived_parent.store_identity() | ||
| ), | ||
| "promote_fork", | ||
| )) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any reopen/reload of the store tied to fork/restore/PITR paths,
# and confirm whether restart is documented as required.
rg -n -i --type=rust -C3 '\breopen\b|\breload\b|restart' crates/reddb-server/src crates/reddb-file/src | rg -i 'fork|restore|pitr|promote'
rg -n --type=rust -C5 'fn flush\(' crates/reddb-server/src
rg -n --type=rust -B2 -A15 'fn recover_or_bootstrap' crates/reddb-file/src/operational_manifest.rsRepository: reddb-io/reddb
Length of output: 12165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline crates/reddb-server/src/runtime/impl_ddl.rs --view expanded
ast-grep outline crates/reddb-file/src/operational_manifest.rs --view expanded
# Read the specific sections around promote_fork and nearby mutating DDL paths.
sed -n '1400,1495p' crates/reddb-server/src/runtime/impl_ddl.rs
printf '\n---\n'
sed -n '620,760p' crates/reddb-file/src/operational_manifest.rs
printf '\n---\n'
sed -n '1,260p' crates/reddb-server/src/runtime/impl_lifecycle.rs
# Find any reopen/reload/invalidate behavior tied to DDL/catalog mutation.
rg -n -C3 'invalidate_result_cache|invalidate_plan_cache|reopen|reload|open\(|flush\(\)\?|promote_fork|restore|restart' crates/reddb-server/src/runtime crates/reddb-file/srcRepository: reddb-io/reddb
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the promotion implementation, its tests, and any explicit docs about restart/reopen.
sed -n '706,850p' crates/reddb-file/src/operational_manifest.rs
printf '\n---\n'
sed -n '1980,2035p' crates/reddb-file/src/operational_manifest.rs
printf '\n---\n'
sed -n '1439,1472p' crates/reddb-server/src/runtime/impl_ddl.rs
printf '\n---\n'
rg -n -C2 'PROMOTE FORK|promote_fork|restart|reopen|reload|invalidate_result_cache|invalidate_plan_cache' crates/reddb-file/src crates/reddb-server/src | head -n 200
printf '\n---\n'
rg -n -C2 'store_identity|db\.path\(|open_with_options|open\(' crates/reddb-server/src crates/reddb-file/src | head -n 200Repository: reddb-io/reddb
Length of output: 39478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the store's open/path semantics and how the runtime wraps it.
rg -n -C3 'pub fn path\(|struct RedDB|impl RedDB|open_with_options|open\(' crates/reddb-server/src/storage crates/reddb-server/src | head -n 200
printf '\n---\n'
ast-grep outline crates/reddb-server/src/storage/unified/store.rs --view expanded
printf '\n---\n'
sed -n '1,260p' crates/reddb-server/src/storage/unified/store.rs
printf '\n---\n'
sed -n '260,520p' crates/reddb-server/src/storage/unified/store.rsRepository: reddb-io/reddb
Length of output: 34563
PROMOTE FORK needs a reopen step or an explicit restart-only contract. This path only swaps directories on disk; the current RedDB instance keeps its open handles and caches, so it can keep serving the pre-promotion store until the process is recreated.
🤖 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 `@crates/reddb-server/src/runtime/impl_ddl.rs` around lines 1439 - 1466, The
PROMOTE FORK flow in execute_promote_fork only updates the on-disk manifest and
leaves the current RedDB instance using stale open handles and caches. Add a
post-promotion reopen/reload step in execute_promote_fork (or document/enforce a
restart-only contract) so the runtime switches to the promoted store
immediately; use the existing flush(), OperationalManifest::promote_fork, and
RuntimeQueryResult::ok_message path to locate the update point.
…k.rs to satisfy 2000-line file-contract guard Claude-Session: https://claude.ai/code/session_0156URehfHzsvrbeAwzGdbCy
filipeforattini
deleted the
afk/wSZK6/1780-promote-fork-fork-to-new-primary-via-the
branch
1 participant
Automated AFK landing for #1780. Per-attempt history lives in the issue Envelopes, the JSONL logs, and the
afk-attempts/*snapshot branches.Closes #1780
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
PROMOTE FORK <name>to promote a fork into the primary store.Bug Fixes