feat(orch): live-upgrade envd inside a running sandbox at resume#3333
feat(orch): live-upgrade envd inside a running sandbox at resume#3333kalyazin wants to merge 6 commits into
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit 7b66d5c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
1e16ee2 to
ac7daef
Compare
ac7daef to
426efb4
Compare
426efb4 to
c2a93df
Compare
db05515 to
75e66d0
Compare
75e66d0 to
1fa7e12
Compare
1fa7e12 to
1fc12d7
Compare
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
1fc12d7 to
43c51ac
Compare
43c51ac to
16adb2a
Compare
16adb2a to
0db8e4b
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if resumeHandover { | ||
| func() { | ||
| // A panic here must not crash the new envd: systemd would restart a | ||
| // fresh (old-binary) envd with a new PID that can neither re-adopt nor | ||
| // reap the frozen workload. Recover and keep serving; the workload is | ||
| // thawed by ResumeFromHandover's deferred unfreeze regardless. | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| fmt.Fprintf(os.Stderr, "resume-from-handover panic (recovered): %v\n", r) | ||
| processService.UnfreezeWorkload() | ||
| } | ||
| }() | ||
|
|
||
| watchersJSON, err := processService.ResumeFromHandover() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "resume-from-handover failed: %v\n", err) | ||
| } | ||
| // Re-arm filesystem watchers carried in the handover. | ||
| filesystemService.ImportWatchers(watchersJSON) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🟡 In the incoming live-upgrade handover path, ResumeFromHandover() thaws the workload via a top-level defer s.UnfreezeWorkload() that fires when the function returns — but main.go only calls filesystemService.ImportWatchers(watchersJSON) after that return, so the workload is unfrozen and free to mutate the filesystem for a brief window before the new fsnotify watches are re-armed. This contradicts ImportWatchers' own comment that 'the resume freeze makes this lossless,' and can silently drop filesystem-watcher events in that gap. Fix by re-arming watchers (or moving the thaw) before the deferred unfreeze fires.
Extended reasoning...
ResumeFromHandover in packages/envd/internal/services/process/upgrade.go opens with:
func (s *Service) ResumeFromHandover() ([]byte, error) {
// Always thaw the workload before returning, on ANY path — bad blob, partial
// re-adopt, or panic. A failed handover must never leave the sandbox frozen...
defer s.UnfreezeWorkload()
...
return st.Watchers, nil
}A deferred call runs at the moment the enclosing function returns — i.e. UnfreezeWorkload() executes as part of the return st.Watchers, nil statement completing, strictly before control passes back to the caller in main.go. The caller is:
watchersJSON, err := processService.ResumeFromHandover()
if err != nil {
fmt.Fprintf(os.Stderr, "resume-from-handover failed: %v\n", err)
}
// Re-arm filesystem watchers carried in the handover.
filesystemService.ImportWatchers(watchersJSON)So the sequence at runtime is: (1) workload cgroups thaw, (2) ResumeFromHandover returns the watcher blob, (3) only then does ImportWatchers call CreateFileWatcher for each carried watcher, which is what actually invokes fsnotify's w.Add() and creates a live kernel watch.
The old fsnotify inotify fd does not survive the handover — it's opened with IN_CLOEXEC and is closed by the outgoing envd's execve. So between step (1) and step (3) there is a real (if small — a couple of consecutive statements, scheduler-dependent) window where the just-thawed customer workload is runnable and can freely create/write/delete files, but no filesystem watch of any kind exists in the new envd process. Any event in that gap is not queued anywhere and is permanently lost — there's no buffering or replay for it, unlike the terminal-event retention cache built elsewhere in this same PR for exit codes.
This directly contradicts the guarantee documented on the code this PR adds, ImportWatchers' own comment in watch_handover.go:
"The resume freeze makes this lossless — nothing mutates the filesystem during the handover window."
The freeze is held correctly for the entire outgoing side (via the new FreezeWorkloadHold/WorkloadFreezer machinery this PR introduces) and is held right up until ResumeFromHandover returns. But nothing in the incoming path defers the thaw until watchers are actually re-armed — the ordering of "thaw" and "re-arm" is simply backwards relative to the stated invariant.
Step-by-step to trigger:
- A sandbox has an active client-created fsnotify watcher on some directory (via the filesystem service's
CreateWatcher). - A resume-time envd live-upgrade fires (flag-gated, e.g.
envd-upgrade-target=promoted). - The outgoing envd freezes the workload, serializes the watcher (
ExportWatchers), and execve's into the new binary. - The new envd calls
ResumeFromHandover(), which re-adopts processes and, on return, itsdeferthaws the workload cgroups. - The workload's own threads (which are not blocked on anything envd-controlled) resume running immediately and can write/create/delete a file in the watched directory before
main.go's next statement,ImportWatchers(watchersJSON), executes. ImportWatchersthen callsCreateFileWatcher, which only starts watching current state going forward — the mutation from step 5 is never observed by any watcher, silently dropped.
Fix: re-arm the filesystem watchers before the workload is thawed — e.g. pass the watcher blob into ResumeFromHandover (or have it return before the deferred unfreeze runs) so ImportWatchers executes while the workload is still frozen, then thaw only after that completes. Alternatively, have the outgoing side keep the workload frozen (via the already-carried FreezeHold release) until the incoming side confirms watchers are re-armed.
This is a narrow window (two back-to-back statements), fsnotify delivery is already best-effort elsewhere (events can be dropped on internal queue overflow), and the whole live-upgrade feature is gated off by default (envd-upgrade-target=off), so it isn't a main-path or blocking issue. But it is a genuine, easily fixable correctness gap against an invariant this same PR explicitly documents.
There was a problem hiding this comment.
Fixed. ResumeFromHandover now takes a reArmWatchers func([]byte) callback and invokes it on the success path before its deferred UnfreezeWorkload fires, so filesystem watches are re-armed while the workload is still frozen — no gap in which an event can be dropped. main.go passes filesystemService.ImportWatchers as the callback (dropping the old post-return call). The always-thaw safety is unchanged (the deferred thaw still runs on error/panic). Covered by TestResumeFromHandover_ReArmsWatchersBeforeThaw, which asserts the workload is still frozen at callback time.
|
|
||
| go func() { | ||
| outWg.Wait() | ||
| close(outMultiplex.Source) | ||
| outCancel() | ||
| }() | ||
|
|
||
| h.readoptTimeout = args.Timeout |
There was a problem hiding this comment.
🟡 In Readopt() (packages/envd/internal/services/process/handler/readopt.go:94-101), a re-adopted process's remaining timeout is stored only in h.readoptTimeout; the struct literal never sets h.deadline, so Handler.Deadline() always reports "no deadline" for a re-adopted handler. Since Upgrade() carries the remaining timeout forward exclusively via Deadline(), a process with a finite timeout that survives a second chained live-upgrade loses its timeout and runs unbounded (until the sandbox itself is torn down).
Extended reasoning...
The bug. Handler tracks a process's timeout two different ways depending on how the handler was created:
- A freshly-started process (
New()) captures its absolute deadline fromctx.Deadline()intoh.deadline. - A re-adopted process (
Readopt(), used after a live-upgrade handover) only setsh.readoptTimeout, a relative duration used solely to re-arm a local kill-timer viaBeginReaping().
Handler.Deadline() is deadline-based: it returns (time.Time{}, false) whenever p.deadline.IsZero(). Since Readopt() never populates h.deadline, Deadline() unconditionally reports "no deadline" for any re-adopted handler — even though readoptTimeout is nonzero and a kill-timer is correctly running in-process.
Where it bites. Upgrade() (the outgoing side of the next live-upgrade) is the only place that serializes a process's remaining timeout into the handover blob, and it does so exclusively through h.Deadline():
if d, ok := h.Deadline(); ok {
... hp.TimeoutMs = rem
}It never consults readoptTimeout. So the first upgrade of a New()-started process works fine (deadline is set, gets carried, Readopt() on the far side re-arms the kill-timer from the carried Timeout). But that re-adopted handler's own Deadline() is now permanently false. If a second chained upgrade happens while that same process is still running, Upgrade() sees Deadline()==false, leaves hp.TimeoutMs at 0, and the next Readopt() receives Timeout: 0 — BeginReaping()'s if p.readoptTimeout > 0 guard skips arming a kill-timer entirely. The process's original timeout is silently and permanently dropped.
Why nothing else catches this. The whole point of carrying TimeoutMs through the handover is to preserve request-level deadlines across a same-PID envd swap; there's no independent check elsewhere that a re-adopted process still has an active timeout, since the two fields (deadline for serialization, readoptTimeout for re-arming) were meant to be kept in sync but aren't.
Step-by-step proof:
- A client starts a process with
Connect-Timeout-Ms: 60000on envd v1.handler.New()setsh.deadline = now + 60s. - Sandbox pauses/resumes; orchestrator triggers a live-upgrade to envd v2 while the process is still running.
Upgrade()callsh.Deadline()→(now+60s, true), carriesTimeoutMs ≈ remaining. On v2,Readopt()builds the new handler withh.readoptTimeout = remaining;h.deadlineis left zero.BeginReaping()arms a kill-timer fromreadoptTimeout— timeout still enforced. - Before that timer fires, a second live-upgrade to envd v3 is triggered (e.g. another resume).
Upgrade()on v2 callsh.Deadline()on the still-running re-adopted handler →(zero, false)(deadline was never set in step 2).hp.TimeoutMsstays0. - On v3,
Readopt()receivesTimeout: 0;BeginReaping()'sreadoptTimeout > 0check fails, so no kill-timer is armed. The process — originally bounded to 60s — now runs with no timeout at all until the sandbox itself is stopped.
Fix. Readopt() should also set h.deadline = time.Now().Add(args.Timeout) when args.Timeout > 0, mirroring New(), so the deadline round-trips through any number of chained upgrades instead of only surviving one hop.
Severity. This is a real, PR-introduced correctness regression, but the trigger is narrow: it requires (a) a process started with an explicit finite Connect-Timeout-Ms, and (b) that process surviving two or more chained live-upgrades before it would otherwise exit — two separate pause/resume cycles each landing a strictly newer envd, on a flag-gated, off-by-default feature. There's no crash or data loss, just a process that outlives its intended deadline; per the review rubric this doesn't rise to a concrete user-facing failure that should block merge, so it's a nit rather than blocking.
There was a problem hiding this comment.
Fixed. BeginReaping now sets p.deadline = time.Now().Add(p.readoptTimeout) when it arms the re-adopt timeout timer, so Deadline() reports it for a re-adopted handler. Since Upgrade carries the remaining timeout forward across a (further, chained) upgrade exclusively via Deadline(), this stops a timed process that survives a second handover from running unbounded. Covered by TestBeginReaping_ArmsDeadlineForChainedUpgrade.
| // argv: keep original flags, append --resume-handover. | ||
| argv := append([]string{newBin}, os.Args[1:]...) | ||
| argv = append(argv, "--resume-handover") | ||
|
|
||
| // Carry the dup'd fds across execve. In production this runs under | ||
| // syscall.ForkLock with spawners quiesced; the caller has | ||
| // already stopped envd's port watcher. | ||
| syscall.ForkLock.Lock() | ||
| err = syscall.Exec(newBin, argv, os.Environ()) | ||
| syscall.ForkLock.Unlock() | ||
|
|
||
| return fmt.Errorf("execve %s: %w", newBin, err) |
There was a problem hiding this comment.
🟡 In Service.Upgrade (packages/envd/internal/services/process/upgrade.go), if the final syscall.Exec fails (e.g. a corrupt/missing staged envd.next binary), the fds already relocated via dup3 into the dupped slice are never closed before returning the error — unlike the sibling collision path a few lines above, which explicitly closes them. Because these fds have CLOEXEC cleared, they leak into the still-running old envd and get inherited by every subsequent process envd spawns via exec.Command. Fix is a one-line mirror of the existing dupErr cleanup.
Extended reasoning...
The bug. In Upgrade, each live process'''s stdout/stderr/stdin/tty fds are relocated with dup3(old, target, 0) (flags=0 clears CLOEXEC so they survive the upcoming execve) and every successfully relocated target fd is appended to the local dupped slice. If a fd-relocation collision occurs (dupErr != nil), the code explicitly closes everything already in dupped before returning — proving the author was aware these are extra fd-table entries that must be cleaned up on any abort. But after that block, if the terminal step — syscall.Exec(newBin, argv, os.Environ()) — fails, the function does return fmt.Errorf(\"execve %s: %w\", newBin, err) with no equivalent cleanup of dupped.
Why it'''s reachable. syscall.Exec only fails to replace the process image; it does not free anything it already touched. It can fail with ENOENT/EACCES/ENOEXEC if the delivered /usr/bin/envd.next is missing, non-executable, or a truncated/corrupt ELF — a real possibility since that file is written straight from the /upgrade request body. main.go'''s doUpgrade treats a failed Upgrade as recoverable by design: it releases the freeze hold and unfreezes the workload, keeping the old envd process alive and serving. That old process is exactly the one holding the leaked fds.
Why nothing else closes them. The Handler still owns and uses the original pipe/PTY fds for its own I/O; the dupped targets are extra duplicate references that exist purely for the handover and have no other owner once the execve that was supposed to consume them fails. Nothing else in the surrounding code (or in doUpgrade'''s failure path) touches these fd numbers.
Downstream effect. Because dup3 was called with flags=0, CLOEXEC is cleared on the targets. os/exec (used by envd to spawn every subsequent workload process) does not proactively close arbitrary pre-existing open fds — it relies on CLOEXEC — so these leaked fds get silently inherited by every process envd spawns after a failed upgrade attempt, in addition to just sitting open in envd itself.
Step-by-step proof:
- A sandbox has one live process; the orchestrator triggers a resume-time envd upgrade with a corrupted
envd.nextstaged. Upgradewalkss.processes, dup3'''s the process'''s stdout/stderr/stdin/tty fds onto fixed targets (e.g. 200-203), and appends each successfully-duplicated fd todupped. No collision occurs, so this block returns normally without closing anything (that'''s expected — they'''re meant to survive into the new image).- The handover blob is written, then
syscall.Exec(\"/usr/bin/envd.next\", ...)is called. Because the binary is corrupt, the kernel returnsENOEXECandsyscall.Execreturns an error instead of replacing the process. Upgradereturnsfmt.Errorf(\"execve %s: %w\", newBin, err)—dupped(fds 200-203) is never closed.doUpgradein main.go sees the error, releases the freeze hold, unfreezes the workload, and keeps running — the OLD envd process, still holding 4 extra open fds it no longer needs.- If the orchestrator retries the upgrade on a later resume (still hitting the same bad target, or the sandbox keeps getting the manual SIGUSR2 dev trigger), each attempt leaks 4 more fds. Meanwhile, any new process this envd spawns via
exec.Commandinherits copies of the earlier leaked pipe/PTY fds because CLOEXEC was cleared on them.
Fix. Mirror the existing dupErr cleanup: on the final syscall.Exec failure, iterate dupped and unix.Close each fd before returning the error, exactly as the collision branch already does a few lines above.
Why nit rather than blocking. This only manifests on a failed upgrade attempt (bad/corrupt staged binary or repeated manual triggers) — the happy path never returns from Upgrade (execve replaces the process), so normal rollouts never hit this. The consequence (a handful of leaked fds per failed attempt) is real but requires repeated failures to approach fd-table exhaustion, and the fix is a trivial one-line mirror of code that already exists in the same function.
There was a problem hiding this comment.
Fixed. On syscall.Exec failure the relocated fds in dupped (CLOEXEC cleared) are now closed before returning the error — a one-line mirror of the existing dupErr cleanup — so they no longer leak into the still-running old envd or its subsequently-spawned children.
| if err := sbx.CallEnvdUpgrade(upCtx, path, "/usr/bin/envd.next", deliverTimeout); err != nil { | ||
| result = "delivery_failed" | ||
| span.RecordError(err) | ||
| sbxlogger.I(sbx).Error(upCtx, "envd auto-upgrade: trigger failed", zap.Error(err)) | ||
|
|
||
| return false | ||
| } | ||
| // WaitForEnvd re-runs /init, which re-captures the now-running version. | ||
| if err := sbx.WaitForEnvd(upCtx, sandbox.StartTypeResume, readyTimeout); err != nil { | ||
| result = "not_ready" | ||
| span.RecordError(err) | ||
| sbxlogger.I(sbx).Error(upCtx, "envd auto-upgrade: envd not ready after upgrade", zap.Error(err)) | ||
|
|
There was a problem hiding this comment.
🟡 maybeUpgradeEnvd's post-upgrade readiness check re-invokes sbx.WaitForEnvd on the same sandbox handler, which re-fires its unconditional deferred SetStartedAt(time.Now()) and re-records the orchestrator.sandbox.envd.init.duration histogram + envd-init call counter (both labeled start_type=resume) a second time. This overwrites the sandbox's correct StartedAt with a later timestamp (visible via Server.List's RunningSandbox.StartTime and in Pause/Delete execution_time) and double-counts/skews the resume-latency KPI for every successfully upgraded resume. Fix by guarding SetStartedAt and the duration/counter recording the same way the adjacent UFFD startup-stats block already guards itself with startupStatsOnce for exactly this 'later WaitForEnvd on the same handler' scenario.
Extended reasoning...
What happens: WaitForEnvd (packages/orchestrator/pkg/sandbox/sandbox.go, ~line 1906-1984) has a deferred block that, whenever it returns nil, unconditionally calls s.SetStartedAt(time.Now()), and its body unconditionally records waitForEnvdDurationHistogram (orchestrator.sandbox.envd.init.duration, labeled start_type) plus increments envdInitCalls via initEnvd (guarded only by s.skipStartupMetrics, which is never set for a real resumed sandbox). The normal resume path already calls WaitForEnvd exactly once (ResumeSandbox, sandbox.go:1141), which correctly sets StartedAt to the moment the sandbox actually became ready and records one legitimate resume-latency sample.
maybeUpgradeEnvd (packages/orchestrator/pkg/server/sandboxes.go:1264-1276) runs right after that normal resume, and on a successful envd live-upgrade it calls sbx.WaitForEnvd(upCtx, sandbox.StartTypeResume, readyTimeout) a second time on the same sandbox handler, purely to re-check readiness and re-read the version off /init. Because this call also returns nil, its deferred block re-fires: StartedAt gets overwritten with a later timestamp that includes the upgrade's delivery + trigger + readiness time, and the resume-latency histogram/counter get a second, spurious start_type=resume sample/increment.
Why nothing catches this: the adjacent UFFD startup-stats block in the same deferred section (sandbox.go ~1934-1941) is explicitly wrapped in s.startupStatsOnce, and the comment right there calls out precisely this scenario — 'a later WaitForEnvd on the same handler' (referencing the envd-binary-swap case this PR introduces). That shows the authors were aware per-start side effects must fire once per real start, but the guard was applied only to the UFFD stats; SetStartedAt and the duration histogram/init-call counter were left unguarded, so this second call still double-fires them.
Impact: StartedAt corruption is externally visible — Server.List() returns it as RunningSandbox.StartTime, and getSandboxExecutionData (used by Pause/Delete kill events) computes execution_time as time.Since(startedAt) against the corrupted, later value, understating the sandbox's true uptime for the rest of its life. Separately, every successfully upgraded resume adds an extra sample to orchestrator.sandbox.envd.init.duration{start_type=resume} and an extra envdInitCalls increment, even though the upgrade already has its own dedicated orchestrator.envd.upgrade.duration/.attempts metrics — this skews the very resume-latency KPI dashboard the rollout depends on for judging the upgrade's safety.
Step-by-step proof: (1) sandbox resumes normally; ResumeSandbox calls WaitForEnvd once, which sets StartedAt = T0 (the true resume-ready time) and records one init.duration sample. (2) Create then calls maybeUpgradeEnvd; the flag/gate pass, CallEnvdUpgrade delivers+triggers the swap successfully. (3) maybeUpgradeEnvd calls sbx.WaitForEnvd(upCtx, StartTypeResume, readyTimeout) again; envd is not restarting (its process image was replaced in-place via the same-PID exec, the sandbox itself never rebooted), so this call is purely a post-upgrade re-check. (4) It succeeds (nil error) at time T1 = T0 + deliverTime + triggerTime + readyTime (documented ~300ms-2s typical, up to ~45s worst case). (5) The deferred block fires again: StartedAt is overwritten from T0 to T1, and init.duration{start_type=resume} gets a second sample plus a second envdInitCalls increment. (6) Any later Server.List() or a subsequent Pause/Delete now reports StartTime = T1 and computes execution_time relative to T1 instead of the true T0, and the Prometheus dashboard counts two resume-init events for what was really one resume.
Fix: apply the same kind of once-guard used for the UFFD stats — e.g. skip SetStartedAt/the duration-histogram-and-counter recording on this specific re-check call (a parameter or a dedicated lighter-weight readiness-only path), or reuse startupStatsOnce-style idempotency scoped to the whole per-resume side-effect set rather than just the UFFD portion.
This is the same root cause underlying two originally-reported findings (bug_002: StartedAt overwrite: bug_005: metrics double-recording) merged here since both stem from the same extra WaitForEnvd call; all five independent verifier passes confirmed the code-level mechanics and converged on nit severity, since the magnitude is a small, bounded timestamp/metric skew (no crash, hang, or data loss), the feature is gated behind a LaunchDarkly flag defaulting to off, and it only manifests on the subset of resumes where a live envd upgrade actually succeeds.
There was a problem hiding this comment.
Fixed. WaitForEnvd now determines "first start for this handler" once via an atomic.Bool CAS (startupRecorded, replacing the narrower startupStatsOnce) and gates ALL first-start recording on it: the envd-init duration + uffd.startup.* histograms, SetStartedAt, and — by threading the flag into initEnvd — the envdInitCalls counter. A later WaitForEnvd on the same handler (the post-upgrade readiness re-check, or the template-build swap) still re-runs /init to re-capture the live version but records nothing and no longer overwrites StartedAt.
0db8e4b to
c21705a
Compare
This comment has been minimized.
This comment has been minimized.
c21705a to
a2a6960
Compare
| if _, err := os.Stat(candidate); err != nil { | ||
| return "", "" // target not staged on this node | ||
| } |
There was a problem hiding this comment.
I think it's worth, at least, logging an error here and later on. maybe as well some metric? What I'd like is that we can differentiate between "bad SHA/rubbish input in LD" vs already at newest version vs getVersion failed, etc.
There was a problem hiding this comment.
added failure reasons
| for _, e := range fw.Events { | ||
| if b, err := protojson.Marshal(e); err == nil { | ||
| evs = append(evs, b) | ||
| } |
There was a problem hiding this comment.
what if err != nil. Do we silently ignore?
There was a problem hiding this comment.
added a log message. I don't expect it to happen routinely. If it does, we should be able to find when debugging a sandbox.
| if err != nil { | ||
| failed++ | ||
| s.logger.Warn().Err(err).Str("path", wh.Path).Str("watcher_id", wh.Id).Msg("handover: re-arm watcher failed") | ||
|
|
There was a problem hiding this comment.
same-ish question as above? Do we ignore these failures? Does the upgrade continue in their presence?
There was a problem hiding this comment.
yes, if we got to this stage, it's less harmful to ignore the error than attempt to downgrade. However I realised that if we just log it in envd, we'll never be able to observe it in a meaningful way, unless we debug a specific sandbox so added this in envd init's response so it could be seen.
| // drops without a reply: a transport error after the body was sent is the | ||
| // expected success path. The caller must follow with WaitForEnvd. |
There was a problem hiding this comment.
how do we differentiate between the expected transport error and every other transport error?
| // transport error AFTER the request reached it (connection reset/EOF) is | ||
| // the expected success path. But a failure to even reach a running envd | ||
| // (connection refused, dial/deadline timeout) means the upgrade was never |
There was a problem hiding this comment.
Ok, I see this is the answer on my previous question. Are these all the possible bad failures? IOW, is there any blind spot in this protocol?
There was a problem hiding this comment.
So, in the success case, the new envd comes up and reports to us. If it doesn't happen, we know for sure it's a failure, but we may misattribute it because of lack of a low resolution.
| var opErr *net.OpError | ||
| if errors.As(err, &opErr) && opErr.Op == "dial" { | ||
| return true | ||
| } |
There was a problem hiding this comment.
please add a comment here, regarding what errors does this cover.
a2a6960 to
8bb811b
Compare
8bb811b to
9dfc80c
Compare
| s.logger.Info(). | ||
| Str("event_type", "watcher_readopted"). | ||
| Str("watcher_id", wh.Id). | ||
| Str("path", wh.Path). |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
[Privacy Guard] The new watcher handover logs include raw watcher paths (wh.Path) in success/failure events. These paths can contain customer code-data locations and are now emitted into centralized operational logs without an explicit privacy-mode-safe redaction gate.
Impact: privacy-mode sandbox path metadata can be exposed outside the sandbox runtime boundary via log pipelines.
Reviewed by Cursor Security Reviewer for commit 9dfc80c. Configure here.
9dfc80c to
6a564a4
Compare
6a564a4 to
0a1f741
Compare
MinEnvdVersionForUpgrade (0.6.11, the first envd with /upgrade + handover) and the multivariate string flag envd-upgrade-target (off | promoted | <git-sha>; fallback env-overridable via ENVD_UPGRADE_TARGET). ResolveEnvdUpgrade mirrors ResolveFirecrackerVersion and returns (path, version), "" when the target equals built-with (idempotent re-resume); getVersion is injected so this shared package stays free of an orchestrator dependency. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Upgrade the envd binary inside a running sandbox by re-exec'ing into the new binary with the same PID and re-adopting the workload, so arbitrary customer processes and their stdio/PTY survive the swap. - POST /upgrade (authenticated): the new binary is streamed in the body, written, and swapped via same-PID syscall.Exec under ForkLock with the workload frozen; on success envd never responds (it has exec'd). - Handover blob (tmpfs) carries per-process pid/tag/cgroup/config + stdio/PTY fds (CLOEXEC cleared), the terminal-event retention cache, filesystem watchers, and per-process timeouts. The incoming envd re-adopts each process (reaped via pidfd + pid-specific wait4), re-arms watchers/timeouts, restores the retention cache, and thaws the workload. - Failure handling: the upgrade never leaves the sandbox worse off. On an execve failure the outgoing envd thaws the workload it froze and keeps running the old binary; the incoming resume path always thaws via a top-level defer (success, error, or panic) and is recover-wrapped so a bad blob can't crash envd into a fresh systemd restart that orphans the workload. Bump envd to 0.6.11 (the first upgrade-aware version). Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Root-free unit tests: a Connect after a gap-exit returns the retained Start+End by pid/tag (unknown → NotFound); a watcher exported then imported keeps its id, delivers post-handover events, and preserves pending events; and ResumeFromHandover always thaws the workload — on a malformed blob (error) and on no blob — so a failed handover never leaves the sandbox frozen. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…trics
At resume, maybeUpgradeEnvd resolves a target via envd-upgrade-target, gates
on MinEnvdVersionForUpgrade, and (best-effort, recover-wrapped, bounded)
delivers the binary over the authenticated /upgrade body via
Sandbox.CallEnvdUpgrade, then WaitForEnvd. The target binary is read from the
node-local read-only /fc-envd gcsfuse mount.
Metrics: orchestrator.envd.upgrade.attempts{result,from_version,to_version},
.duration{result}, .gated{reason}; an envd.upgraded label on
orchestrator.sandbox.create.duration; and an envd-upgrade child span. The
common per-resume no-op (flag off / same version) is not counted.
Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Table-drive resolveEnvdUpgradePath (off/unset/promoted/sha, newer vs same-version, missing target) without a LaunchDarkly client, and assert the three rollout metrics have description+unit map entries and construct. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The live-upgrade handover added a second workload freeze/unfreeze path (process.Service.FreezeWorkload/UnfreezeWorkload) that swept the user/pty cgroups directly, independent of the HTTP API's existing /freeze, /unfreeze and /init-deferred-thaw. Those API paths serialize their sweep through API.freezeLock precisely so a pause freeze, a rollback unfreeze and the resume thaw can't interleave and strand the workload frozen; the upgrade path took no lock at all, so an upgrade freeze racing the resume thaw could leave the sandbox frozen. Extract the sweep and its lock into a single cgroups.WorkloadFreezer that owns the manager plus a serializing semaphore and exposes Freeze/Unfreeze (best-effort, joined errors; Unfreeze detaches from ctx cancellation so a thaw always lands). main constructs one instance and passes it to both the HTTP API and the process service, so every freeze/unfreeze caller now shares the same lock. The canonical user/pty cgroup list moves to cgroups.WorkloadProcessTypes. The handover now holds that shared lock across the WHOLE window (freeze -> serialize -> execve) via a new FreezeHold/FreezeWorkloadHold, not just the freeze sweep, so a concurrent /init or /unfreeze thaw blocks on the lock until the handover finishes and cannot thaw the workload mid-handover. No behavior change to the existing endpoints: PostFreeze still returns 503 on a cancelled request and 500 on a sweep failure, PostUnfreeze//init still attempt every cgroup best-effort. Per-cgroup error logs collapse into one joined log line per call. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0a1f741 to
7b66d5c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7b66d5c. Configure here.
| if fd >= 0 { | ||
| dupped = append(dupped, fd) | ||
| } | ||
| } |
There was a problem hiding this comment.
Chained upgrade fd collision
High Severity
After the first live upgrade, re-adopted stdio/PTY fds stay at the fixed fdBase slots via os.NewFile. A second upgrade calls dupKeep onto those same targets, so F_GETFD sees them in use (and dup3(fd,fd) would also fail). Chained upgrades therefore abort fd relocation and never swap, breaking the promoted multi-release path the PR aims to support.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 7b66d5c. Configure here.
5aad415 to
d71980e
Compare
3 participants
feat(orch): live-upgrade envd inside a running sandbox at resume
Why
envd is the in-VM daemon every sandbox depends on, but its version is baked into
the snapshot at build time. A long-lived paused sandbox therefore resumes on
whatever envd it was built with, potentially many releases behind — there is no
way to ship an envd fix or behavioral change to the existing fleet of snapshots
short of rebuilding every template. We want to roll a new envd to running
sandboxes the same way we roll Firecracker versions: gated by a LaunchDarkly
flag, ramped by cohort, and without disturbing the customer's workload.
The hard part is doing it without a visible interruption: the swap has to keep
arbitrary customer processes — and their stdio/PTYs, timeouts, filesystem
watchers, and recently-observed exit codes — alive across the binary change.
What
Two halves: an in-guest handover mechanism in envd, and an orchestrator-side
resume-time trigger driven by a flag. They are independent — the envd half is
inert until something calls
POST /upgrade.In-guest handover (envd)
A new authenticated
POST /upgradeendpoint performs a same-PIDsyscall.Execinto a new envd binary and re-adopts the running workload, socustomer processes never notice the daemon changed underneath them.
body, written inside the guest (default
/usr/bin/envd.next), then swapped.This uses the token-authenticated
/upgradepath rather than theunauthenticated
/filescopy, which a live post-/initsandbox rejects.to a tmpfs handover blob, relocates the carried fds and
syscall.Execs underForkLock. On success it never responds — the process image is replaced. Thefreeze lock is held across the whole handover (freeze → serialize → execve,
via
FreezeHold), not just the freeze sweep, so a concurrent/initor/unfreezethaw can't slip in and unfreeze the workload mid-serialization; onexecveit evaporates with the process image, and on any failure it's releasedbefore the thaw. The port scanner is left running: holding
ForkLockacrossthe fd relocation already serializes it against the scanner's fd/socat
creation, so there's no CLOEXEC race to quiesce.
dup3'd to fixed high numbers(so the fresh runtime's early startup can't have grabbed them) under
ForkLock, and each target is checked free first — a collision aborts theupgrade (closing what it already relocated) instead of
dup3silentlyclobbering a live fd, leaving the old envd running.
/run/e2b/envd-handover.json, so itnever touches the rootfs diff): per-process pid / tag / cgroup / config, the
stdio + PTY fds (dup'd with CLOEXEC cleared so they survive execve), the
terminal-event retention cache (exit codes of processes that ended just
before the swap), the filesystem watchers, and each process's remaining
timeout.
streaming its output, and re-arms its timeout — recording a fresh deadline so a
further chained upgrade re-carries the remaining timeout. It restores the
retention cache (skipping any PID already re-adopted as live, so a reused PID
resolves to the live process, not a stale exit) and re-arms the filesystem
watchers before thawing the workload, so the workload stays frozen until
the watches are live and no event is dropped in the gap. Re-adopted processes
are reaped via pidfd + a pid-specific
wait4— neverwait4(-1), whichwould steal
os/exec's post-upgrade children — and the reaper retains aterminal event synchronously, before closing its event channel, so a late
reconnect always finds the exit in the cache rather than racing the retain.
failure the outgoing envd closes the fds it relocated (CLOEXEC-cleared — else
they'd leak into the still-running old envd and its future children), thaws the
workload it froze, and keeps running the old binary. The incoming resume path always thaws via a top-level
defer(success, error, or panic) and is recover-wrapped, so a malformed blob can't
crash envd into a systemd restart that orphans the workload. Degraded at
worst, never hung or dead.
are factored into a single
cgroups.WorkloadFreezershared by the HTTP API(
/freeze,/unfreeze, the/initdeferred thaw) and the upgrade, so theupgrade's freeze can no longer interleave with the resume thaw and strand the
workload frozen.
envd is bumped to 0.6.11, the first upgrade-aware version.
Resume-time trigger + gate (orchestrator + shared)
On resume (
Createwithsnapshot=true, and the checkpoint-resume path),maybeUpgradeEnvddecides against the live running envd version, delivers,and confirms — best-effort, recover-wrapped, and bounded so it can never disrupt
resume.
X-Envd-Versionresponse header on/init; the orchestrator captures it offthe resume-path
/initit already makes (no extra round-trip) and keys thedecision, gate,
from_version, and success check on it. The templatebuilt-with never changes across a live upgrade, so keying on it would
re-trigger the handover on every resume.
ResolveEnvdUpgrade(shared, the resumeanalog of
ResolveFirecrackerVersion) reads theenvd-upgrade-targetflag andreturns the target binary's local path + version, or
""when the target isnot strictly newer than the live version — so an already-upgraded sandbox
(live == target) is a true no-op, skipped with no delivery or handover, and a
downgrade is refused. Target binaries live on the node-local read-only
/fc-envdgcsfuse mount./upgrade, so thetrigger skips a live version below
MinEnvdVersionForUpgrade(0.6.11) andcounts it (
gated{reason=old_envd}).Sandbox.CallEnvdUpgradestreams the binary overthe authenticated
/upgradebody to a fixed guest path; envd reading the bodythen exec'ing without replying (transport drop) is the expected path. After
WaitForEnvd(which re-reads the version off the new envd's/init), successis confirmed by the running version actually equalling the target — a transport
quirk can't mislabel the outcome. Only unambiguous never-reached-envd errors
(connection refused / dial) are treated as delivery failures.
Flags / knobs:
envd-upgrade-target(LD string)off|promoted|<git-sha>offoff: no upgrade.promoted: track node-localHOST_ENVD_PATH, upgrade when strictly newer than the live version.<sha>: pin/fc-envd/envd.<sha>.ENVD_UPGRADE_TARGET(env fallback)offDEFAULT_FIRECRACKER_VERSION.MinEnvdVersionForUpgrade(const)0.6.11/upgrade; trigger skips.Rollout metrics
envd has no metrics pipeline, so its side reports via Loki summary logs
(
handover_resumed{procs,retained},watchers_rearmed{...}). The orchestratorside adds:
orchestrator.envd.upgrade.attempts{result,from_version,to_version}—result ∈ {success, delivery_failed, not_ready, version_mismatch, panic};from_versionis the live running version.successis recorded only whenthe running version actually flips to
to_version(not inferred from thetransport outcome).
success/totalis the rollout success rate; the commonper-resume no-op (flag off / already on target) resolves to
""and isdeliberately not counted.
orchestrator.envd.upgrade.duration{result}— wall-time of delivery + triggerWaitForEnvd, i.e. the overhead added to the resume.orchestrator.envd.upgrade.gated{reason=old_envd}— targeted resumes skippedby the version gate (watch during a ramp).
envd.upgradedbool label onorchestrator.sandbox.create.duration, and anenvd-upgradeTempo child span.The post-upgrade readiness re-check re-runs
/init(to re-read the runningversion) but is guarded to record the per-start resume KPIs — the envd-init
duration histogram + call counter and
StartedAt— once per start, so asuccessful upgrade doesn't double-count the init or push
StartedAt(and thusexecution_time) to a later, wrong timestamp.Validation
Connectafter a gap-exit returns theretained Start+End by pid/tag (unknown → NotFound); a live PID is never served a
stale cached exit and a reused PID keeps its successor; a stale retention timer
can't evict a newer entry; a re-adopted process whose
pidfd_openfails stillemits a terminal event (no orphan); a watcher exported then imported keeps its
id, delivers post-handover events, and preserves pending events;
ResumeFromHandoveralways thaws on a malformed blob and on no blob;Upgraderefuses a target other than the fixed path; the resolver is table-driven over
off/unset/promoted/sha × newer/same/older/missing (downgrade refused) without a
LaunchDarkly client;
/initadvertisesX-Envd-Version; the delivery-errorclassifier flags only never-reached-envd; the three metrics have
description+unit entries and construct.
the final code): this branch's envd was rebuilt at 0.6.11 (baked into the
template) and 0.6.12 (staged as the
promotedtarget), and this branch'sorchestrator (
0.2.0-c21705aeb) deployed to the dev client nodes. Validatedagainst a live workload — a stdout-writing + tmpfs-appending background counter,
so a broken fd-carry would SIGPIPE-kill it and line growth proves it kept
running:
0.6.11→0.6.12; the running envd becomes
/usr/bin/envd.next --resume-handoverand the counter process keeps the same PID and keeps writing across the
swap (7→20 lines) — exercising stdio fd inheritance and pidfd reaping end
to end. The orchestrator confirms by ground truth:
envd_upgrade_attempts_total{result="success",from_version="0.6.11",to_version="0.6.12"} = 1.self-upgradein the journal.its exit code (42) recovered by a late
Connectvia the carried retentioncache.
The rollout metrics are emitted on the same path
(
attempts{result,from,to},duration,gated,create.duration{envd_upgraded}),observed on dev Prometheus with a per-upgrade
durationof ≈ 345–440 ms.review-driven changes: live-version decision, version-confirmed success, the
shared freeze lock held across the handover, the freeze/serialize/exec path, and
the once-per-start resume-KPI guard on the post-upgrade readiness re-check). The
remaining review changes that only manifest on failure/edge paths — reaper
retaining before closing its channel, watcher re-arm before thaw, chained-upgrade
deadline carry, exec-fail fd cleanup, and the PID-reuse / retain-timer /
reaper-orphan / duplicate-forwarder fixes — are covered by the unit tests above;
they don't change the success path the e2e validated.
Key decisions
syscall.Exec, not restart. A fresh process would be reparentedby systemd (
Type=simple), orphaning the workload; keeping the PID means envdstays the workload's parent and the inherited fds/PTYs stay valid.
wait4, neverwait4(-1). A blanket reaper wouldsteal children that
os/execspawns after the upgrade; targeted reaping keepsboth the re-adopted and the new processes correct.
window and gives free cohort ramping (the resume-site LD context already
carries envd-version/team/template). The whole path is recover-wrapped,
version-gated, and bounded, so a failed or slow upgrade degrades to
upgraded=falseand the sandbox resumes on the old envd. Caveat: the triggeris currently synchronous on the resume-critical path, so on pathologically slow
resumes (large cold-fault working sets) it can add up to its timeout budget to
the tail; moving it off the critical path is a candidate follow-up.
/init. Theorchestrator can't cheaply know the running envd version, but envd can report
it — so envd advertises
pkg.Versionvia anX-Envd-Versionheader on/init(which the resume path already calls) and the orchestrator keys the decision,
from_version, idempotency, and the success check on it. Re-resumes of anupgraded sandbox become true no-ops and success is ground-truth-accurate, with
no new endpoint and no extra round-trip. Version comparison still relies on
packages/envd/pkg/version.gobeing bumped per behavioral change (repo rule);the resolver refuses any target that isn't strictly newer.
path and refuses any other, so an authenticated-but-malformed
/upgradecan'tturn the same-PID exec into arbitrary code execution.
getVersioninjected into the shared resolver sopackages/sharedgainsno dependency on the orchestrator's envd build package.
rootfs diff and an outgoing/incoming envd built at different versions stay
compatible.
envd-upgrade-target=off, env fallbackoff);dev has no LaunchDarkly so the feature is inert there unless
ENVD_UPGRADE_TARGETis set.🤖 Generated with Claude Code