fix(storage): recover from full disks with bounded memory fallback - #106
fix(storage): recover from full disks with bounded memory fallback#106danielkov wants to merge 2 commits into
Conversation
| // prefix, but its crash durability cannot be promised here. | ||
| self.memory = Some(crate::storage::MemoryBuffer::default()); | ||
| let _ = io::stderr().write_all( | ||
| b"kit: session storage is full; subsequent transcript changes are memory-only and will be lost when the session closes.\n", |
There was a problem hiding this comment.
[P2] Show the memory-only warning outside the hidden log pane
When an active TUI session encounters a storage-capacity error, this warning is written only to the child process's stderr. The TUI pipes that stream and converts unstructured lines into Update::Log (src/tui/mod.rs:760–789), which only appends to app.logs. Because show_logs defaults to false and the log pane is rendered only when explicitly enabled, the normal session view gives no indication that persistence has stopped. Users can therefore continue working and close or switch sessions without knowing that the subsequent transcript changes will be lost. Emit a structured, session-associated degradation event and display a visible warning or persistent memory-only status independently of the optional log pane, retaining stderr reporting for non-TUI callers.
For agents:
Validate the following issue, address if needed:
<comment>The transition to memory-only transcript storage emits only an stderr warning that the TUI routes to its default-hidden log pane, leaving users unaware that subsequent changes will be lost when the session closes.</comment>
<file_context>
diff --git a/src/session.rs b/src/session.rs
--- a/src/session.rs
+++ b/src/session.rs
+ // Sync itself may be failing: truncation restores the logical
+ // prefix, but its crash durability cannot be promised here.
+ self.memory = Some(crate::storage::MemoryBuffer::default());
+ let _ = io::stderr().write_all(
+ b"kit: session storage is full; subsequent transcript changes are memory-only and will be lost when the session closes.\n",
+ );
</file_context>
4ff715b to
9867ad7
Compare
There was a problem hiding this comment.
The writeback filesystem can silently discard writes from independently opened handles, bypass restrictive creation permissions, and exceed its operation budget through handle chmod. Normal TUI shutdown also kills the storage-owning child without final recovery. Symlinked MCP configurations regress, and queued migration synchronization prevents retained-session reopening during an outage. The prior finding about storage-degradation warnings remaining hidden in the TUI log pane is still valid.
| "native identity unavailable", | ||
| )); | ||
| } | ||
| Ok(_) => object.path = None, |
There was a problem hiding this comment.
[P1] Preserve other live handles across service-owned writes
Independently open two append handles A and B to an existing file on a healthy disk, then append through A followed by B. Clean opens create separate Objects, and A's write publishes a replacement inode. rebase updates only dirty objects, so B still holds the old inode. B's next write reaches this identity-mismatch branch, clears its path, and takes the pathless mutation branch, which returns success without publishing the bytes or creating a recovery obligation. Dropping B then silently discards its accepted append.
Preserve shared logical identity across independently opened handles, or update all matching live objects when an ordinary handle mutation publishes its replacement inode. Keep explicit unlink/replacement and external inode replacement distinct from these service-owned writes.
For agents:
Validate the following issue, address if needed:
<comment>A successful write through one independently opened handle causes another handle's subsequent writes to be silently detached from the named file.</comment>
<file_context>
+ Ok(_) => object.path = None,
+ Err(e) if e.kind() == io::ErrorKind::NotFound => object.path = None,
+ Err(e) => return Err(e),
</file_context>
| #[cfg(unix)] | ||
| { | ||
| let _ = (s, path); | ||
| Ok(permissions(private, dir)) |
There was a problem hiding this comment.
[P1] Honor the process umask when creating files
On Unix, ordinary new files receive fixed 0644 permissions from this function. Replay creates a private temporary file but then applies these permissions with file.set_permissions(...) before publishing it. That chmod bypasses the process umask: with umask 077, a newly created file becomes 0644 rather than 0600, exposing its contents to other users when its parent directory is traversable. This affects healthy-disk creation through write, replace, and creating OpenOptions, not just outage recovery.
Determine and retain creation permissions that respect the process umask, then preserve them during replay. Keep existing-file permission preservation separate, and avoid temporarily changing the process-global umask in concurrent code.
For agents:
Validate the following issue, address if needed:
<comment>Fixed creation permissions are later applied by chmod, making newly created files readable beyond the caller's restrictive umask.</comment>
<file_context>
+ #[cfg(unix)]
+ {
+ let _ = (s, path);
+ Ok(permissions(private, dir))
+ }
</file_context>
| let result = run().await; | ||
| // Finish synchronously: a detached task can be terminated with the process, | ||
| // and timing out spawn_blocking would still make runtime teardown wait. | ||
| let recovery = kit::resilient_fs::finish_recovery(kit::resilient_fs::global()); |
There was a problem hiding this comment.
[P1] Finalize the TUI child's writeback queue before killing it
Normal TUI exit does not run this final recovery in the process that owns transcript and artifact writes. After acknowledging CloseSession, the TUI signals its watcher, which calls child.kill().await (src/tui/mod.rs:823–826,1663–1664). ACP session closure drops the binding but deliberately leaves pending writes in the process-wide filesystem. Killing that child bypasses its main finalizer; the parent's finalizer can inspect only the parent's queue.
Consequently, accepted memory-only changes are discarded without a final recovery attempt or an undurable-storage error reaching the TUI caller. This can lose recoverable changes even when disk capacity has returned before the next background retry. Request graceful child-process shutdown, including its A2A listener, await final recovery and the exit status, and propagate persistence failure. Reserve forced termination for a bounded fallback that reports potential data loss.
For agents:
Validate the following issue, address if needed:
<comment>Normal TUI exit forcibly kills the storage-owning child, bypassing its final recovery and hiding the loss of pending writes from the parent.</comment>
<file_context>
+ kit::resilient_fs::start_recovery_worker();
+ let result = run().await;
+ // Finish synchronously: a detached task can be terminated with the process,
+ // and timing out spawn_blocking would still make runtime teardown wait.
+ let recovery = kit::resilient_fs::finish_recovery(kit::resilient_fs::global());
+ // Always attempt recovery, but preserve the original command error. The
+ // recovery helper separately warns if accepted data remains undurable.
+ result?;
+ recovery?;
+ Ok(())
</file_context>
| self.fs.authority(&path)?; | ||
| self.fs.secure_path(&s, &path, false)?; | ||
| Fs::prepare(&mut s, 0)?; | ||
| self.fs.enqueue( |
There was a problem hiding this comment.
[P2] Apply operation-budget admission to handle chmod
File::set_permissions calls enqueue directly after prepare, but prepare only reserves allocator capacity; it does not enforce the configured retention limits. When a capacity-blocked operation remains at the queue front, each handle chmod is appended and returns success, including after max_operations has been reached. Repeated calls can therefore grow the queue beyond its bound without setting the budget-exhaustion flag that triggers cancellation.
Use the checked admission path provided by submit, as the path-based permission operation does, or enforce admission centrally before enqueueing. A rejected chmod must leave both the queue and visible metadata unchanged.
For agents:
Validate the following issue, address if needed:
<comment>Handle chmod bypasses configured operation-budget checks and can grow a capacity-blocked queue without triggering exhaustion shutdown.</comment>
<file_context>
+ Fs::prepare(&mut s, 0)?;
+ self.fs.enqueue(
+ &mut s,
+ Action::Chmod {
+ path,
+ permissions: p.clone(),
+ },
+ )
</file_context>
| async fn read_source(source: &ConfigSource) -> Result<Option<Vec<u8>>, String> { | ||
| match tokio::fs::read(&source.path).await { | ||
| let path = source.path.clone(); | ||
| match tokio::task::spawn_blocking(move || crate::resilient_fs::read(path)) |
There was a problem hiding this comment.
[P2] Preserve symlinked MCP configuration reads
This changes previously working symlinked .mcp.json files and --mcp-config paths into configuration-load failures. Runtime path resolution makes relative paths absolute without resolving the final symlink. resilient_fs::read then normalizes the parent and calls secure_path(..., false), which rejects the final symlink with PermissionDenied; the previous tokio::fs::read followed it. The optional-project-file exception only suppresses NotFound, so it does not preserve this case either.
Preserve symlink-following semantics for user-supplied configuration reads by resolving the target before reading through the coherent filesystem, while retaining no-follow enforcement for security-sensitive managed paths.
For agents:
Validate the following issue, address if needed:
<comment>Switching MCP configuration reads to the no-follow managed filesystem rejects previously supported symlinked configuration files.</comment>
<file_context>
- match tokio::fs::read(&source.path).await {
+ let path = source.path.clone();
+ match tokio::task::spawn_blocking(move || crate::resilient_fs::read(path))
+ .await
+ .map_err(|error| format!("MCP config read task failed: {error}"))?
+ {
</file_context>
| ) -> io::Result<Lease> { | ||
| let path = self.norm(path.as_ref())?; | ||
| let scope = self.norm(scope.as_ref())?; | ||
| self.require_disk(&path)?; |
There was a problem hiding this comment.
[P3] Reuse retained authority before requiring lock-path durability
A legacy-session migration can retain accepted writes during a capacity outage but then fail to reopen after its observer closes. Creating the scoped migration record queues sync_parent_directory behind the blocked write (src/session.rs:1707–1708). Action::Sync::touches treats that parent-directory synchronization as touching the descendant lock path, so this require_disk call returns the unresolved capacity error before the retained-lease registry is consulted. The real lease is still retained by pending work, but its validated owner handoff is unreachable until recovery succeeds.
Check for a valid retained-authority handoff before imposing the native-materialization barrier required for fresh lease acquisition. Preserve the identity, scope, and live-observer checks during that handoff.
For agents:
Validate the following issue, address if needed:
<comment>A queued migration directory sync blocks lock-path durability before retained lease authority can be handed to a reopened session.</comment>
<file_context>
+ let path = self.norm(path.as_ref())?;
+ let scope = self.norm(scope.as_ref())?;
+ self.require_disk(&path)?;
+ let mut state = lock(&self.service.state);
+ state
+ .leases
+ .retain(|(_, authority, _)| authority.strong_count() > 0);
</file_context>
Summary
Route Kit-owned filesystem reads and writes through a shared, bounded write-back service so disk-full and quota failures do not crash active sessions. Retain accepted changes across session close/reopen, recover them to disk automatically, and explicitly shut down when retention is exhausted.
Closes #100.
Impact
Pending state is process-local and does not survive termination. The default retention limits are 64 MiB and 4,096 queued operations. A final recovery pass runs before normal exit; Kit warns and exits unsuccessfully if accepted changes remain unpersisted.
Explicit
editand shell operations still report real filesystem failures. Native consumers require disk-materialized inputs, and session and credential-refresh locks retain real OS authority rather than process-local substitutes.Technical details
Shared reads and ordered recovery
Session histories, credentials, configuration, plugin caches, diagnostics, and output artifacts use one coherent view of disk contents and pending changes. Ordered replay retains ownership checks and atomic replacement state across failures; idle retries and subsequent internal operations recover pending work when capacity returns. Non-capacity, identity, and integrity errors remain failures rather than enabling unsafe fallback.
Bounded retention and output access
Retained payloads use fallible allocation and zeroizing storage. Budget exhaustion requests orderly cancellation; an actual fallback allocation failure takes a separate emergency exit path with a static diagnostic and best-effort terminal restoration. The
artifacttool provides bounded, session-scoped reads of spilled output, including memory-only artifacts that a shell cannot access.