refactor(controller): collapse fork reconcile status round-trips off the hot path

[stubbi]

Thinking Path

• Mitos boots Firecracker microVMs and forks them via copy-on-write snapshots, exposed through CRDs; the hosted fork runs through the husk-pod reconcile in internal/controller/sandboxfork_controller.go.
• The measured prod fork is noisy (432 to 551 ms), yet an in-process fork-bench shows the fork ENGINE is only ~75 ms, so most of the wall clock is orchestration and client RTT, not the engine.
• The husk fork reconcile persisted ForkSnapshotTaken in its OWN r.Status().Update before the child loop, on every path. Each status write is an apiserver plus etcd round-trip the client waits on, and this one records only intermediate progress.
• On the NEW-POD path that write is redundant: no child is activated in the snapshot pass, so the flag can ride the unconditional pass-boundary write the pass already makes. On the CO-LOCATION path the child is activated in the SAME pass before the per-spawn write commits, so the flag must stay durable before that first activation, else a crash could re-snapshot under an already-restored child and split a multi-replica fork point.
• This pull request folds the standalone write into the pass-boundary write on the new-pod path only, and keeps it on the co-location path exactly where it was, made conditional on the pass actually co-locating a child.
• The benefit is one fewer apiserver round-trip (and one fewer self-triggered reconcile) on the new-pod snapshot pass, with the crash-coherence invariant preserved and no change to the husk RPC calls, fork correctness, or isolation.

Linked Issues or Issue Description

No tracked issue. Problem: the husk fork reconcile persisted ForkSnapshotTaken in its own r.Status().Update before the child loop on every path, adding an apiserver plus etcd round-trip the fork client waits on purely to record intermediate progress. On the new-pod path that write is redundant with the pass-boundary write.

What Changed

• reconcileHuskFork: the standalone ForkSnapshotTaken write is no longer unconditional. It is folded into the pass-boundary write on the new-pod path (where the child pods are only CREATED in the snapshot pass and activated a later pass), and KEPT on the co-location path (where the child is ACTIVATED in the same pass before its per-spawn write commits, so the flag must be durable first). The write now fires only when spawnInSourcePod && coLocationBudgetRemaining > 0, preserving the exact pre-change ordering on that path.
• Documented the load-bearing reason inline so a future edit does not re-collapse the co-location write.
• Added an in-package unit test (fork_reconcile_roundtrips_test.go) driving reconcileHuskFork through a status-write counting client:
  • TestHuskForkNewPodPathFoldsSnapshotWrite: the new-pod snapshot pass writes status EXACTLY ONCE (was twice), the flag and timing anchors stay durable, the snapshot is taken once, and the fork reaches Ready on the next pass.
  • TestHuskForkCoLocatedSnapshotDurableBeforeActivation: ForkSnapshotTaken is durable in the store at the FIRST co-located spawn-vm activation (the crash-coherence guard), and the fork reaches Ready.

Verification

• go build ./...
• go test ./internal/controller/ (full envtest suite green on the pre-image; the two new tests pass; setup-envtest 1.31)
• TestHuskForkNewPodPathFoldsSnapshotWrite and TestHuskForkCoLocatedSnapshotDurableBeforeActivation pass
• golangci-lint run --timeout=5m ./internal/controller/ clean
• GOOS=linux golangci-lint run --timeout=5m ./internal/controller/ clean

Risks

Low risk. The change is confined to K8s status bookkeeping; it does not touch the husk RPC calls (ForkSnapshot / SpawnVM / Activate), fork correctness, isolation, or the leak-prevention paths. The co-location (hot) path keeps its pre-activation flag write, so its crash coherence is byte-for-byte the pre-change behavior; the only behavioral change is on the new-pod path, where no child is activated in the snapshot pass so the deferred flag is always durable before any later-pass activation. Status.Phase transitions and observed generation semantics for watchers are unchanged; fewer status writes reduce, not increase, conflict pressure. The saving is one round-trip on the new-pod snapshot pass (not the co-located hot path), so no wall-clock number is claimed; the operator measures on prod.

Model Used

Claude Opus 4.8 (claude-opus-4-8), extended thinking. Agent-assisted implementation and verification.

Checklist

