Add provider-neutral sandbox execution with sbx and Islo backends - #22637
Add provider-neutral sandbox execution with sbx and Islo backends#22637zozo123 wants to merge 32 commits into
Conversation
Introduce a hardened sandbox lifecycle contract and an initial Docker Sandboxes backend so untrusted commands can run in disposable isolation while future providers share the same Prefect API. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d8e470b4c
ℹ️ 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".
desertaxle
left a comment
There was a problem hiding this comment.
Thanks for putting this together @zozo123! I’m supportive of sandbox execution in Prefect, but I don’t think pre-built tasks are the correct Prefect concept for this integration. Before adding public APIs, we need to decide what a sandbox represents in Prefect. I think the design discussion should compare:
- A worker/work-pool type and infrastructure decorator if the sandbox owns an entire flow run.
- A task runner if the sandbox determines where submitted Prefect tasks execute.
- A new durable
SandboxRun-like concept if it executes arbitrary untrusted code while keeping the Prefect runtime and credentials outside the guest.
Whichever concept we choose needs to define the unit of isolation, durable identity, input and result transfer, logs, cancellation, retries, credentials, and orphan cleanup. Those lifecycle behaviors are where Prefect can provide differentiated value; run_in_sandbox tasks alone do not.
Can we move this back to the issue for a focused design discussion before committing to a design? The existing sbx implementation may still be useful as the first provider adapter once we agree on the Prefect concept.
Implement the sandbox contract a second time against a hosted REST provider so the lifecycle is demonstrably vendor-neutral: an API key is exchanged host-side for a short-lived session token, commands stream over SSE with output capped as it arrives, files upload natively, egress is denied at provisioning time, and a provider-side reaper deletes sandboxes a killed worker never tore down. A cross-provider conformance suite runs the same assertions against both backends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prebuilt tasks commit to a public API before the design discussion on PrefectHQ#22636 has settled what a sandbox is in Prefect, so remove them and leave the provider-neutral backend contract, its two adapters, and the imperative SandboxOperation escape hatch. Documentation now reaches the same outcomes through SandboxOperation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@desertaxle acted on the review rather than arguing it: the four prebuilt tasks are gone, and the concept comparison you asked for is on the issue (#22636 (comment)) with all eight lifecycle behaviors filled in. Short version — the credential boundary is what makes a sandbox a sandbox, a worker-submitted flow run needs This PR stays a draft until you weigh in. What's left in it is the layer you said was useful: the |
The unreachable-endpoint test compared the error message against a hardcoded URL, which CodeQL reads as incomplete URL substring sanitization. Reading the constant is both quieter and one less literal to drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A token whose payload cannot be read, an error body that is not the provider's JSON shape, and an empty output event once the cap is full were all reachable and untested. Covering them takes the backend to 99 percent and pins the cap arithmetic, which is easy to get wrong once there is no room left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With egress="deny" an OSError launching the policy subprocess lands after sbx create has already succeeded, and _acli reports it as SandboxUnavailableError, so keying cleanup on the exception type concluded nothing existed and leaked a live microVM still holding the egress the caller asked to block. Cleanup now tracks whether the CLI was launched. The staging file for sbx cp also cleans up after a failed write, which previously stranded a partial file on a disk that was already full. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
prefect-sandbox was the only one of the nineteen in-tree integrations without one, so its sdist relied on setuptools defaults instead of the shared include and exclude rules. The file is byte-identical to its siblings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
awrite_file creates the parent directory with aexec, which raises rather than returning a nonzero exit when the sandbox itself is gone, so a failed write surfaced a message about running a command and named neither the path nor the write. Found by pointing the backend at the real API with a sandbox name it had never seen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A with block inside async code closes on Prefect's sync loop while the thread owning the provisioning loop is blocked, so cancellation of that provisioning could only be scheduled, never awaited, and a sandbox already created was abandoned. Joining the foreign task would deadlock unconditionally, so the handle is published as soon as acreate returns and the closer destroys it from its own loop instead. aclose's docstring claimed the guarantee it did not provide, and its INFO line reported closing zero sandboxes while one was live. Also drops a guard in the Islo stream reader that became unreachable when the reader started breaking at the exit event, and rewrites two tests that could not fail: one exercised that dead guard, the other asserted URL normalization that httpx performs regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Source permalinks across all four pages pointed at pre-fix line numbers, and prefect_sandbox-operations.mdx still published the cleanup guarantee aclose no longer claims. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review Since your first pass the branch has changed substantially, and most of it you have not seen:
Please pay particular attention to |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 155e20cb78
ℹ️ 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".
|
@desertaxle The implementation side is ready for your design call now:
I have left the PR in draft because the maintainer-confirmation checklist is still genuinely open. Could you confirm whether this narrowed adapter-first scope is acceptable, or whether you want the backend contract split out / a short design doc before it is marked ready? |
|
Revalidated after the host upgrade and marking this ready for the focused design re-review:
This does not mark the design checkbox approved; it means the narrowed adapter-first implementation and the #22636 comparison are ready for that decision. Re-requesting @desertaxle. |
|
@codex review Please review the current head ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6be77489ef
ℹ️ 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".
|
@codex review Please review exact head |
|
@codex review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df98dd2df2
ℹ️ 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".
|
@codex review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 555adc7a37
ℹ️ 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".
|
@codex review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76b0fccbf0
ℹ️ 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".
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Please review exact head This head adds explicit pickle/cloudpickle stripping for all ephemeral Islo session and API-key cache fields. It retains stable, API-key-independent handle signing, and the regression suite now has 558 passing tests at 96% coverage. The real Islo proof created with key A, serialized without key A bytes, then unpickled and re-authenticated with key B to upload, execute, and destroy the original sandbox; cleanup left zero test resources. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Please review exact head All prior inline threads are resolved. This head closes the last one by making sbx host-key staging cleanup span |
|
@desertaxle Re-review requested on exact head The branch now implements the concrete changes from your requested-design review:
All automated threads through the latest review are resolved. The commits after the original request are implementation hardening only and do not broaden the public concept. Please clear the existing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 880cae83d7
ℹ️ 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".
|
@codex review Please review exact head All prior inline threads are resolved. This head addresses the latest three findings with: 32 KiB argv chunks plus an executed 256 KiB ceiling regression; cancellation-safe joining and deletion of sbx host staging; and POSIX directory fsync before a published or newly observed handle key is trusted. Full validation is 564 tests, 96% statement coverage, all static/build/pre-commit checks, and a fresh-home real sbx demo with zero sandbox residue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a813a25fe
ℹ️ 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".
|
@codex review Please review exact head All prior threads are resolved. This head adds one immutable cross-thread Islo session snapshot, fsyncs a new sbx key directory in its parent before publishing the key, stages UTF-8 bytes in binary mode, and reports every staged-file deletion failure except an already-absent path. Deterministic race and cleanup regressions pass. Full validation: 567 tests, 96% statement coverage, all static/build/pre-commit checks, and a real Islo A→B pickle/upload/exec/destroy proof with zero resources. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Ready for focused design review against #22636. @desertaxle asked to settle what a sandbox represents in Prefect before any public API lands, so the prebuilt task API was removed and this branch now carries only the provider-neutral backend layer, its two adapters, and an explicitly provisional imperative operation.
prefect-sandboxintegration with a provider-neutral lifecycle for creating, executing in, writing to, and destroying disposable sandboxessbx) backend with isolated per-run state, explicit egress-policy verification, bounded output, host-enforced timeouts, and failure-visible cleanupTwo transports with nothing in common — a local CLI and a hosted REST API — implementing the same contract is what makes the abstraction credible. Neither backend's shape leaks into the other.
Changed since the first review
Both automated review findings on
sbx.pyfrom the first pass are fixed, each with a regression test that fails if the fix is reverted:P1,
sbx.pyacreate— withegress="deny", anOSErrorlaunching the policy subprocess happens aftersbx createhas succeeded, and_aclireports it asSandboxUnavailableError. The cleanup branch keyed on that exception type, concluded nothing had been provisioned, removed only the workspace, and leaked a live microVM — still holding exactly the egress the caller asked to block. Cleanup now tracks whether the CLI was launched rather than inferring it from the exception type. The same bug class was found and fixed inIsloSandboxindependently.P2,
_write_host_temp_file— a write that died aftermkstempnever returned the path, soawrite_file'sfinallycould not unlink it, stranding a partial file on the worker exactly when the disk was already full. The helper now cleans up after itself.P2,
operations.pyaclose— awithblock inside async code closes on Prefect's sync loop while the thread owning the provisioning loop is blocked, so cancelling that provisioning could only be scheduled, never awaited, and a sandbox already created was abandoned. Joining the foreign task is not an option — it deadlocks unconditionally, which I verified — so the handle is now published as soon asacreatereturns and the closing loop destroys it itself.aclose's docstring had claimed the guarantee it did not provide, and its INFO line reported closing zero sandboxes while one was live; both are fixed and the api-ref is regenerated.the four prebuilt tasks in
tasks.pyare removed. They committed to a public API ahead of the concept decision;SandboxOperationremains as the imperative escape hatch and is explicitly provisional.design comparison of the three concepts @desertaxle named, with all eight lifecycle behaviors filled in, posted at Add a provider-neutral sandbox execution integration #22636 (comment)
Latest review fixes and runnable demo
Current head
9995253closes every automated review and security finding, with a regression test for each:max_output_bytes; the incremental decoder retains a fixed framing budget, marks the retained output truncated, discards the rest of that event, and still parses the following exit event.SandboxOperation.aclose()now shields the complete captured cleanup generation, so cancellation is re-delivered only after every moved-out process has been visited.api_urlrecorded inSandbox.metadata, including from a differently configured block instance.creating. Create now receives the configured provisioning budget, and failed-create cleanup polls and retries deletion through that transition.examples/hello_sandbox.pyruns the same Prefect flow and uploaded Python file with eithersbxor Islo. The README and integration guide contain the two commands.handle_signing_key, independent of the provider API key. Prefect persists its generated value with a saved block; independently constructed workers can share it throughPREFECT_SANDBOX_ISLO_HANDLE_SIGNING_KEY. Missing or altered metadata is rejected before any credential or request is sent, while rotating the Islo API key leaves live handles usable for teardown.SecretStridentity is compared in constant time, and the complete snapshot is explicitly removed from pickle/cloudpickle payloads so process workers re-authenticate and no raw provider key crosses the serialization boundary.sbxexec, copy, and destroy requires a host-signed HMAC binding the sandbox id and canonical workspace. The 0600 signing key is atomically published outside the guest mount; unsigned or altered handles are rejected before any targeting subprocess.Fresh validation on current head: 567 tests passed with 96% statement coverage; mypy, Ruff, formatting, interrogate (100%), pre-commit, and package build passed. The committed demo also passed against both real providers: Docker Sandboxes v0.37.0 printed guest kernel
7.0.12; Islo printed6.16.9+; both exited 0 and destroyed their sandbox. Deeper live matrices on both providers verified large-file fidelity, no ambient Prefect/AWS credentials, explicit env only, denied egress, exactly 1024/200000 output bytes retained, nonzero exits as data, timeout destruction, idempotent teardown, and zero test resources afterward.Deliberately not in this PR
Two registration points that other integrations have are left out on purpose, because both reference a package that is not on PyPI yet. Flagging them rather than guessing at the convention:
prefect[sandbox]extra in the rootpyproject.toml, alongsidedocker = ["prefect-docker>=0.6.0"]andshell = [...]. Adding it before the first release would makepip install prefect[sandbox]resolve to nothing.IsloSandboxblock logo is currentlyhttps://avatars.githubusercontent.com/islo-labs, a third-party-controlled avatar endpoint that whoever owns that org can change. Every other in-tree block uses a Prefect-hosted CDN asset; happy to swap in whatever you upload.src/prefect/server/collection_blocks_data.json, the whitelist that shows a collection's blocks in the UI catalog before it is installed. An entry here would advertiseSbx SandboxandIslo Sandboxto users who cannot install them yet. Its entries carry no version or checksum, so it can be added whenever you prefer.Happy to add either now if you would rather they land with the code. I checked the other places a new integration might need registering and they need nothing: the test matrix is generated from changed paths by
scripts/generate_integration_package_tests_matrix.py(which is why this PR already ran the suite on 3.10 through 3.13), anddependabot.yml,CODEOWNERSandlabeler.ymldo not enumerate integrations.Also proposed on the issue but deliberately not built, since they add public API ahead of the concept decision:
aread_file(the contract is write-only today) and avalidate_envdenylist that makes invariant 7 enforced rather than documented.Test plan
uv run coverage run --branch -m pytest -q && uv run coverage report— 567 passed, 96% statement coverageuv run ruff format --check src/integrations/prefect-sandboxuv run ruff check src/integrations/prefect-sandboxuv run mypy prefect_sandboxuv run interrogate prefect_sandbox --config pyproject.toml— 100%docs.jsonresolves, no broken internal links in the changed pages, nothing references the removed api-ref page (mint broken-linksitself does not run on Node 25+, so this was checked directly)full mutation coverage — reverting invariant 5 in
sbx.pyfrom cap-while-streaming to buffer-then-trim, for instance,is not currently caught, because the distinguishing property is memory rather than output
The Islo backend's tests use
httpx.MockTransport, so no network access and no credentials are needed in CI.Validated against the real providers, outside CI.
SbxSandboxwas run end to end against a real Docker Sandboxes microVM on macOS (sbxv0.37.0, revalidated after upgrade), and all checks passed:Linux 7.0.12 aarch64, host macOSPREFECT_API_KEYandAWS_SECRET_ACCESS_KEYboth absent in the guest; the caller'senvpresentawrite_filefidelitytruncatedsettimed_outandsandbox_terminated, and the microVM was confirmed gone from the hostegress="deny"BLOCKEDprefect-sandbox-*left behindIsloSandboxwas then run end to end against the real hosted API onapi.islo.dev, and every check passed:POST /auth/tokenin 0.5 s; the raw API key is not accepted as a bearer token, which is the one part of this backend that cannot be inferred from the public OpenAPI documentacreateSandbox.metadataaexecPREFECT_API_KEYabsent in the guest, the caller'senvpresentawrite_filetruncatedsettimed_outandsandbox_terminatedset, and the sandbox really destroyedadestroyidempotent;asessiondestroys, after which exec correctly failsadestroyon a never-existent name succeeded (invariant 3, live);/exec/streamand/filesagainst an unknown sandbox both surfacedSANDBOX_NOT_FOUNDlegiblyChecklist
closes.Closes #22636
AI assistance was used to draft and review portions of the implementation. The contributor reviewed the full diff and ran the checks listed above locally.