Skip to content

refactor(api): persist routing with sandboxes#3255

Draft
jakubno wants to merge 4 commits into
mainfrom
move-sandbox-routing-to-be-part-of-the-sandbox-store-en-1654
Draft

refactor(api): persist routing with sandboxes#3255
jakubno wants to merge 4 commits into
mainfrom
move-sandbox-routing-to-be-part-of-the-sandbox-store-en-1654

Conversation

@jakubno

@jakubno jakubno commented Jul 10, 2026

Copy link
Copy Markdown
Member

No description provided.

@linear-code

linear-code Bot commented Jul 10, 2026

Copy link
Copy Markdown

EN-1654

@cla-bot cla-bot Bot added the cla-signed label Jul 10, 2026
@cursor

cursor Bot commented Jul 10, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Routing correctness now depends on storage add/remove/rollback ordering and on two catalog handles (injected Store vs embedded redis Storage) staying aligned on the same Redis client. A failed Add after the catalog write can leave an expiration-index member if the Lua store fails, since rollback intentionally skips removing it.

Overview
Sandbox routing is written and torn down together with Redis sandbox persistence instead of a separate orchestrator callback after Add. Callers pass optional RoutingMetadata (skipped for remote cluster nodes that register via gRPC); the Redis Add path stores catalog entries with rollback on failure, and route removal happens on terminal StartRemoving for local clusters plus idempotent cleanup on Remove. The orchestrator drops lifecycle.go / AddSandboxToRoutingTable and wires the shared catalog through sandbox.Store.

Reviewed by Cursor Bugbot for commit 634026f. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
3323 5 3318 9
View the top 3 failed test(s) by shortest run time
github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/uffd/userfaultfd::TestRemoveThenWriteGated
Stack Traces | 0s run time
=== RUN   TestRemoveThenWriteGated
=== PAUSE TestRemoveThenWriteGated
=== CONT  TestRemoveThenWriteGated
--- FAIL: TestRemoveThenWriteGated (0.00s)
github.com/e2b-dev/infra/tests/integration/internal/tests/api/sandboxes::TestUpdateNetworkConfig/12_allow_internet_access_false_blocks_all
Stack Traces | 0.07s run time
=== RUN   TestUpdateNetworkConfig/12_allow_internet_access_false_blocks_all
Executing command curl in sandbox irele2nutkkwbqlt1tckd
    sandbox_network_update_test.go:355: Command [curl] output: event:{start:{pid:1082}}
    sandbox_network_update_test.go:355: 
        	Error Trace:	.../api/sandboxes/sandbox_network_out_test.go:78
        	            				.../api/sandboxes/sandbox_network_update_test.go:87
        	            				.../api/sandboxes/sandbox_network_update_test.go:355
        	Error:      	"failed to execute command curl in sandbox irele2nutkkwbqlt1tckd: invalid_argument: protocol error: incomplete envelope: unexpected EOF" does not contain "failed with exit code"
        	Test:       	TestUpdateNetworkConfig/12_allow_internet_access_false_blocks_all
        	Messages:   	Expected connection failure message
--- FAIL: TestUpdateNetworkConfig/12_allow_internet_access_false_blocks_all (0.07s)
github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/uffd/userfaultfd::TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write
Stack Traces | 0.38s run time
=== RUN   TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write
=== PAUSE TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write
=== CONT  TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write
    remove_test.go:289: 
        	Error Trace:	.../uffd/userfaultfd/remove_test.go:289
        	Error:      	elements differ
        	            	
        	            	extra elements in list A:
        	            	([]interface {}) (len=1) {
        	            	 (uint) 0
        	            	}
        	            	
        	            	
        	            	listA:
        	            	([]uint) (len=1) {
        	            	 (uint) 0
        	            	}
        	            	
        	            	
        	            	listB:
        	            	([]uint) <nil>
        	Test:       	TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write
    remove_test.go:290: 
        	Error Trace:	.../uffd/userfaultfd/remove_test.go:290
        	Error:      	Should be empty, but was [0]
        	Test:       	TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write
