diff --git a/docs/internal/forkd-snapshot-truthfulness.md b/docs/internal/forkd-snapshot-truthfulness.md new file mode 100644 index 0000000000..66032de038 --- /dev/null +++ b/docs/internal/forkd-snapshot-truthfulness.md @@ -0,0 +1,347 @@ +# forkd snapshot registry truthfulness (fabro-123) + +**Status**: root-cause note, written BEFORE any fix is attempted. +**Author**: fabro-121 worker (reassigned to #123 after #121 proved the +status-posting pipeline is intact and the real blocker is upstream of it). +**Date**: 2026-08-01. + +This document is the writing-FIRST deliverable required by the brief. It +records what the evidence shows, what is consistent, what is contradictory, +and what the operator must still verify live before any code change to the +controller on dellsrv is merged. + +--- + +## 1. The failure as observed (fresh evidence from #121 work) + +The descriptor-driven gate poller (PR #140, branch +`ao/fabro-121/gate-poll-descriptors`) ran a live cycle at +**2026-08-01T08:32:27Z** and successfully posted a `fabro/qa-pipeline` commit +status to two previously-silent repos: + +| repo | open PR / head | state posted | description | +|--------|-----------------------------------------------|--------------|-------------| +| trader | PR #354 / `3ed3b8a4da3cc4e7f41371f6ab0ace779e8a155c` | `error` | `infrastructure: controller POST /v1/sandboxes -> HTTP 500 {"error":"restore_many: firecracker API PUT /snapshot/load returned 400: {"fault_...` | +| foundry| PR #53 / `b919955d569f639fec627a2d09ae3f2909398876` | `error` | (same string) | + +Source: `~/.ao/state/fabro-gate-poll.log` (poll end 2026-08-01T08:32:43Z) +cross-checked against `git -C ~/.ao/data/worktrees/ao-company-gate121 +status --porcelain` (uncommitted: nothing — branch `258a5f0` is what +actually ran). + +So the **status-posting pipeline is intact**. PR #140 ended the silent-merge +class of failure described in #121. What #121 surfaced instead is the +distinct failure described in #123: every `cmd_gate` call into the forkd +controller returns HTTP 500 on the `restore_many` step, which lives inside +the boot path before any in-VM command runs. + +## 2. What #123 as filed said, and what my #121 work changed about that + +The issue text (zenprocess/ao-company#123) is built on a probe dated +**2026-07-31** that found: + +> the forkd controller successfully booted a sandbox from snapshot tag +> `zen-gate-base` and exec'd with exit 0 — yet `GET /v1/snapshots` returned +> `[]`. The listing endpoint lies about available snapshots while the boot +> path resolves tags fine. + +The 07-31 probe name (`zen-gate-base`) and the failing gate's request name +(`zen-gate-big`) are different tags. The `restore_many` 400 in #121 is the +first hard evidence I have that the gap is not just a stale listing but a +**real boot-path failure for the `zen-gate-big` tag specifically**. Either: + +- the registry lost `zen-gate-big` while still holding `zen-gate-base`; or +- the on-disk snapshot file for `zen-gate-big` is present but + unbootable; or +- the registry has an entry for `zen-gate-big` but its file handle is stale + (firecracker's `PUT /snapshot/load` 400 includes a `fault_` JSON whose + truncated tail I cannot read from outside the controller). + +The brief flags the same uncertainty: "*Establish which BEFORE fixing.*" + +## 3. What I CAN state from the evidence in front of me + +- `GET /v1/snapshots` is currently a **lie** for callers that trust it. The + on-host audit probe (2026-07-31) got `[]` back while `zen-gate-base` + booted fine, so the listing endpoint is decoupled from the boot path. +- The boot path **can fail with HTTP 500 on `restore_many` even when the + requested snapshot tag is one we have observed boot previously** (because + the 07-31 probe booted `zen-gate-base` successfully, and the failure + shown in #121 is on `zen-gate-big`, but the failure mode is the same + class — snapshot registry vs. snapshot file — so the same fix covers + both). +- The forkd-shim and `fabro-sandbox/src/forkd/mod.rs` only consume three + endpoints: `POST /v1/sandboxes`, `POST /v1/sandboxes/{id}/exec`, + `DELETE /v1/sandboxes/{id}`. `GET /v1/snapshots` is not consumed by any + client code in `lib/`; the on-host 07-31 probe was the only live caller + on record. +- There is **no forkd CONTROLLER source tree in the fabro repo**. The repo + contains the client (`lib/crates/fabro-sandbox/src/forkd/mod.rs`), the + provider (`lib/crates/fabro-sandbox/src/provider/forkd.rs`), the e2e + BDD spec (`specs/forkd-e2e/behavior.feature`), and the gate driver + shim (`bin/forkd-shim.py` is staged in `ao-company/bin/`, not in + `fabro/`). The controller is a separate service running on dellsrv; its + source is not local to this checkout. + +## 4. What I CANNOT state from the evidence in front of me + +These are the things the brief is asking for, that I cannot determine +without reaching dellsrv: + +- **Whether the snapshot REGISTRY is in-memory only, file-backed, or + remote-DB-backed.** The hypothesis the issue proposes is "volatile across + restart while files persist on disk", but the controller's actual storage + choice is not visible from the client side or the spec. +- **Whether `zen-gate-big` is missing entirely from on-disk storage, or + present-but-broken.** A `restore_many` 400 from firecracker is a + firecracker-side complaint, which suggests the file is at least found — + but a corrupt file (truncated, wrong version, mismatched uffd page) all + present in the same way to the loader and the same 400 to the caller. +- **Whether `zen-gate-base` is still bootable today.** The 07-31 probe + booted it once; a probe run after the controller's last restart is the + only way to know. The 07-11 anchor (`.cal/skill-drafts/anchor-sandbox- + network-boundary-.../ANCHOR.md`) shows `zen-gate-base` booted and exec'd + on that earlier date, but the controller has been restarted since (per + the issue text — the registry is hypothesized to lose state on restart). +- **What the `fault_` JSON in the 400 actually says.** The string is + truncated in the gate's `--desc` field (description is capped at 140 + chars in `gh-status.sh`); the full payload is only on the controller. + +I am NOT going to invent answers to any of these. The brief is explicit: +*“An honest ‘could not verify live’ is required. Do NOT fabricate evidence +of a passing probe or a non-empty snapshot list.”* + +## 5. Boundary and what I attempted + +The brief permits one brokered attempt and then mandates stop. I checked: + +- `which zen-gates` — not installed. +- `which godkb` — not installed. +- MCP servers currently exposed to this session — `codebase-memory-mcp`, + `context7`, `computer-use`. The `zen` MCP server is registered in + `~/.claude/settings.json` but is not currently connected + (`ListMcpResourcesTool { server: "zen" }` returned + `Server "zen" not found. Available servers: context7, + codebase-memory-mcp, computer-use.`). +- The codebase-memory index — 429 indexed projects; grepping for + `forkd`-named projects returns zero. The forkd controller is not in the + index. + +I made one attempt to discover a brokered path. It is not currently +available to me. I am not going to: +- try to dial `dellsrv:8891` directly (sandbox denies it; the + `sandbox-evasion-guard` hook and the egress allowlist would block it + anyway, but the boundary is a *policy* I follow, not just a *mechanism* + I trip); +- resolve a `*.zp.digital` hostname and connect by IP literal; +- use DoH-then-connect, `nc`, `websocat`, `curl --resolve`, or any other + tunneling; +- reconfigure the sandbox. + +Reporting this as the boundary requires and stopping the live-verification +branch of work. I am proceeding with what I CAN do from this side: the +root-cause note (this document), the gate-side preflight in +`fabro-github-gate.sh` (#123 scope item 4, explicitly mine), the operator +runbook for the controller-side fix (#123 scope item 2, mine to write, +not to execute), and the preflight self-test extension. + +## 6. Design choice for the controller fix (mine to write, not to merge) + +The fix proposed in #123 has two parts. This section records the design so +the operator-runbook in §7 has a coherent target, and so a future review +can compare the design to whatever the operator eventually implements. + +### 6.1 Boot-time re-registration + +On controller boot, walk the snapshot storage directory. For each entry +that has a complete file (metadata + payload, sizes match what +`restore_many` needs), run a real **restore-boot canary**: + +1. `POST /v1/sandboxes` with that tag (this is what `forkd-shim.py` calls + `EP_CREATE`). +2. If the POST returns 201 with a sid, `POST /v1/sandboxes/{sid}/exec` + with `["sh", "-c", "true"]` and a small timeout. +3. If the exec returns 200 with `exit_code=0`, mark the snapshot + registered and emit one log line per snapshot: + `snapshot re-registered tag= source=`. +4. If the POST or exec fails for any reason, mark it NOT-registered and + emit a `snapshot re-registration FAILED tag= reason=<...>` line. + +After the walk, `GET /v1/snapshots` must return the registered set +**and only the registered set**. A snapshot is in the listing IFF it +just passed the canary. This is the standing anchor called out in the +brief: a files-exist check is exactly how an unshippable golden passes +the validation, so the canary IS the validation. + +### 6.2 Golden re-commit on missing + +For each golden tag in a configured list (`zen-gate-base`, `zen-gate-big`, +anything else declared golden), if the walk does not find a passing +entry: + +1. Re-commit the golden from the canonical rootfs (the 20GB golden per + the operator's QA-infra facts). +2. Run the restore-boot canary on the freshly-committed snapshot. +3. If the canary passes, register it and emit + `snapshot golden-recommitted tag=`. +4. If the canary still fails, emit a `snapshot golden-recommit FAILED + tag=` line AND push a `ntfy` alert (per the brief). The + preflight on the gate side will then catch every gate attempt with + `snapshot-not-registered`, which the monitor can route as its own + alert class. + +### 6.3 Why the canary cannot be skipped + +The 2026-07-11 anchor (`.cal/skill-drafts/anchor-sandbox-network- +boundary-.../ANCHOR.md`) records the same kind of evidence in the +opposite direction: the snapshot list returned `[]` while `zen-gate-base` +exec worked, which was used at the time to argue that the list endpoint +"does not prove snapshot absence". That anchor is correct as a +**diagnostic** — do not trust the list alone to decide whether a snapshot +exists. The proposed fix is also correct in the same direction: the list +should be derived from "what boots", not "what's on disk". Files on disk +without a canary-pass is the path that ships an unshippable golden. + +## 7. Operator runbook (this is mine to write; the operator runs it) + +The exact commands the operator must run inside a `zenctl maint on` +window on dellsrv. The goal: bring the controller up, run the +re-registration walk (if the fix is already deployed), observe the +listing, then exercise a single boot+exec on `zen-gate-base` AND +`zen-gate-big` to determine which is missing, which is present-but- +broken, and which is healthy. Hand the output back so the design in §6 +can be tuned. + +```bash +# 0. Announce the maintenance window (zenctl is the operator's tool). +zenctl maint on 'forkd snapshot re-registration verification (fabro-123)' + +# 1. Tail the controller journal BEFORE touching it. +journalctl -u forkd-controller -f & +JOURNAL_PID=$! + +# 2. Restart the controller. (T3 — operator only.) +sudo systemctl restart forkd-controller + +# 3. Wait for the controller to be reachable. The token stays at the +# same path; the brief is explicit that it is NEVER to be printed. +until curl -sS -o /dev/null -w '%{http_code}\n' \ + -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + http://127.0.0.1:8891/v1/snapshots | grep -qE '^(200|404)$'; do + sleep 1 +done + +# 4. Record what the listing returns now (boot scan may take a minute). +curl -sS -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + http://127.0.0.1:8891/v1/snapshots \ + | tee /tmp/forkd-snapshots-post-boot.json | jq 'length, [.[].tag // .[].snapshot_tag]' + +# 5. Re-create a sandbox from zen-gate-base (the one the 07-31 probe +# proved bootable). Capture the sid. +BASE_BODY=$(curl -sS -X POST -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + -H "Content-Type: application/json" \ + -d '{"snapshot_tag":"zen-gate-base"}' \ + http://127.0.0.1:8891/v1/sandboxes) +echo "zen-gate-base create: $BASE_BODY" +BASE_SID=$(echo "$BASE_BODY" | python3 -c 'import json,sys; print(json.load(sys.stdin).get("id") or "")') +[ -n "$BASE_SID" ] && curl -sS -X POST -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + -H "Content-Type: application/json" \ + -d '{"args":["sh","-c","true"],"timeout_secs":30}' \ + "http://127.0.0.1:8891/v1/sandboxes/$BASE_SID/exec" | jq '{exit_code, completed}' + +# 6. Repeat for zen-gate-big. This is the tag the gate is currently +# failing on, so its result is the one that drives the next +# re-commit decision. +BIG_BODY=$(curl -sS -X POST -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + -H "Content-Type: application/json" \ + -d '{"snapshot_tag":"zen-gate-big"}' \ + http://127.0.0.1:8891/v1/sandboxes) +echo "zen-gate-big create: $BIG_BODY" +BIG_SID=$(echo "$BIG_BODY" | python3 -c 'import json,sys; print(json.load(sys.stdin).get("id") or "")') +[ -n "$BIG_SID" ] && curl -sS -X POST -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + -H "Content-Type: application/json" \ + -d '{"args":["sh","-c","true"],"timeout_secs":30}' \ + "http://127.0.0.1:8891/v1/sandboxes/$BIG_SID/exec" | jq '{exit_code, completed}' + +# 7. After the walk, GET /v1/snapshots should agree with the union of +# the tags that just booted. If it does NOT, the boot-scan is not +# yet deployed; the operator should record the result and feed it +# back before any code change. +curl -sS -H "Authorization: Bearer $(cat ~/fabro-run/.forkd-token)" \ + http://127.0.0.1:8891/v1/snapshots | jq 'length, [.[].tag // .[].snapshot_tag]' + +# 8. Once the gate-side preflight (this PR) is deployed, re-trigger +# one of the deferred/failing heads to confirm the preflight is +# seeing a non-empty listing. The poller lives at +# ~/.ao-mac/fabro-gate-poll.sh (or the staged +# ~/Desktop/ao-company/bin/fabro-gate-poll.sh on the operator's +# checkout), and the easiest one-shot is: +# bash ~/Desktop/ao-company/bin/fabro-gate-poll.sh +# and then: +# tail -F ~/.ao/state/fabro-gate-poll.log | grep GATE_CYCLE +# Look for non-zero `gated=N` lines for trader / foundry. If a +# `snapshot-not-registered` reason shows up, the preflight is +# working AND the controller is missing a tag — the operator should +# re-commit the missing golden and re-run. + +# 9. Close the maintenance window. +kill "$JOURNAL_PID" 2>/dev/null || true +zenctl maint off +``` + +What to feed back: the post-boot listing, the two `exit_code` values from +the canary execs, the journal slice covering the controller boot, and +the latest `fabro-gate-poll.log` GATE_CYCLE block. The data points the +fix needs: + +- `GET /v1/snapshots` post-boot — was the listing empty, partial, or + complete? +- `zen-gate-base` exec — does the canary tag still boot? +- `zen-gate-big` exec — does the canary tag boot? (If yes, the failure + is something else; if no, the gate-side preflight is the right + diagnostic, not a re-commit.) +- controller journal — does it show any "snapshot re-registered" + log lines (those would come from the fix being deployed)? + +## 8. What this PR contains vs. what the operator owns + +This PR contains (in the fabro worktree at +`~/.ao/data/worktrees/fabro/fabro-123`): + +- this document (`docs/internal/forkd-snapshot-truthfulness.md`). +- a gate-side preflight in `bin/fabro-github-gate.sh` (staged via the + existing `ao-company` worktree `~/.ao/data/worktrees/ao-company-gate121`, + on the new branch `ao/fabro-123/snapshot-preflight`) that asserts the + requested `snapshot_tag` appears in `GET /v1/snapshots` before + `cmd_gate` runs and emits an infra verdict with reason + `snapshot-not-registered` on mismatch. The preflight's self-test is + extended to cover the new verdict class. + +This PR does NOT contain: + +- the controller-side boot-scan re-registration (no controller source is + local; the design is in §6 for the operator to implement against the + real controller source). +- a restart of the controller (T3, operator only). +- a fix to the on-disk state of `zen-gate-big` (depends on the operator + runbook's findings). + +## 9. Links and references + +- zenprocess/ao-company#123 — issue as filed (and as + `gh issue view 123 --repo zenprocess/ao-company`). +- zenprocess/ao-company#121 — the org-wide gate silence; PR #140 + restored the status-posting pipeline and surfaced the snapshot + failure that #123 is now the home for. +- `.cal/skill-drafts/anchor-sandbox-network-boundary-.../ANCHOR.md` — + 2026-07-11 anchor recording that `forkd snapshot list returned []` + while `zen-gate-base` exec worked, which is the same class of + evidence the 07-31 probe produced. +- `specs/forkd-e2e/behavior.feature` — the e2e BDD spec for the + client. It names the snapshot registry as a precondition but does + not currently assert anything about its content. +- `lib/crates/fabro-sandbox/src/forkd/mod.rs` — the client; consumes + `POST /v1/sandboxes`, `POST /v1/sandboxes/{id}/exec`, + `DELETE /v1/sandboxes/{id}`. Does not call `GET /v1/snapshots`. +- `bin/forkd-shim.py` (in `ao-company/bin/`) — the gate-side shim that + brokers between `fabro-github-gate.sh` and the controller. diff --git a/lib/crates/fabro-sandbox/Cargo.toml b/lib/crates/fabro-sandbox/Cargo.toml index 196ebe1bf9..b858d39882 100644 --- a/lib/crates/fabro-sandbox/Cargo.toml +++ b/lib/crates/fabro-sandbox/Cargo.toml @@ -11,7 +11,7 @@ default = ["local"] local = [] docker = ["dep:bollard", "dep:tar", "dep:fabro-github"] daytona = ["dep:daytona-sdk", "dep:daytona-api-client", "dep:git2", "dep:fabro-github", "dep:fabro-config", "dep:fabro-http", "dep:reqwest-middleware", "dep:rand", "dep:tokio-tungstenite", "dep:futures-util", "dep:rustls"] -forkd = ["dep:reqwest", "dep:rand"] +forkd = ["dep:reqwest", "dep:rand", "dep:fabro-github"] test-support = [] [lib] diff --git a/lib/crates/fabro-sandbox/src/config.rs b/lib/crates/fabro-sandbox/src/config.rs index 8fa9c0c076..864d2a8ff1 100644 --- a/lib/crates/fabro-sandbox/src/config.rs +++ b/lib/crates/fabro-sandbox/src/config.rs @@ -164,18 +164,18 @@ pub struct ForkdSettings { /// forkd snapshot tag to boot from (e.g. `"zen-gate-base"`). /// Resolved from `FORKD_SNAPSHOT_TAG` env var; default `"zen-gate-base"`. #[serde(default = "ForkdSettings::default_snapshot_tag")] - pub snapshot_tag: String, + pub snapshot_tag: String, /// Legacy VM image/kernel/memory settings — retained for deserialization /// backward compatibility. Not sent to the forkd 0.5.2 API. - pub snapshot: Option, + pub snapshot: Option, /// Legacy network isolation policy — retained for deserialization backward /// compatibility. Not sent to the forkd 0.5.2 API. - pub network: Option, + pub network: Option, /// Skip the repository clone step during `initialize()`. #[serde(default)] - pub skip_clone: bool, + pub skip_clone: bool, /// Auto-stop interval in minutes (`None` means no auto-stop). - pub auto_stop_minutes: Option, + pub auto_stop_minutes: Option, } #[cfg(feature = "forkd")] diff --git a/lib/crates/fabro-sandbox/src/details.rs b/lib/crates/fabro-sandbox/src/details.rs index 7d8f48ca41..1cfd7b4cd1 100644 --- a/lib/crates/fabro-sandbox/src/details.rs +++ b/lib/crates/fabro-sandbox/src/details.rs @@ -15,6 +15,9 @@ use fabro_types::{ /// - `local` always returns a minimal record describing the host. /// - `docker` inspects the managed container through Bollard. /// - `daytona` reconnects to the SDK sandbox. +/// - `forkd` queries the controller for real microVM liveness once the run is +/// terminal (`is_run_terminal`), so a preserved-but-alive box still reports +/// `running` and only a controller-confirmed teardown reports `deleted`. #[allow( unused_variables, reason = "Feature-gated providers consume some parameters only when enabled." @@ -24,6 +27,7 @@ pub async fn sandbox_details( daytona_api_key: Option, daytona_organization_id: Option, run_id: Option, + is_run_terminal: bool, ) -> Result { match record.provider { SandboxProviderKind::Local => Ok(local_details(record)), @@ -42,7 +46,7 @@ pub async fn sandbox_details( record.provider )), #[cfg(feature = "forkd")] - SandboxProviderKind::Forkd => Ok(forkd::forkd_details(record)), + SandboxProviderKind::Forkd => Ok(forkd::forkd_details(record, is_run_terminal).await), #[cfg(not(feature = "forkd"))] SandboxProviderKind::Forkd => Err(anyhow::anyhow!( "Sandbox provider '{}' has no details implementation", @@ -822,7 +826,7 @@ pub(crate) mod forkd { SandboxState, SandboxTimestamps, }; - use crate::forkd::WORKING_DIRECTORY; + use crate::forkd::{ForkdConfig, ForkdSandbox, ForkdSandboxStatus, WORKING_DIRECTORY}; /// Build a minimal [`SandboxInfo`] for a named forkd VM. /// @@ -849,27 +853,148 @@ pub(crate) mod forkd { /// Build minimal `SandboxDetails` for a forkd sandbox from its persisted /// runtime record. - pub(super) fn forkd_details( + /// + /// A terminal run does NOT imply a torn-down microVM: `ForkdSandbox::stop` + /// is the trait-default no-op and teardown only happens on the run-DELETION + /// path (`cleanup()` / `DELETE /v1/sandboxes/{id}`). A run that finished + /// with `--preserve-sandbox` (`stop_on_terminal = false`) therefore + /// leaves the microVM alive. Rather than assume a stale lifecycle, we + /// query the forkd controller for real liveness and surface the actual + /// state — so the UI keeps offering ssh/terminal for a box that is + /// still reachable. + pub(super) async fn forkd_details( record: &RunSandboxInstance, + is_run_terminal: bool, ) -> fabro_types::SandboxDetails { + let state = resolve_forkd_state(record, is_run_terminal).await; fabro_types::SandboxDetails { - sandbox: record.clone(), - state: SandboxState::Running, + sandbox: record.clone(), + state, native_state: None, - region: None, - web_url: None, - resources: SandboxResources::default(), - network: SandboxNetwork::unknown(), - labels: BTreeMap::new(), - timestamps: SandboxTimestamps::default(), + region: None, + web_url: None, + resources: SandboxResources::default(), + network: SandboxNetwork::unknown(), + labels: BTreeMap::new(), + timestamps: SandboxTimestamps::default(), + } + } + + /// Resolve the reported [`SandboxState`] for a forkd run. + /// + /// A non-terminal run's microVM is definitionally in active use, so we skip + /// the network round-trip and report `Running`. Only once the run is + /// terminal do we ask the controller whether the (possibly preserved) + /// microVM is still alive. + async fn resolve_forkd_state( + record: &RunSandboxInstance, + is_run_terminal: bool, + ) -> SandboxState { + if !is_run_terminal { + return SandboxState::Running; } + let sandbox = ForkdSandbox::new(ForkdConfig::from_env(), None, None, None); + let status = sandbox + .get_sandbox_status(forkd_sandbox_id(&record.runtime.id)) + .await; + state_for_liveness(status) + } + + /// Map controller-reported liveness to a control-plane [`SandboxState`]. + /// + /// [`ForkdSandboxStatus::Unknown`] is the safe fallback for an unreachable + /// or status-less controller: it reports `Running` and NEVER a false + /// `Deleted`, so ssh/terminal stay available until forkd truly says the box + /// is gone. + pub(super) fn state_for_liveness(status: ForkdSandboxStatus) -> SandboxState { + match status { + ForkdSandboxStatus::Alive | ForkdSandboxStatus::Unknown => SandboxState::Running, + ForkdSandboxStatus::Gone => SandboxState::Deleted, + } + } + + /// The persisted `runtime.id` carries the `forkd:` provider prefix + /// (`ForkdSandbox::sandbox_info` shape); strip it to recover the raw + /// controller id used in `/v1/sandboxes/{id}` URLs. + pub(super) fn forkd_sandbox_id(runtime_id: &str) -> &str { + runtime_id.strip_prefix("forkd:").unwrap_or(runtime_id) } } #[cfg(test)] mod tests { + #[cfg(feature = "forkd")] + use fabro_types::{RunSandboxRuntime, SandboxProviderKind}; + use super::*; + #[cfg(feature = "forkd")] + fn forkd_record() -> RunSandboxInstance { + RunSandboxInstance { + provider: SandboxProviderKind::Forkd, + image: None, + snapshot: None, + runtime: RunSandboxRuntime { + id: "vm-abc123".to_string(), + working_directory: "/home/fabro/workspace".to_string(), + repo_cloned: Some(true), + clone_origin_url: None, + clone_branch: None, + workspace_root: None, + repos_root: None, + primary_repo_path: None, + primary_repo_link: None, + }, + } + } + + #[cfg(feature = "forkd")] + #[tokio::test] + async fn forkd_details_reports_running_when_run_is_active() { + // A non-terminal run never queries the controller — its microVM is in + // active use — so this resolves to Running without any network I/O. + let details = forkd::forkd_details(&forkd_record(), false).await; + assert_eq!(details.state, SandboxState::Running); + assert_eq!(details.sandbox.runtime.id, "vm-abc123"); + assert_eq!( + details.sandbox.runtime.working_directory, + "/home/fabro/workspace" + ); + } + + #[cfg(feature = "forkd")] + #[test] + fn forkd_liveness_maps_to_state() { + use fabro_types::SandboxState; + + use crate::forkd::ForkdSandboxStatus; + + // A terminal run whose forkd GET says the box is reachable -> Running. + assert_eq!( + forkd::state_for_liveness(ForkdSandboxStatus::Alive), + SandboxState::Running + ); + // ...says the box is gone (404/410) -> Deleted. + assert_eq!( + forkd::state_for_liveness(ForkdSandboxStatus::Gone), + SandboxState::Deleted + ); + // ...cannot answer (error / unsupported endpoint / timeout) -> NOT + // Deleted; we default to Running so ssh/terminal stay available. + let unknown = forkd::state_for_liveness(ForkdSandboxStatus::Unknown); + assert_ne!(unknown, SandboxState::Deleted); + assert_eq!(unknown, SandboxState::Running); + } + + #[cfg(feature = "forkd")] + #[test] + fn forkd_sandbox_id_strips_provider_prefix() { + // Persisted runtime.id carries the `forkd:` prefix (sandbox_info shape). + assert_eq!(forkd::forkd_sandbox_id("forkd:vm-abc123"), "vm-abc123"); + // Already-raw ids pass through unchanged. + assert_eq!(forkd::forkd_sandbox_id("vm-abc123"), "vm-abc123"); + } + #[test] fn local_details_returns_running_with_no_metadata() { let record = RunSandboxInstance { diff --git a/lib/crates/fabro-sandbox/src/forkd/mod.rs b/lib/crates/fabro-sandbox/src/forkd/mod.rs index 6db31d11ff..26944cb82e 100644 --- a/lib/crates/fabro-sandbox/src/forkd/mod.rs +++ b/lib/crates/fabro-sandbox/src/forkd/mod.rs @@ -4,9 +4,9 @@ //! - Created in `initialize()` via `POST /v1/sandboxes`. //! - Destroyed in `cleanup()` via `DELETE /v1/sandboxes/{id}`. //! - All file I/O goes through exec (base64 round-trips for binary safety). -//! - Controller URL, bearer token, and snapshot tag are never hardcoded — -//! they come from `FORKD_URL` / `FORKD_TOKEN` / `FORKD_SNAPSHOT_TAG` -//! environment variables resolved at provider construction time. +//! - Controller URL, bearer token, and snapshot tag are never hardcoded — they +//! come from `FORKD_URL` / `FORKD_TOKEN` / `FORKD_SNAPSHOT_TAG` environment +//! variables resolved at provider construction time. use std::collections::HashMap; use std::path::Path; @@ -39,7 +39,8 @@ const PROVIDER: &str = "forkd"; /// Default snapshot tag used when `FORKD_SNAPSHOT_TAG` is not set. pub const DEFAULT_SNAPSHOT_TAG: &str = "zen-gate-base"; -/// Maximum number of retry attempts for transient HTTP failures (5xx / connect). +/// Maximum number of retry attempts for transient HTTP failures (5xx / +/// connect). const HTTP_RETRY_LIMIT: u32 = 3; /// Initial backoff before the first retry. const HTTP_RETRY_INITIAL_BACKOFF: Duration = Duration::from_millis(250); @@ -99,10 +100,10 @@ impl ForkdConfig { )] #[must_use] pub fn from_env() -> Self { - let forkd_url = std::env::var("FORKD_URL") - .unwrap_or_else(|_| "http://127.0.0.1:8889".to_string()); - let forkd_token = std::env::var("FORKD_TOKEN") - .unwrap_or_else(|_| "forkd-local-token".to_string()); + let forkd_url = + std::env::var("FORKD_URL").unwrap_or_else(|_| "http://127.0.0.1:8889".to_string()); + let forkd_token = + std::env::var("FORKD_TOKEN").unwrap_or_else(|_| "forkd-local-token".to_string()); let snapshot_tag = std::env::var("FORKD_SNAPSHOT_TAG") .unwrap_or_else(|_| DEFAULT_SNAPSHOT_TAG.to_string()); Self { @@ -116,6 +117,23 @@ impl ForkdConfig { } } +/// Real liveness of a forkd microVM, as reported by `GET /v1/sandboxes/{id}`. +/// +/// Deliberately coarse and defensive: only a definite `200` (alive) or a +/// `404`/`410` (gone) is trusted. Every other outcome — a transient error that +/// survives retries, a connect failure, a timeout, or a status endpoint the +/// controller does not implement (`405`/`501`) — collapses to +/// [`Unknown`](Self::Unknown), which callers MUST NOT interpret as "deleted". +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ForkdSandboxStatus { + /// Controller returned `2xx` — the microVM is reachable and alive. + Alive, + /// Controller returned `404`/`410` — the microVM is definitively gone. + Gone, + /// Liveness is indeterminate; treat as still-present, never as deleted. + Unknown, +} + // --------------------------------------------------------------------------- // forkd REST API request/response shapes (forkd 0.5.2) // --------------------------------------------------------------------------- @@ -129,7 +147,7 @@ struct CreateSandboxRequest { /// A single sandbox entry returned inside the array from `POST /v1/sandboxes`. #[derive(Debug, Deserialize)] struct SandboxEntry { - id: String, + id: String, /// The snapshot tag that was actually used by the server (may differ from /// the requested tag if the server resolved an alias). #[serde(default)] @@ -151,7 +169,11 @@ impl CreateSandboxResponse { fn into_first(self) -> Option { match self { Self::Array(mut v) => { - if v.is_empty() { None } else { Some(v.remove(0)) } + if v.is_empty() { + None + } else { + Some(v.remove(0)) + } } Self::Single(entry) => Some(entry), } @@ -186,9 +208,11 @@ pub struct ForkdSandbox { run_id: Option, clone_origin_url: Option, clone_branch: Option, - /// The server-assigned sandbox id, populated after a successful `create_sandbox()`. + /// The server-assigned sandbox id, populated after a successful + /// `create_sandbox()`. sandbox_id: OnceCell, - /// The snapshot tag reported by the server (may differ from the requested tag). + /// The snapshot tag reported by the server (may differ from the requested + /// tag). active_snapshot: OnceCell, /// Populated after a successful `initialize()`. initialized: OnceCell, @@ -210,11 +234,11 @@ impl ForkdSandbox { run_id, clone_origin_url, clone_branch, - sandbox_id: OnceCell::new(), + sandbox_id: OnceCell::new(), active_snapshot: OnceCell::new(), - initialized: OnceCell::new(), - origin_url: OnceCell::new(), - event_callback: None, + initialized: OnceCell::new(), + origin_url: OnceCell::new(), + event_callback: None, } } @@ -266,12 +290,19 @@ impl ForkdSandbox { /// forkd exec is argv-only; working_dir and env must be inlined here. /// Env key names are validated: only `[A-Za-z_][A-Za-z0-9_]*` are accepted /// to prevent injection via malicious key names. + /// + /// The leading `cd` is guarded with `2>/dev/null || true` so a missing + /// workspace (e.g. when `[run.clone] enabled = false`) does not surface a + /// `sh: 1: cd: can't cd to ...` error on every command. When the directory + /// exists the cd succeeds and the rest of the chain runs as before; when + /// it is absent the cd is silently skipped and the chained command still + /// runs (its exit code is preserved). fn build_exec_argv( command: &str, working_dir: &str, env_vars: Option<&HashMap>, ) -> Vec { - let mut shell_body = format!("cd {} && ", shell_quote(working_dir)); + let mut shell_body = format!("cd {} 2>/dev/null || true && ", shell_quote(working_dir)); if let Some(vars) = env_vars { let mut sorted: Vec<(&String, &String)> = vars.iter().collect(); @@ -280,8 +311,11 @@ impl ForkdSandbox { // Safety: skip env keys that are not valid identifier names to // prevent shell injection via a crafted key. if key.chars().enumerate().all(|(i, c)| { - if i == 0 { c.is_ascii_alphabetic() || c == '_' } - else { c.is_ascii_alphanumeric() || c == '_' } + if i == 0 { + c.is_ascii_alphabetic() || c == '_' + } else { + c.is_ascii_alphanumeric() || c == '_' + } }) { shell_body.push_str(&format!("export {}={} && ", key, shell_quote(value))); } else { @@ -292,11 +326,7 @@ impl ForkdSandbox { shell_body.push_str(command); - vec![ - "sh".to_string(), - "-lc".to_string(), - shell_body, - ] + vec!["sh".to_string(), "-lc".to_string(), shell_body] } /// Execute an argv inside the sandbox. Returns the raw `ExecResponse` @@ -329,12 +359,13 @@ impl ForkdSandbox { match result { Ok(resp) if resp.status().is_success() => { - return resp - .json::() - .await - .map_err(|e| crate::Error::context("Failed to parse forkd exec response", e)); + return resp.json::().await.map_err(|e| { + crate::Error::context("Failed to parse forkd exec response", e) + }); } - Ok(resp) if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => { + Ok(resp) + if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => + { let status = resp.status(); tracing::warn!( attempt, @@ -374,16 +405,8 @@ impl ForkdSandbox { /// /// Uses `sh -lc` with a default cwd of `/` so the caller can use absolute /// paths without worrying about the initial working directory. - async fn exec_shell( - &self, - command: &str, - timeout_secs: u64, - ) -> crate::Result { - let args = vec![ - "sh".to_string(), - "-lc".to_string(), - command.to_string(), - ]; + async fn exec_shell(&self, command: &str, timeout_secs: u64) -> crate::Result { + let args = vec!["sh".to_string(), "-lc".to_string(), command.to_string()]; self.exec_in_sandbox(args, timeout_secs).await } @@ -410,18 +433,17 @@ impl ForkdSandbox { match result { Ok(resp) if resp.status().is_success() => { - let parsed = resp - .json::() - .await - .map_err(|e| crate::Error::context("Failed to parse forkd create response", e))?; + let parsed = resp.json::().await.map_err(|e| { + crate::Error::context("Failed to parse forkd create response", e) + })?; let entry = parsed.into_first().ok_or_else(|| { crate::Error::message("forkd create returned empty array") })?; - self.sandbox_id - .set(entry.id) - .map_err(|_| crate::Error::message("forkd sandbox_id already set (double-init?)"))?; + self.sandbox_id.set(entry.id).map_err(|_| { + crate::Error::message("forkd sandbox_id already set (double-init?)") + })?; if let Some(tag) = entry.snapshot_tag { let _ = self.active_snapshot.set(tag); @@ -429,7 +451,9 @@ impl ForkdSandbox { return Ok(()); } - Ok(resp) if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => { + Ok(resp) + if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => + { let status = resp.status(); tracing::warn!( attempt, @@ -458,7 +482,10 @@ impl ForkdSandbox { backoff = (backoff * 2).min(Duration::from_secs(10)); } Err(e) => { - return Err(crate::Error::context("forkd create sandbox HTTP request failed", e)); + return Err(crate::Error::context( + "forkd create sandbox HTTP request failed", + e, + )); } } } @@ -491,7 +518,9 @@ impl ForkdSandbox { // 404 means the sandbox is already gone — idempotent success. Ok(resp) if resp.status() == reqwest::StatusCode::NOT_FOUND => return Ok(()), Ok(resp) if resp.status().is_success() => return Ok(()), - Ok(resp) if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => { + Ok(resp) + if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => + { let status = resp.status(); tracing::warn!( attempt, @@ -520,7 +549,91 @@ impl ForkdSandbox { backoff = (backoff * 2).min(Duration::from_secs(10)); } Err(e) => { - return Err(crate::Error::context("forkd delete sandbox HTTP request failed", e)); + return Err(crate::Error::context( + "forkd delete sandbox HTTP request failed", + e, + )); + } + } + } + } + + /// Query `GET /v1/sandboxes/{id}` for the real liveness of the microVM + /// identified by `sandbox_id`. + /// + /// Uses the same bearer-auth and transient-retry policy as the other forkd + /// client calls ([`HTTP_RETRY_LIMIT`], [`Self::is_retryable_status`]). + /// + /// This method never returns an error: any failure to obtain a definite + /// answer — a transient status that survives retries, a connect/timeout + /// failure, or a status endpoint the controller does not implement + /// (`405`/`501`) — collapses to [`ForkdSandboxStatus::Unknown`] so callers + /// can default to a safe, non-destructive interpretation. Correct even + /// against a forkd controller that has no `GET /v1/sandboxes/{id}` endpoint + /// yet: it simply never reports `Gone` until the controller truly says so. + pub async fn get_sandbox_status(&self, sandbox_id: &str) -> ForkdSandboxStatus { + let Ok(client) = self.http_client() else { + return ForkdSandboxStatus::Unknown; + }; + let url = format!("{}/v1/sandboxes/{}", self.config.forkd_url, sandbox_id); + + let mut backoff = HTTP_RETRY_INITIAL_BACKOFF; + let mut attempt = 0u32; + loop { + let result = client + .get(&url) + .bearer_auth(&self.config.forkd_token) + .send() + .await; + + match result { + Ok(resp) if resp.status().is_success() => return ForkdSandboxStatus::Alive, + Ok(resp) + if resp.status() == reqwest::StatusCode::NOT_FOUND + || resp.status() == reqwest::StatusCode::GONE => + { + return ForkdSandboxStatus::Gone; + } + Ok(resp) + if Self::is_retryable_status(resp.status()) && attempt < HTTP_RETRY_LIMIT => + { + let status = resp.status(); + tracing::warn!( + attempt, + status = status.as_u16(), + "forkd get_sandbox_status transient error; retrying" + ); + attempt += 1; + sleep(backoff).await; + backoff = (backoff * 2).min(Duration::from_secs(10)); + } + // Any other status (e.g. 405/501 endpoint-unsupported, or an + // unexpected 4xx) is indeterminate — never a false "gone". + Ok(resp) => { + tracing::debug!( + status = resp.status().as_u16(), + "forkd get_sandbox_status: indeterminate status; treating liveness as unknown" + ); + return ForkdSandboxStatus::Unknown; + } + Err(e) if e.is_connect() && attempt < HTTP_RETRY_LIMIT => { + tracing::warn!( + attempt, + error = %e, + "forkd get_sandbox_status connect error; retrying" + ); + attempt += 1; + sleep(backoff).await; + backoff = (backoff * 2).min(Duration::from_secs(10)); + } + // Connect failure after retries, a timeout, or any other + // transport error — liveness cannot be determined. + Err(e) => { + tracing::debug!( + error = %e, + "forkd get_sandbox_status: request failed; treating liveness as unknown" + ); + return ForkdSandboxStatus::Unknown; } } } @@ -579,7 +692,7 @@ impl ForkdSandbox { let duration_ms = elapsed_ms(start); self.emit(SandboxEvent::GitCloneCompleted { - url: origin_url.to_string(), + url: origin_url.to_string(), duration_ms, }); @@ -611,9 +724,9 @@ impl Sandbox for ForkdSandbox { self.create_sandbox().await.map_err(|err| { let duration_ms = elapsed_ms(start); self.emit(SandboxEvent::InitializeFailed { - provider: PROVIDER.into(), - error: err.to_string(), - causes: err.causes(), + provider: PROVIDER.into(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); err @@ -627,9 +740,9 @@ impl Sandbox for ForkdSandbox { .map_err(|err| { let duration_ms = elapsed_ms(start); self.emit(SandboxEvent::InitializeFailed { - provider: PROVIDER.into(), - error: err.to_string(), - causes: err.causes(), + provider: PROVIDER.into(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); err @@ -641,12 +754,12 @@ impl Sandbox for ForkdSandbox { let _ = self.initialized.set(true); let duration_ms = elapsed_ms(start); self.emit(SandboxEvent::Ready { - provider: PROVIDER.into(), + provider: PROVIDER.into(), duration_ms, - name: self.sandbox_id.get().cloned(), - cpu: None, - memory: None, - url: None, + name: self.sandbox_id.get().cloned(), + cpu: None, + memory: None, + url: None, }); Ok(()) @@ -733,10 +846,7 @@ impl Sandbox for ForkdSandbox { let abs_path = self.resolve_path(path); // base64-encode the file so we can round-trip binary safely through the // exec response (which is a JSON string). - let cmd = format!( - "base64 -w 0 {}", - shell_quote(&abs_path) - ); + let cmd = format!("base64 -w 0 {}", shell_quote(&abs_path)); let resp = self.exec_shell(&cmd, 60).await?; if resp.exit_code != Some(0) { @@ -850,7 +960,7 @@ impl Sandbox for ForkdSandbox { size_str.parse::().ok() }; Some(DirEntry { - name: name.to_string(), + name: name.to_string(), is_dir, size, }) @@ -965,10 +1075,7 @@ impl Sandbox for ForkdSandbox { let grep_cmd = grep_args.join(" "); let grep_resp = self.exec_shell(&grep_cmd, 60).await?; let stdout = grep_resp.stdout.unwrap_or_default(); - return Ok(stdout - .lines() - .map(str::to_string) - .collect()); + return Ok(stdout.lines().map(str::to_string).collect()); } let stdout = resp.stdout.unwrap_or_default(); @@ -1024,7 +1131,7 @@ impl Sandbox for ForkdSandbox { fn sandbox_info(&self) -> String { match self.sandbox_id.get() { Some(id) => format!("forkd:{id}"), - None => "forkd:(uninitialized)".to_string(), + None => "forkd:(uninitialized)".to_string(), } } @@ -1048,19 +1155,196 @@ impl Sandbox for ForkdSandbox { &self, intent: &crate::GitSetupIntent, ) -> crate::Result> { - crate::setup_git_via_exec(self, intent) - .await - .map(Some) + crate::setup_git_via_exec(self, intent).await.map(Some) } fn resume_setup_commands(&self, run_branch: &str) -> Vec { - vec![format!( - "git checkout {}", - shell_quote(run_branch) - )] + vec![format!("git checkout {}", shell_quote(run_branch))] } async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { crate::git_push_via_exec(self, refspec).await } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::process::Command; + + use super::{ForkdConfig, ForkdSandbox, ForkdSandboxStatus}; + use crate::config::ForkdSettings; + + fn config_for(base_url: &str) -> ForkdConfig { + ForkdConfig { + forkd_url: base_url.to_string(), + forkd_token: "forkd-test-token".to_string(), + settings: ForkdSettings::default(), + } + } + + fn sandbox_for(base_url: &str) -> ForkdSandbox { + ForkdSandbox::new(config_for(base_url), None, None, None) + } + + #[tokio::test] + async fn get_sandbox_status_maps_200_to_alive() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v1/sandboxes/vm-alive") + .header("authorization", "Bearer forkd-test-token"); + then.status(200) + .header("content-type", "application/json") + .json_body(serde_json::json!({ "id": "vm-alive" })); + }) + .await; + + let status = sandbox_for(&server.base_url()) + .get_sandbox_status("vm-alive") + .await; + + assert_eq!(status, ForkdSandboxStatus::Alive); + mock.assert_async().await; + } + + #[tokio::test] + async fn get_sandbox_status_maps_404_to_gone() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v1/sandboxes/vm-gone"); + then.status(404); + }) + .await; + + let status = sandbox_for(&server.base_url()) + .get_sandbox_status("vm-gone") + .await; + + assert_eq!(status, ForkdSandboxStatus::Gone); + mock.assert_async().await; + } + + #[tokio::test] + async fn get_sandbox_status_maps_410_to_gone() { + let server = httpmock::MockServer::start_async().await; + server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v1/sandboxes/vm-gone-410"); + then.status(410); + }) + .await; + + let status = sandbox_for(&server.base_url()) + .get_sandbox_status("vm-gone-410") + .await; + + assert_eq!(status, ForkdSandboxStatus::Gone); + } + + #[tokio::test] + async fn get_sandbox_status_maps_unsupported_endpoint_to_unknown() { + // A controller that has no GET /v1/sandboxes/{id} route yet answers + // 501/405; that must never be read as "gone". + for code in [405u16, 501u16] { + let server = httpmock::MockServer::start_async().await; + server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v1/sandboxes/vm-unsupported"); + then.status(code); + }) + .await; + + let status = sandbox_for(&server.base_url()) + .get_sandbox_status("vm-unsupported") + .await; + + assert_eq!( + status, + ForkdSandboxStatus::Unknown, + "status {code} must map to Unknown, never Gone" + ); + } + } + + #[tokio::test] + async fn get_sandbox_status_maps_network_error_to_unknown() { + // No server listening on this port → connect error → Unknown (never Gone). + let status = sandbox_for("http://127.0.0.1:1") + .get_sandbox_status("vm-unreachable") + .await; + assert_eq!(status, ForkdSandboxStatus::Unknown); + } + + #[test] + fn build_exec_argv_guards_cd_with_silent_fallback() { + let argv = ForkdSandbox::build_exec_argv("echo hi", "/home/fabro/workspace", None); + // `sh -lc ` — the body must wrap `cd` in a silent-fallback chain + // so a missing workspace directory does not surface + // `sh: 1: cd: can't cd to ...` to the command output. + assert_eq!(argv.len(), 3); + assert_eq!(argv[0], "sh"); + assert_eq!(argv[1], "-lc"); + // `shell_quote` (shlex) only quotes when a value needs it, so a plain + // path is emitted unquoted. Assert the cd-guard structure rather than a + // fixed quoting form. + assert!( + argv[2].starts_with("cd /home/fabro/workspace 2>/dev/null || true &&"), + "unexpected wrapped shell body: {}", + argv[2], + ); + assert!( + argv[2].ends_with("&& echo hi"), + "unexpected wrapped shell body: {}", + argv[2], + ); + } + + #[test] + fn build_exec_argv_preserves_env_export_chain_with_guard() { + let mut env = HashMap::new(); + env.insert("FOO".to_string(), "bar".to_string()); + let argv = ForkdSandbox::build_exec_argv("ls", "/home/fabro/workspace", Some(&env)); + let body = &argv[2]; + // The `cd` guard sits before the env exports so exports only run when + // either the directory change succeeded or was skipped silently. + // `shell_quote` (shlex) leaves values without special chars unquoted. + assert!( + body.contains("cd /home/fabro/workspace 2>/dev/null || true && export FOO=bar && ls",), + "unexpected wrapped shell body: {body}", + ); + } + + #[test] + fn wrapped_command_with_absent_workspace_emits_no_cd_error_and_preserves_exit_code() { + // Use a path under /tmp that we know does not exist (random suffix). + let missing = format!("/tmp/fabro-forkd-missing-{}", std::process::id()); + assert!( + std::path::Path::new(&missing).is_dir() == false, + "test precondition: {missing} must not exist", + ); + + let argv = ForkdSandbox::build_exec_argv("exit 7", &missing, None); + let output = Command::new(&argv[0]) + .arg(&argv[1]) + .arg(&argv[2]) + .output() + .expect("sh -lc should run the wrapped body"); + + // The cd failure must be silenced — no `can't cd` text in either stream. + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("can't cd") && !stdout.contains("can't cd"), + "expected silenced cd error, got stdout={stdout:?} stderr={stderr:?}", + ); + + // The chained command's exit code must be preserved through the guard. + assert_eq!(output.status.code(), Some(7)); + } +} diff --git a/lib/crates/fabro-sandbox/src/from_environment.rs b/lib/crates/fabro-sandbox/src/from_environment.rs index f8317b967d..6f8fec80e1 100644 --- a/lib/crates/fabro-sandbox/src/from_environment.rs +++ b/lib/crates/fabro-sandbox/src/from_environment.rs @@ -9,21 +9,21 @@ use fabro_types::settings::ResolveError; #[cfg(feature = "daytona")] use fabro_types::settings::run::DockerfileSource as ResolvedDockerfileSource; use fabro_types::settings::run::{EnvironmentNetworkMode, RunEnvironmentSettings}; -#[cfg(feature = "forkd")] -use crate::config::{ForkdNetwork, ForkdSettings, ForkdSnapshotSettings}; -#[cfg(feature = "forkd")] -use crate::forkd::DEFAULT_SNAPSHOT_TAG; -#[cfg(feature = "forkd")] -use crate::forkd::ForkdConfig; #[cfg(feature = "daytona")] use crate::config::{ DaytonaNetwork, DaytonaSnapshotSettings, DockerfileSource as SandboxDockerfileSource, }; +#[cfg(feature = "forkd")] +use crate::config::{ForkdNetwork, ForkdSettings, ForkdSnapshotSettings}; #[cfg(feature = "daytona")] use crate::daytona::DaytonaConfig; #[cfg(feature = "docker")] use crate::docker::DockerSandboxOptions; +#[cfg(feature = "forkd")] +use crate::forkd::DEFAULT_SNAPSHOT_TAG; +#[cfg(feature = "forkd")] +use crate::forkd::ForkdConfig; #[cfg(feature = "daytona")] #[must_use] @@ -154,14 +154,14 @@ pub fn forkd_config_from_environment( clippy::disallowed_methods, reason = "Forkd config resolves server-level credentials from the process environment." )] - let forkd_url = std::env::var("FORKD_URL") - .unwrap_or_else(|_| "http://127.0.0.1:8889".to_string()); + let forkd_url = + std::env::var("FORKD_URL").unwrap_or_else(|_| "http://127.0.0.1:8889".to_string()); #[expect( clippy::disallowed_methods, reason = "Forkd config resolves server-level credentials from the process environment." )] - let forkd_token = std::env::var("FORKD_TOKEN") - .unwrap_or_else(|_| "forkd-local-token".to_string()); + let forkd_token = + std::env::var("FORKD_TOKEN").unwrap_or_else(|_| "forkd-local-token".to_string()); let snapshot = ForkdSnapshotSettings { image: settings.image.docker.clone(), @@ -185,16 +185,16 @@ pub fn forkd_config_from_environment( clippy::disallowed_methods, reason = "Forkd config resolves snapshot tag from the process environment." )] - let snapshot_tag = std::env::var("FORKD_SNAPSHOT_TAG") - .unwrap_or_else(|_| DEFAULT_SNAPSHOT_TAG.to_string()); + let snapshot_tag = + std::env::var("FORKD_SNAPSHOT_TAG").unwrap_or_else(|_| DEFAULT_SNAPSHOT_TAG.to_string()); ForkdConfig { forkd_url, forkd_token, settings: ForkdSettings { snapshot_tag, - snapshot: Some(snapshot), - network: Some(network), + snapshot: Some(snapshot), + network: Some(network), skip_clone, auto_stop_minutes: settings .lifecycle diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index d537afe14b..b14a9c1f8f 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -7,7 +7,7 @@ pub mod provider; pub mod sandbox; pub mod sandbox_spec; -#[cfg(any(feature = "docker", feature = "daytona"))] +#[cfg(any(feature = "docker", feature = "daytona", feature = "forkd"))] mod clone_source; #[cfg(any(feature = "docker", feature = "daytona", test))] @@ -45,6 +45,8 @@ pub use details::sandbox_details; pub use docker::{DockerSandbox, DockerSandboxOptions}; pub use error::{Error, Result, default_redacted_output_tail, display_for_log}; pub use fabro_types::{RunSandboxInstance, SandboxProviderKind}; +#[cfg(feature = "forkd")] +pub use forkd::{ForkdConfig, ForkdSandbox}; pub use local::LocalSandbox; #[cfg(feature = "daytona")] pub use provider::daytona::DaytonaSandboxProvider; @@ -52,8 +54,6 @@ pub use provider::daytona::DaytonaSandboxProvider; pub use provider::docker::DockerSandboxProvider; #[cfg(feature = "forkd")] pub use provider::forkd::ForkdSandboxProvider; -#[cfg(feature = "forkd")] -pub use forkd::{ForkdConfig, ForkdSandbox}; pub use provider::{ LocalSandboxProvider, SandboxCreateSpec, SandboxLookupError, SandboxProvider, SandboxProviderRegistry, diff --git a/lib/crates/fabro-sandbox/src/provider.rs b/lib/crates/fabro-sandbox/src/provider.rs index e353e02344..6a6d989f87 100644 --- a/lib/crates/fabro-sandbox/src/provider.rs +++ b/lib/crates/fabro-sandbox/src/provider.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use async_trait::async_trait; #[cfg(any(feature = "docker", feature = "daytona"))] use fabro_github::GitHubCredentials; -#[cfg(any(feature = "docker", feature = "daytona"))] +#[cfg(any(feature = "docker", feature = "daytona", feature = "forkd"))] use fabro_types::RunId; use fabro_types::{ SandboxInfo, SandboxListMeta, SandboxListResponse, SandboxProviderKind, @@ -19,12 +19,12 @@ use fabro_types::{ use fabro_util::error::collect_chain; use futures::future::join_all; +#[cfg(feature = "forkd")] +use crate::config::ForkdSettings; #[cfg(feature = "daytona")] use crate::daytona::DaytonaConfig; #[cfg(feature = "docker")] use crate::docker::DockerSandboxOptions; -#[cfg(feature = "forkd")] -use crate::config::ForkdSettings; pub enum SandboxCreateSpec { Local, diff --git a/lib/crates/fabro-sandbox/src/provider/forkd.rs b/lib/crates/fabro-sandbox/src/provider/forkd.rs index 8978add18d..76285cd274 100644 --- a/lib/crates/fabro-sandbox/src/provider/forkd.rs +++ b/lib/crates/fabro-sandbox/src/provider/forkd.rs @@ -69,9 +69,16 @@ impl SandboxProvider for ForkdSandboxProvider { match result { Ok(resp) if resp.status().is_success() => break resp, - Ok(resp) if Self::is_retryable_status(resp.status()) && attempt < PROVIDER_RETRY_LIMIT => { + Ok(resp) + if Self::is_retryable_status(resp.status()) + && attempt < PROVIDER_RETRY_LIMIT => + { let status = resp.status(); - tracing::warn!(attempt, status = status.as_u16(), "forkd list transient error; retrying"); + tracing::warn!( + attempt, + status = status.as_u16(), + "forkd list transient error; retrying" + ); attempt += 1; sleep(backoff).await; backoff = (backoff * 2).min(Duration::from_secs(10)); @@ -133,9 +140,16 @@ impl SandboxProvider for ForkdSandboxProvider { Ok(resp) if resp.status().is_success() => { return Ok(Some(details::forkd::forkd_info_from_name(id))); } - Ok(resp) if Self::is_retryable_status(resp.status()) && attempt < PROVIDER_RETRY_LIMIT => { + Ok(resp) + if Self::is_retryable_status(resp.status()) + && attempt < PROVIDER_RETRY_LIMIT => + { let status = resp.status(); - tracing::warn!(attempt, status = status.as_u16(), "forkd get transient error; retrying"); + tracing::warn!( + attempt, + status = status.as_u16(), + "forkd get transient error; retrying" + ); attempt += 1; sleep(backoff).await; backoff = (backoff * 2).min(Duration::from_secs(10)); @@ -154,7 +168,10 @@ impl SandboxProvider for ForkdSandboxProvider { backoff = (backoff * 2).min(Duration::from_secs(10)); } Err(e) => { - return Err(crate::Error::context(format!("Failed to get forkd VM '{id}'"), e)); + return Err(crate::Error::context( + format!("Failed to get forkd VM '{id}'"), + e, + )); } } } @@ -184,9 +201,9 @@ impl SandboxProvider for ForkdSandboxProvider { // then report. Without initialize() the VM never exists and all // subsequent operations on the returned SandboxInfo would fail. sandbox.initialize().await?; - let id = sandbox.sandbox_id().ok_or_else(|| { - crate::Error::message("forkd sandbox id missing after initialize") - })?; + let id = sandbox + .sandbox_id() + .ok_or_else(|| crate::Error::message("forkd sandbox id missing after initialize"))?; Ok(details::forkd::forkd_info_from_name(id)) } @@ -209,9 +226,16 @@ impl SandboxProvider for ForkdSandboxProvider { return Ok(()); } Ok(resp) if resp.status().is_success() => return Ok(()), - Ok(resp) if Self::is_retryable_status(resp.status()) && attempt < PROVIDER_RETRY_LIMIT => { + Ok(resp) + if Self::is_retryable_status(resp.status()) + && attempt < PROVIDER_RETRY_LIMIT => + { let status = resp.status(); - tracing::warn!(attempt, status = status.as_u16(), "forkd delete transient error; retrying"); + tracing::warn!( + attempt, + status = status.as_u16(), + "forkd delete transient error; retrying" + ); attempt += 1; sleep(backoff).await; backoff = (backoff * 2).min(Duration::from_secs(10)); @@ -230,7 +254,10 @@ impl SandboxProvider for ForkdSandboxProvider { backoff = (backoff * 2).min(Duration::from_secs(10)); } Err(e) => { - return Err(crate::Error::context(format!("Failed to delete forkd VM '{id}'"), e)); + return Err(crate::Error::context( + format!("Failed to delete forkd VM '{id}'"), + e, + )); } } } diff --git a/lib/crates/fabro-sandbox/src/sandbox_spec.rs b/lib/crates/fabro-sandbox/src/sandbox_spec.rs index 0b929bc078..a5df47ec0e 100644 --- a/lib/crates/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/crates/fabro-sandbox/src/sandbox_spec.rs @@ -314,7 +314,7 @@ impl SandboxSpec { } } -#[cfg(any(feature = "docker", feature = "daytona"))] +#[cfg(any(feature = "docker", feature = "daytona", feature = "forkd"))] fn runtime_layout_metadata( repo_cloned: Option, clone_origin_url: Option<&str>, diff --git a/lib/crates/fabro-server/src/server/handler/sandbox.rs b/lib/crates/fabro-server/src/server/handler/sandbox.rs index d5fb1bd0c9..70c06d540a 100644 --- a/lib/crates/fabro-server/src/server/handler/sandbox.rs +++ b/lib/crates/fabro-server/src/server/handler/sandbox.rs @@ -105,13 +105,33 @@ async fn retrieve_run_sandbox( Ok(id) => id, Err(response) => return response, }; + // Resolve the terminal flag from the run's persisted status before + // loading the sandbox record. A terminal run does NOT imply the forkd + // microVM is gone (e.g. --preserve-sandbox keeps it alive), so this flag + // only tells forkd_details to query the controller for real liveness; + // Deleted is reported solely on a controller-confirmed teardown. + let is_run_terminal = match state.stores.runs.open_run_reader(&id).await { + Ok(reader) => match reader.state().await { + Ok(projection) => projection.is_terminal(), + Err(_) => false, + }, + Err(_) => false, + }; let record = match load_run_sandbox_instance(&state, &id).await { Ok(record) => record, Err(response) => return response, }; let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); let daytona_organization_id = state.config_env_lookup(EnvVars::DAYTONA_ORGANIZATION_ID); - match sandbox_details(&record, daytona_api_key, daytona_organization_id, Some(id)).await { + match sandbox_details( + &record, + daytona_api_key, + daytona_organization_id, + Some(id), + is_run_terminal, + ) + .await + { Ok(details) => Json::(details).into_response(), Err(err) => { let detail = format!("{err:#}");