Repository navigation
fix(api): drain in-flight background tails on shutdown via existing TrackWork (rebase of #3375) - #3725
Open
AdaAibaby wants to merge 1 commit into
Open
fix(api): drain in-flight background tails on shutdown via existing TrackWork (rebase of #3375)#3725AdaAibaby wants to merge 1 commit into
AdaAibaby wants to merge 1 commit into
Conversation
…rackWork The API shutdown path drains HTTP/gRPC handlers and then waits on the orchestrators tracked work (apiStore.Drain -> o.work). But several goroutines spawned during request handling with a detached context are launched with a bare go and are tracked by nothing, so shutdown abandons them: - store.go: AsyncNewlyCreatedSandbox (create analytics + posthog event) - delete_instance.go x2: analyticsRemove (billing-critical delete event) - create_instance.go: removeSandboxFromNode (kills the orphaned sandbox left when adding it to the store fails) - volume_delete.go / volume_create.go: deleteVolume (removes volume data on the node) Rather than add a second parallel WaitGroup (as the original e2b-dev#3375 did, before main grew TrackWork/Drain), route these through the mechanism that already exists. New GoTrackedTail registers the goroutine with TrackWork when the drain has not started, so Drain waits for it; once the drain has started TrackWork refuses new counts (adding to the WaitGroup after Wait would panic), so the goroutine still runs untracked and fire-and-forget - dropping it would lose a billing event or leak a sandbox. The sandbox store reaches it through a new optional GoTracked callback, keeping the package free of an orchestrator import. No new shutdown step and no new public WaitGroup: the tails now ride the apiStore.Drain already in main.go. Adds tests for both paths (Drain waits for a pre-drain tail; a tail started mid-drain still runs and never panics). Rebased follow-up to e2b-dev#3375 at the maintainers invitation. Closes e2b-dev#3357.
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
October 22, 2026 07:10
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased follow-up to #3375, which @ryan-sana closed with: "tracking the background goroutines is the right shape of fix … A rebased PR against current main would be welcome." This is that rebase — but adapted to a mechanism main grew in the meantime, so it's smaller than the original.
Problem (still present on current main)
The shutdown path drains HTTP/gRPC handlers and then waits on the orchestrator's tracked work (
apiStore.Drain→o.work). But several goroutines spawned during request handling with a detached context (context.WithoutCancel) are launched with a baregoand tracked by nothing, so shutdown abandons them mid-flight:sandbox/store.goAsyncNewlyCreatedSandboxorchestrator/delete_instance.go(×2)analyticsRemoveorchestrator/create_instance.goremoveSandboxFromNodehandlers/volume_delete.go,volume_create.godeleteVolumeWhy this isn't the original #3375
When #3375 was written, there was no drain-tracking mechanism, so it added a parallel
sync.WaitGroup(bgWg) +GoBackground/WaitBackground+ a new 30s shutdown step. Since then main grewTrackWork()/o.work/Drain()(already wired intomain.go). ReintroducingbgWgnow would just duplicate that.So instead this routes the four sites through the existing mechanism:
GoTrackedTail(fn)— registers the goroutine withTrackWork()when the drain hasn't started, soDrainwaits for it. Once the drain has started,TrackWorkrefuses new counts (adding to the WaitGroup afterWaitwould panic), sofnstill runs — untracked, fire-and-forget, because dropping a billing event or leaking a sandbox is worse than running past the drain.GoTracked func(func())callback, so thesandboxpackage keeps its independence fromorchestrator.No new shutdown step and no new public WaitGroup — the tails ride the
apiStore.Drain(ctx)that's already inmain.go.Semantics decision worth a look
These goroutines are the tail of an already-admitted operation, not new work. So during a drain they are not dropped (which
TrackWork'sok=falsewould do for genuine new work) — they run fire-and-forget. If you'd rather they be dropped once draining, that's a one-line change; flagging it explicitly.Tests
orchestrator/go_tracked_tail_test.go:TestGoTrackedTail_DrainWaitsForTail— a tail started before the drain is tracked;Drainblocks until it finishes.TestGoTrackedTail_RunsUntrackedDuringDrain— a tail started mid-drain still runs and never panics.go build ./...andgo vetpass. (TestListSandboxesUndefinedFlagIsQuietfails identically on cleanmain— it needs a Redis URL in the env — so it's unrelated.)Closes #3357. Supersedes #3375.