--- FAIL: TestRemoveThenWriteGated/4k_gated_remove_with_concurrent_write (0.38s)
github.com/e2b-dev/infra/tests/integration/internal/tests/proxies::TestMaskRequestHostAPIParameter
Stack Traces | 4.93s run time
=== RUN   TestMaskRequestHostAPIParameter
=== PAUSE TestMaskRequestHostAPIParameter
=== CONT  TestMaskRequestHostAPIParameter
    mask_request_host_test.go:44: Command [python3] output: event:{start:{pid:1114}}
    mask_request_host_test.go:68: Command [cat] output: event:{start:{pid:1115}}
    mask_request_host_test.go:68: Command [cat] output: event:{data:{stderr:"cat: /tmp/nc_output.txt: No such file or directory\n"}}
    mask_request_host_test.go:68: Command [cat] output: event:{end:{exit_code:1 exited:true status:"exit status 1" error:"exit status 1"}}
    mask_request_host_test.go:69: 
        	Error Trace:	.../tests/proxies/mask_request_host_test.go:69
        	Error:      	Received unexpected error:
        	            	command cat in sandbox i77so05n9wrnqxubsix2s failed with exit code 1
        	Test:       	TestMaskRequestHostAPIParameter
--- FAIL: TestMaskRequestHostAPIParameter (4.93s)
github.com/e2b-dev/infra/packages/api/internal/sandbox/storage/redis::TestStorageLocker_ObtainUsesProvidedContext
Stack Traces | 8.61s run time
=== RUN   TestStorageLocker_ObtainUsesProvidedContext
=== PAUSE TestStorageLocker_ObtainUsesProvidedContext
=== CONT  TestStorageLocker_ObtainUsesProvidedContext
    lock_test.go:97: 
        	Error Trace:	.../storage/redis/lock_test.go:289
        	            				.../storage/redis/lock_test.go:97
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestStorageLocker_ObtainUsesProvidedContext
--- FAIL: TestStorageLocker_ObtainUsesProvidedContext (8.61s)
github.com/e2b-dev/infra/tests/integration/internal/tests/api/sandboxes::TestUpdateNetworkConfig
Stack Traces | 34.1s run time
=== RUN   TestUpdateNetworkConfig
=== PAUSE TestUpdateNetworkConfig
=== CONT  TestUpdateNetworkConfig
--- FAIL: TestUpdateNetworkConfig (34.07s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors sandbox routing management by moving the responsibility of storing and deleting routing metadata from orchestrator callbacks directly into the sandbox store and Redis storage layers. A new Routing field has been added to the Sandbox struct, and the Redis storage now handles route persistence, rollback on failure, and cleanup during removal. The review feedback highlights a potential nil pointer dereference in the new sandboxRoutingMetadata helper function when the node parameter is nil, recommending a defensive nil check.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread packages/api/internal/orchestrator/routing.go
return errors.Join(
fmt.Errorf("failed to store sandbox in Redis: %w", err),
s.rollbackRouting(context.WithoutCancel(ctx), sbx, routing),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Route stored before sandbox

Medium Severity

Storage.Add writes the routing catalog before the Lua SET that persists the sandbox. The prior flow registered routes only after a successful store add, so catalog lookups could briefly point at a sandbox that is not yet in Redis team storage.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7b437df. Configure here.

Comment thread packages/api/internal/sandbox/storage/redis/operations.go

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 634026f. Configure here.

var sbx sandboxtypes.Sandbox
if err := json.Unmarshal([]byte(raw), &sbx); err != nil {
return fmt.Errorf("failed to unmarshal removed sandbox: %w", err)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove fails after key deleted

Medium Severity

After removeSandboxScript deletes the sandbox key, a JSON unmarshal error makes Remove return an error and skips expiration-index and routing cleanup that follow.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 634026f. Configure here.

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.

1 participant