Require containers 0.2.1 for sleepAfter streams#544
Conversation
Update sandbox to require the containers release that tracks in-flight proxy work for sleepAfter. Add an E2E regression so long-running quiet execStream calls do not get terminated while the container is still active.
🦋 Changeset detectedLatest commit: fc11058 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
commit: |
🐳 Docker Images Published
Usage: FROM cloudflare/sandbox:0.0.0-pr-544-fc11058Version: 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-544-fc11058 /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 23808170391 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-544-fc11058 cat /container-server/sandbox > sandbox && chmod +x sandbox |
whoiskatrin
requested review from
aron-cf,
ghostwriternr and
scuffi
as code owners
|
This is primarily a comment regarding the original implementation on the containers package, but it looks like we're currently firing alarms in a loop every 50ms. In our test, a 5-second stream with 3s sleepAfter produced 13 alarms in ~400ms (the window between sleepAfter expiring and the stream ending. If a user runs a long running command that streams output for 10 minutes with sleepAfter: '30s', there will be 9.5 minutes of alarms in a 50ms loop, which will be in the thousands but all doing nothing useful and charging the user for every invocation. We should maybe add a more graceful 1-second sleep between checks? cc @gabivlj sample logs |
ghostwriternr
left a comment
ghostwriternr
left a comment
There was a problem hiding this comment.
Change LGTM. Non-blocking, I think we should also have another complementary test that creates a sandbox with a short sleepAfter like 3s, runs a quick command, waits 4+ seconds, and asserts the sandbox was stopped. That way the we always prove both sides: sleepAfter kills idle containers, and the inflightRequests check from the containers package prevents it from killing active ones.
Switch the sandbox dependency to the released containers package and update the pending changeset text to match the minimum supported version.
3 participants
Summary
@cloudflare/containers@^0.2.1, which keepssleepAfterfrom expiring while proxied request or stream work is still in flightsleepAfterto expire normally once the sandbox is actually idleexecStream()must survive pastsleepAfter, and an idle sandbox with shortsleepAftermust eventually stopTesting
npm run typecheckReviewer Notes
containerFetch()/fetch(), so@cloudflare/containers@0.2.1is the right upstream fix for the original customer reportSANDBOX_TRANSPORT=websocket; after the containers fix, that persistent proxy connection counts as in-flight activity and can keep the sandbox artificially alivepackage-lock.jsonincludes npm metadata churn, but the functional dependency change is the bump to@cloudflare/containers@0.2.1