mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-16 23:43:10 +00:00
refactor(scratch): remove store-backed diff and blob cache writes
Stop writing scratch final.patch files now that diffs are projected from run state, and remove the unused cache/artifacts/values plumbing while keeping runtime/blobs materialization intact. Update tests and run-directory docs to match the current scratch contract.
This commit is contained in:
parent
45f8d94df6
commit
c1507b57a3
13 changed files with 43 additions and 93 deletions
|
|
@ -15,7 +15,6 @@ There is no `_init.json` anymore. Run existence in the database is determined by
|
|||
| File | Purpose | Source |
|
||||
|---|---|---|
|
||||
| `workflow_bundle.json` | Bundled workflow input used by `start` to restore `workflow_path` and bundled child workflows/files | Written during create from the resolved workflow bundle |
|
||||
| `final.patch` | Final git diff for checkpointed runs | Local git state at finalize time, not a direct event payload |
|
||||
| `run.pid` | Legacy detached-run pid file from older runs | Legacy only; current flows do not rely on it |
|
||||
|
||||
## Local-Only Directories
|
||||
|
|
@ -41,3 +40,4 @@ These names are still real, but they are no longer live scratch files by default
|
|||
## Notes
|
||||
|
||||
- Artifact binaries are no longer stored in the SlateDB keyspace. They live in `ArtifactStore`; the run scratch tree only contains local cached copies when a workflow stage writes them to disk.
|
||||
- Final diffs for checkpointed runs are projected from the run store; they are no longer written as scratch files.
|
||||
|
|
|
|||
|
|
@ -408,7 +408,7 @@ fabro pr create <run-id> --model claude-opus-4-6
|
|||
| `<RUN_ID>` | Run ID or prefix (required) |
|
||||
| `--model <MODEL>` | LLM model for generating the PR description |
|
||||
|
||||
The run must have completed successfully (or with partial success) and have a `final.patch` with changes.
|
||||
The run must have completed successfully (or with partial success) and have a stored diff with changes.
|
||||
|
||||
### `fabro pr list`
|
||||
|
||||
|
|
@ -658,20 +658,17 @@ fabro repo deinit
|
|||
|
||||
## `fabro diff`
|
||||
|
||||
Show the diff from a workflow run. Displays the `final.patch` for completed runs, or connects to the sandbox for a live diff from in-progress runs.
|
||||
Show the diff from a workflow run. Reads the stored diff for completed runs.
|
||||
|
||||
```bash
|
||||
fabro diff <run-id>
|
||||
fabro diff <run-id> --node work
|
||||
fabro diff <run-id> --stat
|
||||
```
|
||||
|
||||
| Argument / Flag | Description |
|
||||
|---|---|
|
||||
| `<RUN>` | Run ID or prefix (required) |
|
||||
| `--node <NODE>` | Show diff for a specific node instead of the full run |
|
||||
| `--stat` | Show diffstat instead of full patch (live diffs only) |
|
||||
| `--shortstat` | Show only files-changed/insertions/deletions summary (live diffs only) |
|
||||
|
||||
Output is colorized when writing to a terminal.
|
||||
|
||||
|
|
|
|||
|
|
@ -20,7 +20,6 @@ The naming format is `YYYYMMDD-{run_id}`, where `run_id` is the ULID assigned to
|
|||
| File | Format | When written | Description |
|
||||
|---|---|---|---|
|
||||
| `workflow_bundle.json` | JSON | Run create | Bundled workflow input used to restart the run without re-reading the original workflow files. Includes the root workflow path plus bundled child workflow sources and inline files. |
|
||||
| `final.patch` | Diff | Run end | Git diff from `base_sha` to final HEAD. Only present in git checkpoint mode. |
|
||||
| `run.pid` | Text | Legacy only | Legacy process ID file from older runs. Current detached launches use launcher records instead, and current attach/resume no longer read `run.pid`. |
|
||||
|
||||
## Local-only directories
|
||||
|
|
@ -32,7 +31,7 @@ These paths are local runtime state and caches, not the canonical run record.
|
|||
- **`cache/artifacts/files/`** — Captured artifact files organized by node and retry, plus a `manifest.json` for each retry directory.
|
||||
- **`nodes/{manager_node}_{visit}/child/`** — Nested scratch directories for manager-loop child workflows.
|
||||
|
||||
Large durable values, event streams, checkpoints, conclusions, and retros are no longer projected into live scratch by default. Use `fabro logs`, `fabro inspect`, the API, or `fabro store dump` for those surfaces.
|
||||
Large durable values, event streams, checkpoints, diffs, conclusions, and retros are no longer projected into live scratch by default. Use `fabro logs`, `fabro inspect`, the API, or `fabro store dump` for those surfaces.
|
||||
|
||||
## Reconstructed and export-only layouts
|
||||
|
||||
|
|
@ -59,7 +58,6 @@ fabro ps --filter workflow=my-workflow
|
|||
├── 20260307-01JQXYZ123ABC456DEF789/ # One directory per run
|
||||
│ ├── workflow_bundle.json
|
||||
│ ├── run.pid # Legacy only; older runs may contain this
|
||||
│ ├── final.patch
|
||||
│ ├── runtime/
|
||||
│ │ └── blobs/
|
||||
│ │ └── 01JT5Y3KJ0N5S9E1Y7YFBR2G4D.json
|
||||
|
|
|
|||
|
|
@ -51,7 +51,7 @@ pub(super) async fn create_command(
|
|||
.final_patch
|
||||
.context("Failed to load final patch from store — no diff available")?;
|
||||
if diff.trim().is_empty() {
|
||||
bail!("final.patch is empty — nothing to create a PR for");
|
||||
bail!("Stored diff is empty — nothing to create a PR for");
|
||||
}
|
||||
|
||||
let cwd = std::env::current_dir().context("Failed to get current directory")?;
|
||||
|
|
|
|||
|
|
@ -64,13 +64,13 @@ fn resolve_diff(state: &RunProjection, args: &DiffArgs) -> Result<String> {
|
|||
.ok_or_else(|| anyhow::anyhow!("This run was not git-checkpointed; no diff available"))?;
|
||||
|
||||
if let Some(patch) = state.final_patch.clone() {
|
||||
debug!("Reading final.patch from store");
|
||||
debug!("Reading stored diff from run state");
|
||||
return Ok(patch);
|
||||
}
|
||||
|
||||
if state.conclusion.is_some() {
|
||||
bail!(
|
||||
"Run completed but no final.patch exists — the run may not have produced any changes"
|
||||
"Run completed but no stored diff exists — the run may not have produced any changes"
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -43,7 +43,7 @@ fn diff_completed_run_without_changes_reports_no_patch() {
|
|||
exit_code: 1
|
||||
----- stdout -----
|
||||
----- stderr -----
|
||||
error: Run completed but no final.patch exists — the run may not have produced any changes
|
||||
error: Run completed but no stored diff exists — the run may not have produced any changes
|
||||
");
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -120,11 +120,6 @@ impl RunScratch {
|
|||
self.root.join("cache").join("artifacts")
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn blob_cache_dir(&self) -> PathBuf {
|
||||
self.artifact_cache_dir().join("values")
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn artifact_files_dir(&self) -> PathBuf {
|
||||
self.artifact_cache_dir().join("files")
|
||||
|
|
@ -137,15 +132,9 @@ impl RunScratch {
|
|||
.join(format!("retry_{attempt}"))
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn final_patch(&self) -> PathBuf {
|
||||
self.root.join("final.patch")
|
||||
}
|
||||
|
||||
pub fn create(&self) -> std::io::Result<()> {
|
||||
std::fs::create_dir_all(self.worktree_dir())?;
|
||||
std::fs::create_dir_all(self.runtime_dir())?;
|
||||
std::fs::create_dir_all(self.blob_cache_dir())?;
|
||||
std::fs::create_dir_all(self.artifact_files_dir())?;
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -230,14 +219,6 @@ mod tests {
|
|||
scratch.artifact_cache_dir(),
|
||||
scratch.root().join("cache").join("artifacts")
|
||||
);
|
||||
assert_eq!(
|
||||
scratch.blob_cache_dir(),
|
||||
scratch
|
||||
.root()
|
||||
.join("cache")
|
||||
.join("artifacts")
|
||||
.join("values")
|
||||
);
|
||||
assert_eq!(
|
||||
scratch.artifact_files_dir(),
|
||||
scratch.root().join("cache").join("artifacts").join("files")
|
||||
|
|
@ -252,14 +233,21 @@ mod tests {
|
|||
.join("plan")
|
||||
.join("retry_2")
|
||||
);
|
||||
assert_eq!(scratch.final_patch(), scratch.root().join("final.patch"));
|
||||
|
||||
scratch.create().unwrap();
|
||||
assert!(scratch.root().exists());
|
||||
assert!(scratch.worktree_dir().exists());
|
||||
assert!(scratch.runtime_dir().exists());
|
||||
assert!(scratch.blob_cache_dir().exists());
|
||||
assert!(scratch.artifact_files_dir().exists());
|
||||
assert!(
|
||||
!scratch
|
||||
.root()
|
||||
.join("cache")
|
||||
.join("artifacts")
|
||||
.join("values")
|
||||
.exists()
|
||||
);
|
||||
assert!(!scratch.root().join("final.patch").exists());
|
||||
|
||||
scratch.remove().unwrap();
|
||||
assert!(!scratch.root().exists());
|
||||
|
|
|
|||
|
|
@ -32,14 +32,11 @@ const ARTIFACT_POINTER_PREFIX: &str = "file://";
|
|||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns an error if blob persistence or cache materialization fails.
|
||||
/// Returns an error if blob persistence fails.
|
||||
pub async fn offload_large_values(
|
||||
updates: &mut HashMap<String, Value>,
|
||||
run_store: &RunStoreHandle,
|
||||
cache_dir: &Path,
|
||||
) -> Result<()> {
|
||||
let _ = cache_dir;
|
||||
|
||||
for value in updates.values_mut() {
|
||||
let bytes = serde_json::to_vec(&*value)
|
||||
.map_err(|e| FabroError::engine(format!("artifact serialize failed: {e}")))?;
|
||||
|
|
@ -381,7 +378,6 @@ mod tests {
|
|||
|
||||
#[tokio::test]
|
||||
async fn offload_replaces_large_values_with_blob_backed_pointer() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let run_store = make_run_store("artifact-offload").await;
|
||||
|
||||
let large_string = "x".repeat(BLOB_OFFLOAD_THRESHOLD + 1);
|
||||
|
|
@ -391,7 +387,7 @@ mod tests {
|
|||
let mut updates = HashMap::new();
|
||||
updates.insert("response.plan".to_string(), serde_json::json!(large_string));
|
||||
|
||||
offload_large_values(&mut updates, &run_store.clone().into(), dir.path())
|
||||
offload_large_values(&mut updates, &run_store.clone().into())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
|
|
@ -408,26 +404,20 @@ mod tests {
|
|||
.expect("blob should exist");
|
||||
let blob_value: serde_json::Value = serde_json::from_slice(&blob).unwrap();
|
||||
assert_eq!(blob_value, serde_json::json!(large_string));
|
||||
assert!(
|
||||
std::fs::read_dir(dir.path()).unwrap().next().is_none(),
|
||||
"offload should not materialize host cache files"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn offload_leaves_small_values_untouched() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let run_store = make_run_store("artifact-small").await;
|
||||
let small_value = serde_json::json!("hello world");
|
||||
let mut updates = HashMap::new();
|
||||
updates.insert("small_key".to_string(), small_value.clone());
|
||||
|
||||
offload_large_values(&mut updates, &run_store.clone().into(), dir.path())
|
||||
offload_large_values(&mut updates, &run_store.clone().into())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(updates.get("small_key").unwrap(), &small_value);
|
||||
assert!(std::fs::read_dir(dir.path()).unwrap().next().is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -37,7 +37,6 @@ const ARTIFACT_UPLOAD_RETRY_DELAYS: [Duration; 3] = [
|
|||
pub(crate) struct ArtifactLifecycle {
|
||||
pub sandbox: Arc<dyn fabro_sandbox::Sandbox>,
|
||||
pub run_store: RunStoreHandle,
|
||||
pub blob_cache_dir: PathBuf,
|
||||
pub emitter: Arc<Emitter>,
|
||||
pub artifacts_dir: PathBuf,
|
||||
pub artifact_globs: Vec<String>,
|
||||
|
|
@ -52,7 +51,6 @@ impl ArtifactLifecycle {
|
|||
pub(crate) fn new(
|
||||
sandbox: Arc<dyn fabro_sandbox::Sandbox>,
|
||||
run_store: RunStoreHandle,
|
||||
blob_cache_dir: PathBuf,
|
||||
emitter: Arc<Emitter>,
|
||||
artifacts_dir: PathBuf,
|
||||
artifact_globs: Vec<String>,
|
||||
|
|
@ -62,7 +60,6 @@ impl ArtifactLifecycle {
|
|||
Self {
|
||||
sandbox,
|
||||
run_store,
|
||||
blob_cache_dir,
|
||||
emitter,
|
||||
artifacts_dir,
|
||||
artifact_globs,
|
||||
|
|
@ -174,12 +171,8 @@ impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
|||
let node_id = node.id();
|
||||
|
||||
// Offload large context_updates values to artifact store
|
||||
if let Err(e) = offload_large_values(
|
||||
&mut result.outcome.context_updates,
|
||||
&self.run_store,
|
||||
&self.blob_cache_dir,
|
||||
)
|
||||
.await
|
||||
if let Err(e) =
|
||||
offload_large_values(&mut result.outcome.context_updates, &self.run_store).await
|
||||
{
|
||||
self.emitter.emit(&Event::RunNotice {
|
||||
level: RunNoticeLevel::Warn,
|
||||
|
|
|
|||
|
|
@ -1,11 +1,8 @@
|
|||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
use async_trait::async_trait;
|
||||
use fabro_config::RunScratch;
|
||||
use fabro_types::RunId;
|
||||
use tokio::fs;
|
||||
|
||||
use fabro_core::error::{CoreError, Result as CoreResult};
|
||||
use fabro_core::graph::NodeSpec;
|
||||
|
|
@ -67,7 +64,6 @@ pub(crate) struct GitCheckpointResult {
|
|||
pub(crate) struct GitLifecycle {
|
||||
pub sandbox: Arc<dyn fabro_sandbox::Sandbox>,
|
||||
pub emitter: Arc<Emitter>,
|
||||
pub run_dir: PathBuf,
|
||||
pub run_id: RunId,
|
||||
pub run_store: RunStoreHandle,
|
||||
pub run_options: Arc<RunOptions>,
|
||||
|
|
@ -301,7 +297,7 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
|||
}
|
||||
|
||||
async fn on_run_end(&self, outcome: &Outcome, _state: &WfRunState) {
|
||||
// Write final.patch on success
|
||||
// Capture the final diff on success for event/store projection.
|
||||
if (outcome.status == StageStatus::Success || outcome.status == StageStatus::PartialSuccess)
|
||||
&& self.run_options.git.is_some()
|
||||
{
|
||||
|
|
@ -314,15 +310,6 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
|||
match git_diff(&*self.sandbox, &base_sha).await {
|
||||
Ok(patch) if !patch.is_empty() => {
|
||||
*self.final_patch.lock().unwrap() = Some(patch.clone());
|
||||
if let Err(err) =
|
||||
fs::write(RunScratch::new(&self.run_dir).final_patch(), patch).await
|
||||
{
|
||||
self.emitter.emit(&Event::RunNotice {
|
||||
level: RunNoticeLevel::Warn,
|
||||
code: "final_patch_write_failed".to_string(),
|
||||
message: format!("failed to write final.patch: {err}"),
|
||||
});
|
||||
}
|
||||
}
|
||||
Ok(_) => {
|
||||
*self.final_patch.lock().unwrap() = None;
|
||||
|
|
|
|||
|
|
@ -153,7 +153,6 @@ impl WorkflowLifecycle {
|
|||
let git = GitLifecycle {
|
||||
sandbox: Arc::clone(sandbox),
|
||||
emitter: Arc::clone(emitter),
|
||||
run_dir: run_dir.clone(),
|
||||
run_id: run_options.run_id,
|
||||
run_store: run_store.clone(),
|
||||
run_options: Arc::clone(run_options),
|
||||
|
|
@ -166,7 +165,6 @@ impl WorkflowLifecycle {
|
|||
let artifact = ArtifactLifecycle::new(
|
||||
Arc::clone(sandbox),
|
||||
run_store.clone(),
|
||||
run_scratch.blob_cache_dir(),
|
||||
Arc::clone(emitter),
|
||||
run_scratch.artifact_files_dir(),
|
||||
run_options.artifact_globs().to_vec(),
|
||||
|
|
|
|||
|
|
@ -728,11 +728,12 @@ async fn daytona_git_checkpoint_remote_emits_events() {
|
|||
"checkpoint should have git_commit_sha"
|
||||
);
|
||||
|
||||
// Assert final.patch exists and contains changes from the run
|
||||
// Assert scratch final.patch is no longer written
|
||||
let final_patch = dir.path().join("final.patch");
|
||||
assert!(final_patch.exists(), "final.patch should exist in run_dir");
|
||||
let patch_content = std::fs::read_to_string(&final_patch).unwrap();
|
||||
assert!(!patch_content.is_empty(), "final.patch should not be empty");
|
||||
assert!(
|
||||
!final_patch.exists(),
|
||||
"final.patch should not be written to scratch"
|
||||
);
|
||||
|
||||
env.cleanup().await.unwrap();
|
||||
}
|
||||
|
|
@ -1242,9 +1243,12 @@ async fn daytona_git_checkpoint_with_shadow_branch() {
|
|||
"sandbox commit should have Fabro-Run trailer, got:\n{commit_msg}"
|
||||
);
|
||||
|
||||
// Assert final.patch exists
|
||||
// Assert scratch final.patch is no longer written
|
||||
let final_patch = dir.path().join("final.patch");
|
||||
assert!(final_patch.exists(), "final.patch should exist in run_dir");
|
||||
assert!(
|
||||
!final_patch.exists(),
|
||||
"final.patch should not be written to scratch"
|
||||
);
|
||||
|
||||
env.cleanup().await.unwrap();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8667,7 +8667,10 @@ async fn large_context_values_are_offloaded_to_artifact_store() {
|
|||
|
||||
assert!(
|
||||
!RunScratch::new(dir.path())
|
||||
.blob_cache_dir()
|
||||
.root()
|
||||
.join("cache")
|
||||
.join("artifacts")
|
||||
.join("values")
|
||||
.join(format!("{expected_blob_id}.json"))
|
||||
.exists(),
|
||||
"legacy host blob cache file should not exist"
|
||||
|
|
@ -10384,13 +10387,11 @@ async fn git_checkpoint_host_emits_events_and_diff_patch() {
|
|||
"checkpoint should have git_commit_sha"
|
||||
);
|
||||
|
||||
// 9. Assert final.patch exists and contains the changes
|
||||
// 9. Assert scratch final.patch is no longer written
|
||||
let final_patch = run_dir.path().join("final.patch");
|
||||
assert!(final_patch.exists(), "final.patch should exist in run_dir");
|
||||
let patch_content = std::fs::read_to_string(&final_patch).unwrap();
|
||||
assert!(
|
||||
patch_content.contains("hello.txt"),
|
||||
"final.patch should contain hello.txt changes"
|
||||
!final_patch.exists(),
|
||||
"final.patch should not be written to scratch"
|
||||
);
|
||||
|
||||
// Cleanup worktree
|
||||
|
|
@ -10827,17 +10828,11 @@ async fn parallel_git_branching_host_e2e() {
|
|||
"parallel branch ref should still exist for debugging"
|
||||
);
|
||||
|
||||
// 11. Verify final.patch contains the winner's changes
|
||||
// 11. Verify scratch final.patch is no longer written
|
||||
let final_patch = run_dir.path().join("final.patch");
|
||||
assert!(final_patch.exists(), "final.patch should exist in run_dir");
|
||||
let patch_content = std::fs::read_to_string(&final_patch).unwrap();
|
||||
assert!(
|
||||
patch_content.contains(&format!("{best_id}.txt")),
|
||||
"final.patch should contain winner's file"
|
||||
);
|
||||
assert!(
|
||||
!patch_content.contains(&format!("{loser_id}.txt")),
|
||||
"final.patch should NOT contain loser's file"
|
||||
!final_patch.exists(),
|
||||
"final.patch should not be written to scratch"
|
||||
);
|
||||
|
||||
// 12. Verify events
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue