Skip to content

Conversation

@whoiskatrin
Copy link
Collaborator

@whoiskatrin whoiskatrin commented Oct 23, 2025

keepAlive flag that prevents containers from sleeping while processes are running. If used this would require calling destroy() manually afterwards

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2025

🦋 Changeset detected

Latest commit: f042419

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/sandbox Patch

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

@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@cloudflare cloudflare deleted a comment from claude bot Oct 23, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 23, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@137

commit: f042419

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2025

🐳 Docker Image Published

FROM cloudflare/sandbox:0.0.0-pr-137-8c80b12

Version: 0.0.0-pr-137-8c80b12

You can use this Docker image with the preview package from this PR.

@whoiskatrin whoiskatrin marked this pull request as ready for review October 24, 2025 10:07
@whoiskatrin whoiskatrin force-pushed the keep-container-alive-during-the-process branch from e325f76 to 42b76c7 Compare October 26, 2025 11:49
@whoiskatrin whoiskatrin changed the title add auto timeout renewal and some health checks add keepAlive flag to prevent container from shutting down for long processes Oct 28, 2025
Copy link
Member

@ghostwriternr ghostwriternr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, and I like the DX. Added some minor comments to fix.

Copy link
Member

@ghostwriternr ghostwriternr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also pls address e2e failures

@claude
Copy link

claude bot commented Oct 28, 2025

Claude Code Review

Summary: Implementation looks solid. The keepAlive feature is well-designed with proper override pattern and comprehensive test coverage.

Issues Found

1. Missing activity timeout renewal (Critical)

The onActivityExpired() override prevents shutdown but doesn't renew the activity timeout when keepAlive is enabled. This means the container will repeatedly trigger onActivityExpired() after each timeout period.

Solution: In onActivityExpired() when keepAlive is true, call this.ctx.renewActivityTimeout() or similar mechanism to reset the activity timer.

2. Type safety issue in test mock (packages/sandbox/tests/sandbox.test.ts:51)

blockConcurrencyWhile: vi.fn().mockImplementation(<T>(callback: () => Promise<T>): Promise<T> => callback()),

The generic constraint is incomplete - blockConcurrencyWhile can accept non-Promise callbacks. Should be:

blockConcurrencyWhile: vi.fn().mockImplementation(<T>(callback: () => T | Promise<T>): Promise<T> => Promise.resolve(callback())),

3. Incomplete workflow file pattern (line 11)

- '**'
- '\!**/*.md'

This will trigger on ALL file changes then exclude markdown, which negates the previous exclusions. Either remove the - '**' line or restructure to only include relevant paths.

Minor observations

  • Documentation: Consider adding a warning in JSDoc that keepAlive requires manual cleanup to prevent resource leaks (already present, good)
  • E2E tests: Excellent coverage including edge cases like intermittent output, high volume streams, and explicit destroy
  • Import ordering: Consistent fix applied (packages/sandbox/src/request-handler.ts:1)

Architecture alignment

✅ Follows established patterns:

  • Uses RPC method setKeepAlive() like existing setSleepAfter()
  • Properly overrides lifecycle hook onActivityExpired()
  • Adds option to SandboxOptions with clear documentation
  • Test coverage matches project standards (unit + E2E)

The implementation is clean and well-integrated. Fix the activity renewal and you're good to merge.

@ghostwriternr ghostwriternr merged commit 7f4442b into main Oct 28, 2025
10 checks passed
@ghostwriternr ghostwriternr deleted the keep-container-alive-during-the-process branch October 28, 2025 15:28
@threepointone threepointone mentioned this pull request Oct 28, 2025
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.

2 participants