Skip to content

test(sandbox/forkd): pin provider retry, fail-fast, env, mapping, and delete behavior (ao-company #125) - #40

Open
zenprocess wants to merge 1 commit into
mainfrom
fix/forkd-provider-tests-rebased
Open

test(sandbox/forkd): pin provider retry, fail-fast, env, mapping, and delete behavior (ao-company #125)#40
zenprocess wants to merge 1 commit into
mainfrom
fix/forkd-provider-tests-rebased

Conversation

@zenprocess

Copy link
Copy Markdown
Owner

Closes ao-company fabro-sh#125.

This PR supersedes PR #39, which the coach sent back because it was
written against the pre-reorg `lib/crates/fabro-sandbox` tree
(merge-base de3349d, 293 commits behind origin/main). This branch
is rebased on origin/main (HEAD ed3a1e4) and targets the new
`lib/components/fabro-sandbox/src/provider/forkd.rs`. The diff is
exactly the same test additions, retargeted.

Acceptance command (verbatim)

```bash
cargo nextest run -p fabro-sandbox --features forkd
```

The issue's literal `cargo test -p fabro-sandbox` cannot exercise
the forkd provider because the provider module is gated on
`#[cfg(feature = "forkd")]` and the crate's default features are
only `["local"]`. The above command mirrors the workflow used by
the existing `forkd::tests` module in
`lib/components/fabro-sandbox/src/forkd/mod.rs`.

base_rc ≠ 0 (falsification at origin/main)

```
$ git show origin/main:lib/components/fabro-sandbox/src/provider/forkd.rs | wc -l
265
$ git show origin/main:lib/components/fabro-sandbox/src/provider/forkd.rs | grep -c '#[test]|#[tokio::test]'
0
rc=1
```
Same 265-line file, 0 tests — issue admitted at the correct path.

head_rc = 0 (this PR)

```
$ grep -c '#[test]|#[tokio::test]' lib/components/fabro-sandbox/src/provider/forkd.rs
16
$ cargo nextest run -p fabro-sandbox --features forkd
...
PASS provider::forkd::tests::list_returns_sandbox_infos_on_200
PASS provider::forkd::tests::list_retries_5xx_then_succeeds_with_exponential_backoff
PASS provider::forkd::tests::list_succeeds_when_first_response_is_200
PASS provider::forkd::tests::list_5xx_until_retry_limit_yields_typed_error
PASS provider::forkd::tests::list_connect_error_after_retries_yields_typed_error
PASS provider::forkd::tests::list_401_fails_fast_without_retries
PASS provider::forkd::tests::get_returns_none_on_404
PASS provider::forkd::tests::get_returns_info_on_200
PASS provider::forkd::tests::create_rejects_non_forkd_spec_with_typed_error
PASS provider::forkd::tests::create_reports_server_assigned_id_and_provider_kind
PASS provider::forkd::tests::delete_200_returns_ok
PASS provider::forkd::tests::delete_404_is_treated_as_idempotent_success
PASS provider::forkd::tests::delete_401_fails_fast_without_retries
PASS provider::forkd::tests::from_env_uses_defaults_when_no_overrides_set
PASS provider::forkd::tests::from_env_honors_forkd_url_token_snapshot_tag
PASS provider::forkd::tests::from_env_only_snapshot_tag_overrides_default
Summary 16 tests run: 16 passed
```

Whole crate: 124/124 pass on this Mac in 3.0s (budget < 120s).

Commit-litmus (acceptance detects its subject)

With `PROVIDER_RETRY_LIMIT = 0` locally:
```
FAIL provider::forkd::tests::list_retries_5xx_then_succeeds_with_exponential_backoff
panicked: 3rd attempt must succeed: Message("forkd list VMs returned 500 Internal Server Error: ")
FAIL provider::forkd::tests::list_5xx_until_retry_limit_yields_typed_error
panicked: httpmock assert_calls_async(4) mismatch (1 observed)
Summary 16 tests run: 14 passed, 2 failed
```
After revert (`PROVIDER_RETRY_LIMIT = 3`): 16/16 pass.

Diff stat (vs origin/main)

```
lib/components/fabro-sandbox/src/provider/forkd.rs | 465 +++++++++++++++++++++
1 file changed, 465 insertions(+)
```

No production code touched. `httpmock` was already declared as a
dev-dependency in `lib/components/fabro-sandbox/Cargo.toml` —
no new deps, no default-feature toggle, no `Cargo.lock` change.

Coverage map (issue spec → test)

  • retry success w/ backoff → `list_retries_5xx_then_succeeds_with_exponential_backoff`
  • retry exhaustion (typed error) → `list_5xx_until_retry_limit_yields_typed_error`
  • non-retryable 4xx fail-fast → `list_401_fails_fast_without_retries`,
    `delete_401_fails_fast_without_retries`
  • create → SandboxInfo mapping w/ server-assigned id → `create_reports_server_assigned_id_and_provider_kind`
  • create rejects non-Forkd spec → `create_rejects_non_forkd_spec_with_typed_error`
  • env resolution (defaults + overrides) → `from_env_uses_defaults_when_no_overrides_set`,
    `from_env_honors_forkd_url_token_snapshot_tag`, `from_env_only_snapshot_tag_overrides_default`
  • get() 404 → Ok(None), 200 → Ok(Some) → `get_returns_none_on_404`, `get_returns_info_on_200`
  • delete() 404 idempotent → `delete_404_is_treated_as_idempotent_success`
  • delete() 200 OK → `delete_200_returns_ok`
  • connect-refused retry → typed error → `list_connect_error_after_retries_yields_typed_error`

Dependency exclusion (per brief)

The issue lists a dependency on the liveness-query behavior introduced
by `7a008fb9e fix(sandbox/forkd): query controller for real liveness
instead of assuming Deleted`. Verified locally — that commit is NOT
on origin/main:
```
$ git log origin/main | grep 7a008fb
(no output)
```
Per the brief, this PR explicitly excludes the liveness-query test
and the existing tests in `lib/components/fabro-sandbox/src/forkd/mod.rs`
already cover that path once it merges.

Hermetic guarantees

  • localhost only (httpmock + scripted TcpListener on 127.0.0.1)
  • no dellsrv / no real forkd controller
  • dummy bearer strings only (`forkd-test-token`, `forkd-local-token`)
  • no Infisical values, no real secrets
  • 4xx/5xx responses synthesized in-process
  • connect-refused test binds and immediately drops a TcpListener

🤖 Generated with Claude Code

… delete behavior

Hermetic unit tests for provider/forkd.rs (was 265 lines, 0 tests).
The forkd provider handles transient HTTP retries (5xx/connect),
non-retryable 4xx (401) fail-fast, env resolution from
FORKD_URL/FORKD_TOKEN/FORKD_SNAPSHOT_TAG, create() mapping, and
idempotent delete(). Pinned by 16 hermetic tests against a localhost
httpmock + scripted TcpListener — no network beyond the loopback,
no real forkd controller, no real tokens.

The retry/backoff semantics test (list_retries_5xx_then_succeeds)
FAILS when PROVIDER_RETRY_LIMIT is set to 0; with the limit restored
all 16 tests pass in 2.0s on this Mac (acceptance budget < 120s).

Excludes the liveness-query behavior added in 7a008fb because
that commit is not yet on origin/main — once it merges, the
existing forkd/mod.rs tests already cover that path.

Refs: ao-company fabro-sh#125
@zenprocess

Copy link
Copy Markdown
Owner Author

COACH INDEPENDENT VERIFICATION — re-ran by the orchestrator, not the author.

Scope check: 1 file, +465/-0, at lib/components/fabro-sandbox/src/provider/forkd.rs, merge-base exactly current with origin/main (0 commits behind). The 10-file/-129 sprawl on the closed PR #39 was pre-reorg noise, as expected. Good.

A CORRECTION THAT IS MINE, NOT YOURS: my dispatch brief named the acceptance command as

cargo nextest run -p fabro-sandbox

That command is WRONG for this crate and I ran it first. Result: 87 tests, 87 passed, exit 0 — and not a single provider::forkd test among them. provider/forkd.rs sits behind #[cfg(feature = "forkd")], a non-default feature, so the new tests were never compiled. A clean green that proved nothing — the manufactured-green trap wearing a different hat. Your original command carried --features forkd and was right; I overrode it with a worse one. Noting it because the issue body ALSO specifies a featureless command (cargo test -p fabro-sandbox), so that acceptance criterion is itself unsound and should be corrected there.

CORRECTED RUN — cargo nextest run -p fabro-sandbox --features forkd:

124 tests run: 124 passed (4 slow), 0 skipped, exit 0
16 provider::forkd::tests present by name

Coverage matches the spec: retry-then-succeed with exponential backoff, retry exhaustion yielding a typed error, connect-error exhaustion, 401 fail-fast without retries on both list and delete, 404 delete treated as idempotent success, create rejecting a non-forkd spec with a typed error, create reporting server-assigned id and provider kind, get 200/404 mapping, and from_env across defaults, full overrides, and snapshot-tag-only override.

COMMIT-LITMUS — run by me, in an isolated detached worktree outside the repo directory, not taken on trust:

mutated const PROVIDER_RETRY_LIMIT: u32 = 3  ->  0
result: 103/124 run, 101 passed, 2 FAILED
  FAIL provider::forkd::tests::list_5xx_until_retry_limit_yields_typed_error
  FAIL provider::forkd::tests::list_retries_5xx_then_succeeds_with_exponential_backoff

The acceptance detects its subject. Worktree removed and the checkout returned to clean afterwards.

base_rc: grep -c on origin/main at lib/components/.../provider/forkd.rs = 0 across 265 lines, exit 1.
head_rc: 0.

Verdict: verified. Not merging unilaterally — reporting to the operator with PRs fabro-sh#282 and fabro-sh#283.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant