Skip to content

Conversation

@dobrac
Copy link
Contributor

@dobrac dobrac commented Aug 31, 2025

Improve sandbox cleanup logic to prevent accidental mistakes. The Stop now just stops the sandbox, while Close properly cleans up the resources. You're always required to call Close, you don't need to call Stop. You can also call Wait to listen for the sandbox stop event.

@dobrac dobrac self-assigned this Aug 31, 2025
@dobrac dobrac added the bug Something isn't working label Aug 31, 2025
@dobrac dobrac force-pushed the fix-sandbox-cleanup-for-build branch 3 times, most recently from 4e8bd4e to 71962ed Compare August 31, 2025 15:26
@dobrac dobrac marked this pull request as ready for review August 31, 2025 15:38
cursor[bot]

This comment was marked as outdated.

@dobrac dobrac force-pushed the fix-sandbox-cleanup-for-build branch from 71962ed to 684c443 Compare August 31, 2025 16:05
@dobrac dobrac marked this pull request as draft August 31, 2025 16:17
@dobrac dobrac force-pushed the fix-sandbox-cleanup-for-build branch 3 times, most recently from 29bef8a to c547ab3 Compare September 1, 2025 12:53
@dobrac dobrac changed the title Fix sandbox cleanup for template build Improve sandbox cleanup logic Sep 1, 2025
@dobrac dobrac marked this pull request as ready for review September 1, 2025 13:25
cursor[bot]

This comment was marked as outdated.

@dobrac dobrac force-pushed the fix-sandbox-cleanup-for-build branch from c547ab3 to 0e1da21 Compare September 1, 2025 13:29
@dobrac
Copy link
Contributor Author

dobrac commented Sep 1, 2025

@cursor review

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@dobrac
Copy link
Contributor Author

dobrac commented Sep 1, 2025

Bug: Memory Exit Returns Unset Signal

The NoopMemory.Exit() method returns a new, unset utils.SetOnce instance on each call. This causes callers waiting for the exit signal to block indefinitely, as the distinct SetOnce instances are never set.

packages/orchestrator/internal/sandbox/uffd/noop.go#L51-L54
Fix in Cursor Fix in Web

This should be Ok as the noop memory never exits

cursor[bot]

This comment was marked as outdated.

Copy link
Member

@ValentaTomas ValentaTomas left a comment

Choose a reason for hiding this comment

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

Do you need to call stop before close?

@dobrac
Copy link
Contributor Author

dobrac commented Sep 1, 2025

Do you need to call stop before close?

You don't need to now (because it's already handled for you), but you still can if you want to call it sooner

@dobrac dobrac force-pushed the fix-sandbox-cleanup-for-build branch from 4a51822 to 2b3eaa5 Compare September 1, 2025 19:49
cursor[bot]

This comment was marked as outdated.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Sandbox Shutdown Hangs Due to Missing Cancellation

In the ResumeSandbox goroutine, after sbx.Stop() is called, the subsequent Wait() calls on both Firecracker and UFFD processes lack context cancellation. This can cause the goroutine to block indefinitely if one process hasn't fully exited, potentially hanging sandbox shutdowns.

packages/orchestrator/internal/sandbox/sandbox.go#L522-L535

go func() {
// Wait for either uffd or fc process to exit
select {
case <-fcUffd.Exit().Done():
case <-fcHandle.Exit.Done():
}
err := sbx.Stop(context.WithoutCancel(ctx), tracer)
uffdWaitErr := fcUffd.Exit().Wait()
fcErr := fcHandle.Exit.Wait()
exit.SetError(errors.Join(err, fcErr, uffdWaitErr))
}()

Fix in Cursor Fix in Web


@dobrac dobrac merged commit 5cd7234 into main Sep 2, 2025
26 checks passed
@dobrac dobrac deleted the fix-sandbox-cleanup-for-build branch September 2, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants