fix(envd): re-forward a sandbox port that drops out of a single scan - #3514
fix(envd): re-forward a sandbox port that drops out of a single scan#3514tomassrnka wants to merge 1 commit into
Conversation
The port forwarder marked every entry in its ports map DELETE, re-marked the ones the scan still reported, then stopped the rest — but never removed them from the map. A listening socket that missed one scan therefore had its socat killed while its entry survived, and when the same listener showed up again the entry was simply flipped back to FORWARD. Because it was already in the map no socat was ever started for it, so the port stayed unreachable from outside the sandbox for the rest of the sandbox's life even though the app was still listening. The map also grew without bound, one entry per listening socket ever observed. Listeners do drop out of individual scans: an app can close and reopen its socket, and the scanner reports a socket with no pid whenever it cannot map that socket's inode to a process. Drop stopped entries from the map, and do the stopping before starting new forwards so a fresh socat never contends for a bind address that a socat being torn down still holds.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit c051f38. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
What broke
A sandbox app that is listening on a port can become permanently unreachable through the E2B app proxy, for the rest of the sandbox's life. The proxy then answers with the "port closed" page even though the app never stopped listening.
Root cause
Forwarder.StartForwardingreconciles aportsmap against each 1s scan of listening sockets: mark every entryDELETE, re-mark the ones the scan still reports asFORWARD, stop the rest. It stopped them — killing the socat process group — but never removed them from the map.So a listening socket that misses a single scan has its socat killed while its entry survives. When the same listener shows up in the next scan, the entry is found and flipped straight back to
FORWARD; because it is already in the map,startPortForwardingis never called again for it. The port is dead from then on. The map also grew unbounded: one entry per listening socket ever observed.Listeners really do drop out of individual scans. An app can close and reopen its socket, and the scanner (
gopsutilnet.Connections) reports a socket with pid 0 whenever it cannot map that socket's inode to a process, which changes thepid-portkey.Fix
Delete stopped entries from the map, and run the stop pass before starting new forwards so a fresh socat never has to contend for a bind address that a socat being torn down still holds. The reconciliation body moved into
Forwarder.refreshso it can be driven directly from tests.How it was reproduced / verified
forward_refresh_test.godrivesrefreshwith synthetic scan results and puts asocatshim onPATHthat re-executes the test binary and really binds the forwarded address, so "is this port forwarded" is a real TCP dial.Before:
port 54544 forwarded=false, want trueafter the port reappears, plusmap[100-54544:...]left behind. After: both tests pass.gofmt,go vet,go test,go test -raceand golangci-lint 2.11.4 are clean on the package.Found while investigating https://linear.app/e2b/issue/EN-1127/e2b-client-proxy-is-unstable-for-websockets-and-long-running-requests