• PR title is a conventional commit (refactor; note: pr-policy allows feat, fix, docs, ci, chore, refactor, test only, so refactor is used rather than perf)
• Thinking Path traces from project context to this change
• Model Used is filled in (with version and capability details)
• Tests added for behavior changes, in the same commit (TDD)
• Docs updated in the same PR (no doc surface changed; internal bookkeeping only)
• Threat-model delta (docs/threat-model.md) included if the security surface moved (security surface unchanged)
• Benchmark run (bench/) included if the hot path was touched (saving is one round-trip on the non-hot new-pod path; no bench number claimed)
• No em or en dashes introduced anywhere
• Secret values never logged, in errors, in condition messages, or on host paths
• No internal/instance-local references (only public #NNN / mitos-run/mitos URLs)
• Every commit carries a Signed-off-by trailer (git commit -s)

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR changes status write behavior in reconcileHuskFork by removing an immediate Status().Update call after setting ForkSnapshotTaken, relying instead on later status writes for durability, and adds a test verifying exactly two status round-trips on the happy path.

Changes

Fork Reconcile Status Round-trip

Layer / File(s)	Summary
Remove standalone status write internal/controller/sandboxfork_controller.go	Removes the immediate Status().Update call after setting ForkSnapshotTaken=true, relying on later status writes within the same reconcile pass, with documented durability guarantees for new-pod and co-location paths.
Status round-trip test internal/controller/fork_reconcile_roundtrips_test.go	Adds a counting status writer wrapper and a test asserting fork readiness, one recorded child, persisted ForkSnapshotTaken/timing fields, single snapshot/spawn calls, and exactly two Status().Update round-trips.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test as TestHuskForkHappyPathStatusRoundtrips
  participant Reconciler as SandboxReconciler
  participant Client as countingStatusWriter

  Test->>Reconciler: reconcileHuskFork(fork)
  Reconciler->>Reconciler: forkSnapshot() stub called
  Reconciler->>Reconciler: set ForkSnapshotTaken=true (in-memory)
  Reconciler->>Reconciler: spawnVM() stub called
  Reconciler->>Client: Status().Update (record child)
  Reconciler->>Client: Status().Update (pass boundary, persists ForkSnapshotTaken)
  Test->>Client: assert exactly 2 Status().Update calls
  Test->>Reconciler: assert Ready, ForkSnapshotTaken, timing fields

Loading

Possibly related PRs

• mitos-run/mitos#803: Both PRs modify reconcileHuskFork's status write behavior for ForkSnapshotTaken persistence timing relative to child updates.
• mitos-run/mitos#804: Both PRs modify reconcileHuskFork in sandboxfork_controller.go, intersecting on the co-located child path status persistence logic.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, conventional, and accurately highlights the fork reconcile status round-trip refactor.
Description check	✅ Passed	The description follows the template with Thinking Path, issue context, changes, verification, risks, model, and checklist entries.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/fork-reconcile-fewer-roundtrips

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/controller/fork_reconcile_roundtrips_test.go`:
- Around line 50-54: The delegated status update in countingStatusWriter.Update
returns the raw error from SubResourceWriter.Update without context. Update the
Update method to wrap that returned error with fmt.Errorf using a descriptive
message and %w, so failures from the delegated status write are easier to trace
while keeping the existing counting logic intact.

In `@internal/controller/sandboxfork_controller.go`:
- Around line 759-785: `ForkSnapshotTaken` is not guaranteed durable before
co-located child activation, because `spawnForkChildInSourcePod` can activate a
child before the per-spawn `Status().Update` commits. Update the
`SandboxForkController` flow so the flag is persisted before the first
co-located spawn/activation, or refactor `spawnForkChildInSourcePod` into a
pre-activation step plus a durable status write before activation proceeds. Keep
the existing invariant around `ForkSnapshotTaken` and ensure the first
co-located child cannot become active until that flag is confirmed written.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 89914299-d5a8-46fe-9ee9-ff0af46f37e6

📥 Commits

Reviewing files that changed from the base of the PR and between 9952a9c and c88d7b2.

📒 Files selected for processing (2)

• internal/controller/fork_reconcile_roundtrips_test.go
• internal/controller/sandboxfork_controller.go

[stubbi]

Closing: CodeRabbit's Critical finding is correct and decisive. The ForkSnapshotTaken standalone Status().Update is load-bearing on the CO-LOCATION path (the prod path): spawnForkChildInSourcePod activates the child BEFORE the per-spawn status write at line 938, so removing the standalone durable write opens a crash window where an activated child exists while ForkSnapshotTaken is not durable, letting a later pass re-snapshot/re-fork the source and split the fork point. Making it safe means keeping that durable write, which reverts the optimization. The saving was one round-trip (~5-15ms), which the PR itself noted is within the fork's 432-551ms prod variance. So: the reconcile status round-trips are load-bearing, there is no safe wall-clock win here, and correctness wins. Recorded as a finding: the prod fork's orchestration cost is real and necessary, not fat to trim.

[stubbi]

Superseded by #859. GitHub's PR head pointer for this PR got stuck on an old commit (c88d7b2) after a force-push while the branch ref correctly advanced, so CI stopped re-triggering here and the PR could not be reopened. #859 tracks the same branch (with the CodeRabbit co-location crash-coherence fix applied) from a clean head. Closing this in favor of #859.