feat(checkpoint): durable cross-process checkpoint persistence - #218
feat(checkpoint): durable cross-process checkpoint persistence#218ishan-parihar wants to merge 2 commits into
Conversation
Checkpoints were in-memory only (a bridge crash dropped all of them). Now every mutation mirrors to disk under <storage>/<session>/<name>/ (manifest.json + blob-N.bin) and CheckpointStore hydrates on startup: - create() persists after the in-memory insert (atomic manifest write) - delete() removes the on-disk dir - cleanup() prunes aged-out persisted checkpoints - CheckpointFile stores Permissions instead of fs::Metadata so permissions can be re-applied after hydration (Metadata is not reconstructible) - new durability test: create in store A, list+restore in store B over the same storage root Protocol-verified: list_checkpoints and restore_checkpoint both work across fresh bridge subprocesses.
- Store the raw session id in the manifest; hydrate by it instead of the sanitized+hashed on-dir dir name (which broke session scoping) - Sanitize+hash both dir components so names that sanitize identically (a/b vs a_b) never collide on disk or silently overwrite on hydration - cleanup() now compares on-disk dirs against the computed alive set (handles hash-suffixed dir names); prunes empty session dirs - Drop orphaned blobs on overwrite; drop dead readonly manifest field - New tests: sanitize traversal/collision, delete mirrors to disk
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Err(e) = self.persist_checkpoint(session, name) { | ||
| crate::slog_warn!( | ||
| "checkpoint {}: persisted in memory but failed to write to disk: {}", | ||
| name, | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
When a checkpoint blob or manifest write fails, create_impl keeps the checkpoint only in memory and still returns success, causing the checkpoint to disappear when the bridge restarts.
Knowledge Base Used: AFT state, storage, and diagnostic artifacts
| if let Ok(entries) = fs::read_dir(&dir) { | ||
| for entry in entries.flatten() { | ||
| let file_name = entry.file_name().to_string_lossy().to_string(); | ||
| if file_name.starts_with("blob-") && file_name.ends_with(".bin") { | ||
| let _ = fs::remove_file(entry.path()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When an existing checkpoint is overwritten and persistence stops before the new manifest is renamed, this removes or replaces the published blobs while the old manifest remains, causing restart hydration to drop the checkpoint or restore new bytes using old file paths.
Knowledge Base Used: AFT state, storage, and diagnostic artifacts
| if removed { | ||
| if let Err(e) = fs::remove_dir_all(&dir) { | ||
| crate::slog_warn!( | ||
| "checkpoint {}: removed from memory but failed to delete on disk: {}", | ||
| name, | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
When remove_dir_all fails, delete has already removed the in-memory entry and still reports success, so the durable directory remains and the deleted checkpoint is hydrated, listed, and restorable after restart.
Knowledge Base Used: AFT state, storage, and diagnostic artifacts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/aft/src/checkpoint.rs">
<violation number="1" location="crates/aft/src/checkpoint.rs:351">
P1: When checkpoint storage is unavailable or out of space, `create` reports success even though the checkpoint will disappear after a bridge restart. Return the persistence error and keep the memory/disk state consistent, or explicitly mark the result non-durable.</violation>
<violation number="2" location="crates/aft/src/checkpoint.rs:453">
P2: When `fs::remove_dir_all(&dir)` fails here, delete() has already removed the in-memory entry and still returns `true` (success) to the caller. The durable directory on disk is left behind, so hydrate_from_disk resurrects the "deleted" checkpoint after the next restart, making it listable and restorable again despite the delete having reportedly succeeded. Consider returning an error (or re-inserting the in-memory entry) when the on-disk removal fails so the caller and durable state stay consistent.</violation>
<violation number="3" location="crates/aft/src/checkpoint.rs:453">
P2: `delete` now performs destructive on-disk I/O (`fs::remove_dir_all`) without acquiring the mutation lock, unlike `create` which calls `persist_checkpoint` under `acquire_mutation_lock` (the persist path even documents "We are under the mutation lock, so this is safe"). Because the whole point of this PR is cross-process durability, two processes sharing a storage root can now race: process A's locked `persist_checkpoint` writes a manifest/dir while process B's unlocked `delete` removes that same dir, or B removes the dir that A just finalized. Acquire the mutation lock (like `create`/`restore`) before the filesystem removal so deletion is serialized with other processes' persists.</violation>
<violation number="4" location="crates/aft/src/checkpoint.rs:521">
P3: In the cleanup disk mirror, `session_empty` is set to false as soon as any checkpoint subdirectory inside the session is visited, even when that subdirectory is removed because it aged out. As a result a session whose only checkpoint(s) were deleted still has its session directory left behind on disk. Hydration tolerates the empty dir (it iterates to nothing), so the impact is only stale on-disk clutter, but it contradicts the mirroring intent that delete/cleanup keep disk consistent with memory. Set `session_empty = false` only for checkpoint dirs that are actually kept (i.e. present in `alive_dirs`) and re-check emptiness after removals.</violation>
<violation number="5" location="crates/aft/src/checkpoint.rs:522">
P1: A long-lived bridge can delete checkpoints created by another process after startup because those directories are absent from its stale `alive_dirs` set. Refresh state under the mutation lock, or remove only on-disk entries independently proven to be expired.</violation>
<violation number="6" location="crates/aft/src/checkpoint.rs:571">
P2: `persist_checkpoint` deletes every existing `blob-*.bin` before writing the new blobs and renaming the manifest, but the manifest itself is only finalized at the very end. If the persist fails partway (disk full, IO error, or a crash during the cross-process window this PR targets) after the old blobs are removed but before `manifest.json` is replaced, the still-on-disk previous manifest now references blob files that no longer exist. `hydrate_from_disk` treats any blob-missing entry as `!ok` and drops the whole checkpoint, so a failed overwrite destroys the previously good durable snapshot instead of leaving it intact. Write the new blobs to fresh names (or delete the old blobs only after the manifest rename succeeds) so a failed persist leaves the prior snapshot restorable.</violation>
<violation number="7" location="crates/aft/src/checkpoint.rs:765">
P2: On Windows, checkpoint names containing invalid filename characters are not durable: directory creation fails, and `create` only logs that failure. Replace all characters outside a platform-safe allowlist, not just separators and controls.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .or_default() | ||
| .insert(name.to_string(), checkpoint); | ||
|
|
||
| if let Err(e) = self.persist_checkpoint(session, name) { |
There was a problem hiding this comment.
P1: When checkpoint storage is unavailable or out of space, create reports success even though the checkpoint will disappear after a bridge restart. Return the persistence error and keep the memory/disk state consistent, or explicitly mark the result non-durable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 351:
<comment>When checkpoint storage is unavailable or out of space, `create` reports success even though the checkpoint will disappear after a bridge restart. Return the persistence error and keep the memory/disk state consistent, or explicitly mark the result non-durable.</comment>
<file context>
@@ -289,6 +348,14 @@ impl CheckpointStore {
.or_default()
.insert(name.to_string(), checkpoint);
+ if let Err(e) = self.persist_checkpoint(session, name) {
+ crate::slog_warn!(
+ "checkpoint {}: persisted in memory but failed to write to disk: {}",
</file context>
| continue; | ||
| } | ||
| session_empty = false; | ||
| if !alive_dirs.contains(&cp_path) { |
There was a problem hiding this comment.
P1: A long-lived bridge can delete checkpoints created by another process after startup because those directories are absent from its stale alive_dirs set. Refresh state under the mutation lock, or remove only on-disk entries independently proven to be expired.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 522:
<comment>A long-lived bridge can delete checkpoints created by another process after startup because those directories are absent from its stale `alive_dirs` set. Refresh state under the mutation lock, or remove only on-disk entries independently proven to be expired.</comment>
<file context>
@@ -417,6 +497,238 @@ impl CheckpointStore {
+ continue;
+ }
+ session_empty = false;
+ if !alive_dirs.contains(&cp_path) {
+ let _ = fs::remove_dir_all(&cp_path);
+ }
</file context>
| .map(|session_checkpoints| session_checkpoints.remove(name).is_some()) | ||
| .unwrap_or(false); | ||
| if removed { | ||
| if let Err(e) = fs::remove_dir_all(&dir) { |
There was a problem hiding this comment.
P2: When fs::remove_dir_all(&dir) fails here, delete() has already removed the in-memory entry and still returns true (success) to the caller. The durable directory on disk is left behind, so hydrate_from_disk resurrects the "deleted" checkpoint after the next restart, making it listable and restorable again despite the delete having reportedly succeeded. Consider returning an error (or re-inserting the in-memory entry) when the on-disk removal fails so the caller and durable state stay consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 453:
<comment>When `fs::remove_dir_all(&dir)` fails here, delete() has already removed the in-memory entry and still returns `true` (success) to the caller. The durable directory on disk is left behind, so hydrate_from_disk resurrects the "deleted" checkpoint after the next restart, making it listable and restorable again despite the delete having reportedly succeeded. Consider returning an error (or re-inserting the in-memory entry) when the on-disk removal fails so the caller and durable state stay consistent.</comment>
<file context>
@@ -376,12 +443,25 @@ impl CheckpointStore {
+ .map(|session_checkpoints| session_checkpoints.remove(name).is_some())
+ .unwrap_or(false);
+ if removed {
+ if let Err(e) = fs::remove_dir_all(&dir) {
+ crate::slog_warn!(
+ "checkpoint {}: removed from memory but failed to delete on disk: {}",
</file context>
| } | ||
| } | ||
| c if c.is_ascii_control() => out.push('_'), | ||
| c => out.push(c), |
There was a problem hiding this comment.
P2: On Windows, checkpoint names containing invalid filename characters are not durable: directory creation fails, and create only logs that failure. Replace all characters outside a platform-safe allowlist, not just separators and controls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 765:
<comment>On Windows, checkpoint names containing invalid filename characters are not durable: directory creation fails, and `create` only logs that failure. Replace all characters outside a platform-safe allowlist, not just separators and controls.</comment>
<file context>
@@ -429,6 +741,88 @@ impl CheckpointStore {
+ }
+ }
+ c if c.is_ascii_control() => out.push('_'),
+ c => out.push(c),
+ }
+ }
</file context>
| c => out.push(c), | |
| c if c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.') => out.push(c), | |
| _ => out.push('_'), |
| .map(|session_checkpoints| session_checkpoints.remove(name).is_some()) | ||
| .unwrap_or(false); | ||
| if removed { | ||
| if let Err(e) = fs::remove_dir_all(&dir) { |
There was a problem hiding this comment.
P2: delete now performs destructive on-disk I/O (fs::remove_dir_all) without acquiring the mutation lock, unlike create which calls persist_checkpoint under acquire_mutation_lock (the persist path even documents "We are under the mutation lock, so this is safe"). Because the whole point of this PR is cross-process durability, two processes sharing a storage root can now race: process A's locked persist_checkpoint writes a manifest/dir while process B's unlocked delete removes that same dir, or B removes the dir that A just finalized. Acquire the mutation lock (like create/restore) before the filesystem removal so deletion is serialized with other processes' persists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 453:
<comment>`delete` now performs destructive on-disk I/O (`fs::remove_dir_all`) without acquiring the mutation lock, unlike `create` which calls `persist_checkpoint` under `acquire_mutation_lock` (the persist path even documents "We are under the mutation lock, so this is safe"). Because the whole point of this PR is cross-process durability, two processes sharing a storage root can now race: process A's locked `persist_checkpoint` writes a manifest/dir while process B's unlocked `delete` removes that same dir, or B removes the dir that A just finalized. Acquire the mutation lock (like `create`/`restore`) before the filesystem removal so deletion is serialized with other processes' persists.</comment>
<file context>
@@ -376,12 +443,25 @@ impl CheckpointStore {
+ .map(|session_checkpoints| session_checkpoints.remove(name).is_some())
+ .unwrap_or(false);
+ if removed {
+ if let Err(e) = fs::remove_dir_all(&dir) {
+ crate::slog_warn!(
+ "checkpoint {}: removed from memory but failed to delete on disk: {}",
</file context>
| // Drop orphaned blobs from a previous snapshot with the same name | ||
| // (an overwrite with fewer files would otherwise leave stale blob-N | ||
| // files behind). We are under the mutation lock, so this is safe. | ||
| if let Ok(entries) = fs::read_dir(&dir) { |
There was a problem hiding this comment.
P2: persist_checkpoint deletes every existing blob-*.bin before writing the new blobs and renaming the manifest, but the manifest itself is only finalized at the very end. If the persist fails partway (disk full, IO error, or a crash during the cross-process window this PR targets) after the old blobs are removed but before manifest.json is replaced, the still-on-disk previous manifest now references blob files that no longer exist. hydrate_from_disk treats any blob-missing entry as !ok and drops the whole checkpoint, so a failed overwrite destroys the previously good durable snapshot instead of leaving it intact. Write the new blobs to fresh names (or delete the old blobs only after the manifest rename succeeds) so a failed persist leaves the prior snapshot restorable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 571:
<comment>`persist_checkpoint` deletes every existing `blob-*.bin` before writing the new blobs and renaming the manifest, but the manifest itself is only finalized at the very end. If the persist fails partway (disk full, IO error, or a crash during the cross-process window this PR targets) after the old blobs are removed but before `manifest.json` is replaced, the still-on-disk previous manifest now references blob files that no longer exist. `hydrate_from_disk` treats any blob-missing entry as `!ok` and drops the whole checkpoint, so a failed overwrite destroys the previously good durable snapshot instead of leaving it intact. Write the new blobs to fresh names (or delete the old blobs only after the manifest rename succeeds) so a failed persist leaves the prior snapshot restorable.</comment>
<file context>
@@ -417,6 +497,238 @@ impl CheckpointStore {
+ // Drop orphaned blobs from a previous snapshot with the same name
+ // (an overwrite with fewer files would otherwise leave stale blob-N
+ // files behind). We are under the mutation lock, so this is safe.
+ if let Ok(entries) = fs::read_dir(&dir) {
+ for entry in entries.flatten() {
+ let file_name = entry.file_name().to_string_lossy().to_string();
</file context>
| if !cp_path.is_dir() { | ||
| continue; | ||
| } | ||
| session_empty = false; |
There was a problem hiding this comment.
P3: In the cleanup disk mirror, session_empty is set to false as soon as any checkpoint subdirectory inside the session is visited, even when that subdirectory is removed because it aged out. As a result a session whose only checkpoint(s) were deleted still has its session directory left behind on disk. Hydration tolerates the empty dir (it iterates to nothing), so the impact is only stale on-disk clutter, but it contradicts the mirroring intent that delete/cleanup keep disk consistent with memory. Set session_empty = false only for checkpoint dirs that are actually kept (i.e. present in alive_dirs) and re-check emptiness after removals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/checkpoint.rs, line 521:
<comment>In the cleanup disk mirror, `session_empty` is set to false as soon as any checkpoint subdirectory inside the session is visited, even when that subdirectory is removed because it aged out. As a result a session whose only checkpoint(s) were deleted still has its session directory left behind on disk. Hydration tolerates the empty dir (it iterates to nothing), so the impact is only stale on-disk clutter, but it contradicts the mirroring intent that delete/cleanup keep disk consistent with memory. Set `session_empty = false` only for checkpoint dirs that are actually kept (i.e. present in `alive_dirs`) and re-check emptiness after removals.</comment>
<file context>
@@ -417,6 +497,238 @@ impl CheckpointStore {
+ if !cp_path.is_dir() {
+ continue;
+ }
+ session_empty = false;
+ if !alive_dirs.contains(&cp_path) {
+ let _ = fs::remove_dir_all(&cp_path);
</file context>
ualtinok
left a comment
ualtinok
left a comment
There was a problem hiding this comment.
Thanks for this — the core mechanism is right (persist on create, hydrate on startup, mirror mutations), and several details are better than typical first contributions: atomic manifest writes, fail-closed hydration of corrupt manifests, the stable FNV hash with collision disambiguation, and real traversal tests. Two blockers and two design items before this can merge:
Blocker 1: the Windows branch doesn't compile. std::os::windows::fs::PermissionsExt is an unstable library feature — cargo check --target x86_64-pc-windows-gnu fails with E0658 on both permission_mode and permission_from_mode. (Our Rust CI didn't run on this PR — fork PRs need workflow approval, which is why the bot checks are the only green you saw.) Suggested fix: persist the full mode bits under cfg(unix) only, and use the readonly() bit everywhere else (your existing not(any(unix, windows)) branch is the right shape for Windows too).
Blocker 2: the disk mirror in cleanup_expired deletes other processes' live checkpoints. The sweep removes any on-disk checkpoint dir not present in this process's in-memory map. The storage root is shared per project, and multiple bridge processes serve the same project concurrently (multi-session is our production norm): process A — hydrated before B created its checkpoints — would sweep B's live checkpoints on A's next TTL cleanup. Disk deletion needs to be driven by disk state: read each dir's manifest.json, compare its created_at against the TTL under the mutation lock, and remove only expired entries. Memory-diff must never be deletion authority for shared storage.
Design 1: hydration loads every blob into memory at construction. hydrate_from_disk reads all blob bytes for all checkpoints eagerly, and store construction happens on the startup path. A project with a few large checkpoints turns that into real startup cost and resident memory. Suggested: hydrate manifests only, keep the blob path in the map, and read bytes on restore (with the missing-blob case handled as a restore-time error).
Design 2 (note, not a blocker): the storage key inherits the lock path's scoping. CheckpointStore::new keys storage off project_scope_key(current_dir()); under the daemon a single process serves many roots, so all of them share one bucket. That's a pre-existing property of the lock path, but this PR promotes it into the durability layout. Fine to land as-is — we'll thread the real project root through when we touch this next — but worth a comment at the site so the assumption is visible.
Happy to re-review quickly — with blockers 1 and 2 fixed this is a solid contribution we want.
2 participants
Checkpoints were in-memory only (a bridge crash dropped all of them). This makes them durable: persist on create, hydrate on startup, mirror delete/cleanup to disk, store Permissions not Metadata, sanitize+hash dir components. 27 checkpoint tests pass incl. cross-store durability; protocol-verified across 3 subprocesses.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Makes checkpoints durable across process restarts. Previously they were in-memory and lost on crash; now we persist on create, hydrate on startup, and mirror delete/cleanup to disk without changing the external API.
<storage_root>/<session>-<hash>/<name>-<hash>/{manifest.json, blob-N.bin}. The manifest stores the raw session id; hydration uses it (not dir names). Writes are atomic and orphaned blobs are cleaned on overwrite.a/bvsa_b). Hydration skips unreadable/corrupt entries with warnings.CheckpointFilenow storesPermissionsinstead offs::Metadataso permissions survive hydration.Written for commit e6a022b. Summary will update on new commits.
Greptile Summary
This PR adds filesystem-backed checkpoint persistence, startup hydration, durable deletion and cleanup, path-safe checkpoint directories, and serialized permission metadata.
Confidence Score: 2/5
This PR is not safe to merge until checkpoint persistence, overwrite publication, and deletion failures preserve the promised durable state or return errors.
Creation and deletion currently report success after disk failures, while overwrites modify published blobs before the replacement manifest is committed, allowing checkpoints to disappear, reappear, or restore mismatched contents after restart.
Files Needing Attention: crates/aft/src/checkpoint.rs
Important Files Changed
Sequence Diagram
Reviews (1): Last reviewed commit: "fix(checkpoint): review fixes for durabl..." | Re-trigger Greptile
Context used: