Repository navigation
fix: switch to official SmolVM retained forks - #121
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4efc7f2ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } else { | ||
| let uses_packed_artifact = direct_create_from_packed; | ||
| let uses_packed_artifact = config.use_packed_artifact; |
There was a problem hiding this comment.
When rearm_fork_base returns Ok(false) because a live clone still depends on the golden—or when deleting the partial clone fails—this now creates the replacement from the same packed .smolmachine payload. The previous branch deliberately switched to environment.base in these cases because mixing a live fork with another --from creation has produced ESTALE in both machines; retained RAM checkpoints do not remove the storage dependency that makes re-arming unsafe. Keep the independent OCI-image fallback for these two paths so a spent golden cannot corrupt an active job or repeatedly stall the pool.
Useful? React with 👍 / 👎.
| pub fn from_environment(binary: impl Into<PathBuf>) -> Self { | ||
| Self::new(binary) | ||
| .with_retained_fork_checkpoints(env_flag_default_true("PRELOOP_SMOLVM_RETAINED_FORKS")) | ||
| .with_pack_network( | ||
| first_nonempty_env(&[ | ||
| "PRELOOP_RUNNER_PACK_PROXY", | ||
| "HTTPS_PROXY", | ||
| "https_proxy", | ||
| "HTTP_PROXY", | ||
| "http_proxy", | ||
| ]), | ||
| first_nonempty_env(&["PRELOOP_RUNNER_PACK_NO_PROXY", "NO_PROXY", "no_proxy"]), | ||
| ) | ||
| Self::new(binary).with_pack_network( |
There was a problem hiding this comment.
On installations that still resolve an older official SmolVM and previously set PRELOOP_SMOLVM_RETAINED_FORKS=false, from_environment now ignores that supported compatibility setting and unconditionally permits another live fork. Those releases do not retain a reusable checkpoint, so the second call reaches SmolVM against the paused base and can invalidate the first clone's storage. This also affects upgrades performed outside preloop update, and preloop update itself only warns and continues when runtime installation fails; retain the single-live-clone guard or reject incompatible runtime versions before enabling the pool.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Confidence score: 2/5
- In
crates/preloop-orchestrator/src/lib.rs, removing theForkBaseBusypath anddirect_create_from_packed = falsefallback can leave fork-base creation without a safe recovery path when re-arming/deletion preconditions fail, which risks fork setup failures for active clones; restore or replace that branch with an explicit degraded-mode fallback and add coverage for the busy-base scenario.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/preloop-orchestrator/src/lib.rs">
<violation number="1" location="crates/preloop-orchestrator/src/lib.rs:3090">
P1: Removing the ForkBaseBusy branch and the `direct_create_from_packed = false` assignments means that when a fork base can't be safely re-armed (a live clone still depends on it, or the partial clone can't be deleted), the code now falls through to `uses_packed_artifact = config.use_packed_artifact` and creates the replacement from the same packed payload instead of falling back to the job's OCI image (`environment.base`). Mixing a live fork with another `--from` creation against the same packed payload has been documented to produce ESTALE on both machines, so this fallback should still use the independent OCI image in these unsafe-rearm cases rather than reusing the packed artifact.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } else { | ||
| let uses_packed_artifact = direct_create_from_packed; | ||
| let uses_packed_artifact = config.use_packed_artifact; |
There was a problem hiding this comment.
P1: Removing the ForkBaseBusy branch and the direct_create_from_packed = false assignments means that when a fork base can't be safely re-armed (a live clone still depends on it, or the partial clone can't be deleted), the code now falls through to uses_packed_artifact = config.use_packed_artifact and creates the replacement from the same packed payload instead of falling back to the job's OCI image (environment.base). Mixing a live fork with another --from creation against the same packed payload has been documented to produce ESTALE on both machines, so this fallback should still use the independent OCI image in these unsafe-rearm cases rather than reusing the packed artifact.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/preloop-orchestrator/src/lib.rs, line 3090:
<comment>Removing the ForkBaseBusy branch and the `direct_create_from_packed = false` assignments means that when a fork base can't be safely re-armed (a live clone still depends on it, or the partial clone can't be deleted), the code now falls through to `uses_packed_artifact = config.use_packed_artifact` and creates the replacement from the same packed payload instead of falling back to the job's OCI image (`environment.base`). Mixing a live fork with another `--from` creation against the same packed payload has been documented to produce ESTALE on both machines, so this fallback should still use the independent OCI image in these unsafe-rearm cases rather than reusing the packed artifact.</comment>
<file context>
@@ -3108,13 +3087,11 @@ async fn provision_runner<P: VmProvider + 'static>(
}
} else {
- let uses_packed_artifact = direct_create_from_packed;
+ let uses_packed_artifact = config.use_packed_artifact;
let spec = MachineSpec {
name: name.clone(),
</file context>
Address review findings on the retained-fork runtime switch: - Probe the resolved smolvm's --version once per provider and keep the single-live-clone guard only for runtimes that predate retained checkpoints (< 1.7.7); the capability is verified against the actual binary instead of assumed from the pinned install or an env flag. - Keep the orchestrator's independent-OCI fallback for unsafe fork-base re-arms (live clone still depends on the golden, or the partial clone cannot be removed): a spent golden must not be re-created from the same packed payload beside a live fork (ESTALE risk). - Stop advertising Intel macOS in the smolvm platform matrix: official releases ship no darwin-x86_64 artifact, so preloop update must not claim a runtime it cannot install. - Remove the temporary PRELOOP_SMOLVM_RETAINED_FORKS and PRELOOP_SMOLVM_RELEASE_REPOSITORY/PRELOOP_SMOLVM_RELEASE_VERSION overrides; delete the obsolete internal runtime doc.
c4efc7f to
4e28890
Compare
Address review findings on the retained-fork runtime switch: - Probe the resolved smolvm's --version once per provider and keep the single-live-clone guard only for runtimes that predate retained checkpoints (< 1.7.7); the capability is verified against the actual binary instead of assumed from the pinned install or an env flag. - Keep the orchestrator's independent-OCI fallback for unsafe fork-base re-arms (live clone still depends on the golden, or the partial clone cannot be removed): a spent golden must not be re-created from the same packed payload beside a live fork (ESTALE risk). - Stop advertising Intel macOS in the smolvm platform matrix: official releases ship no darwin-x86_64 artifact, so preloop update must not claim a runtime it cannot install. - Remove the temporary PRELOOP_SMOLVM_RETAINED_FORKS and PRELOOP_SMOLVM_RELEASE_REPOSITORY/PRELOOP_SMOLVM_RELEASE_VERSION overrides; delete the obsolete internal runtime doc.
Address review findings on the retained-fork runtime switch: - Probe the resolved smolvm's --version once per provider and keep the single-live-clone guard only for runtimes that predate retained checkpoints (< 1.7.7); the capability is verified against the actual binary instead of assumed from the pinned install or an env flag. - Keep the orchestrator's independent-OCI fallback for unsafe fork-base re-arms (live clone still depends on the golden, or the partial clone cannot be removed): a spent golden must not be re-created from the same packed payload beside a live fork (ESTALE risk). - Stop advertising Intel macOS in the smolvm platform matrix: official releases ship no darwin-x86_64 artifact, so preloop update must not claim a runtime it cannot install. - Remove the temporary PRELOOP_SMOLVM_RETAINED_FORKS and PRELOOP_SMOLVM_RELEASE_REPOSITORY/PRELOOP_SMOLVM_RELEASE_VERSION overrides; delete the obsolete internal runtime doc.
What & why
Protocol surface
Required gates
just test-cipasses locally (fmt-check + clippy-D+ full test suite + runner-watch conformance)runner-watch, or a live capture showing the official bytes), not only unit testsconcurrency_properties, scheduling, matrix expansion, …): they are extended for the changed contract and pass (PROPTEST_CASES=256 cargo test -p preloop-runner-server)Verification performed
Checklist
docs/,CONTRIBUTING.md) where behavior changedSummary by cubic
Switches to the official
smol-machines/smolvmv1.7.7 and gates retained-fork behavior by the resolvedsmolvmbinary’s version. Old behavior: an env flag enabled multiple live forks; new behavior: probesmolvm --versiononce per provider — <1.7.7 keeps the single-live-clone guard, ≥1.7.7 allows multiple live clones while still serializing forks per golden — preventing fork-base corruption on older or unprobeable installs.smol-machines/smolvm@v1.7.7;preloop update --ensure-runtimeinstalls this exact version and verifies--mount-socket; removesPRELOOP_SMOLVM_RELEASE_REPOSITORY/PRELOOP_SMOLVM_RELEASE_VERSION.PRELOOP_SMOLVM_RETAINED_FORKS; provider caches a version probe (fail-closed) and rejects a second live fork only when retained checkpoints are absent.darwin-x86_64in the install matrix; Intel macOS hosts won’t claim an installable runtime.Written for commit d8f8d68. Summary will update on new commits.