Skip to content

fix(envd): validate keepalive intervals before creating tickers - #3651

Open
zegging wants to merge 2 commits into
e2b-dev:mainfrom
zegging:fix/envd-keepalive-interval
Open

zegging wants to merge 2 commits into
e2b-dev:mainfrom
zegging:fix/envd-keepalive-interval

Conversation

@zegging

@zegging zegging commented Sep 20, 2026

Copy link
Copy Markdown

Why

Closes #3650.

An invalid Keepalive-Ping-Interval can panic in time.NewTicker. In the process streaming paths this terminates envd, interrupting command streams and losing its in-memory process tracking. It does not imply that the whole sandbox VM exits.

This was found while developing a Java client that calls the API directly, rather than through the official SDKs. The normal official JavaScript/TypeScript and Python command paths send a fixed positive interval, so this is a lower-priority input-robustness fix, not a claim that typical SDK usage is affected.

What Changed

  • Validate a parsed interval before converting seconds to time.Duration: it must be positive and representable without overflow. Otherwise, keep the existing 90-second fallback used for missing or non-numeric values.
  • Add envd regression tests for invalid values, both signs of duration overflow, valid intervals, the maximum whole-second interval, and ticker reset behavior.
  • Add an E2E scenario that starts a background command, submits another command with a zero interval, and checks completion, retained process tracking, reconnection, and both command streams. A valid-interval case provides a control; each case owns its sandbox.
  • Bump envd to 0.9.1 as required for a behavioral change.

The two commits deliberately separate the regression tests from the fix.

Verification

The new envd regression tests were run with -race against both revisions. Before the fix, zero, negative, and overflowing intervals fail: three cases panic, and positive wraparound produces a roughly 290 ms interval. After the fix, all cases pass, including reset behavior and the maximum valid interval.

The E2E test compiles, but the updated scenario has not been validated against rebuilt envd end to end: the local automated runner was blocked before executing that scenario. This PR remains a draft pending that verification; it does not claim E2E green.

The full envd suite is also not fully validated locally: TestIsPathOnNetworkMount_FuseMount and TestCreateWatcherOnNetworkMount fail at the FUSE mount setup, and tests requiring root are skipped. These environment limitations have not been worked around by changing or disabling tests.

@cla-bot

cla-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @zegging on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check'

@zegging
zegging marked this pull request as ready for review September 20, 2026 09:46
@zegging

zegging commented Sep 20, 2026

Copy link
Copy Markdown
Author

@cla-bot check

@cla-bot cla-bot Bot added the cla-signed label Sep 20, 2026
@cla-bot

cla-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown

The cla-bot has been summoned, and re-checked this pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Invalid keepalive interval can crash envd and lose running-process tracking

1 participant