diff --git a/server/cmd/api/api/process.go b/server/cmd/api/api/process.go index 38a458f4..12eb448a 100644 --- a/server/cmd/api/api/process.go +++ b/server/cmd/api/api/process.go @@ -26,7 +26,6 @@ import ( "github.com/kernel/kernel-images/server/lib/logger" oapi "github.com/kernel/kernel-images/server/lib/oapi" "github.com/kernel/kernel-images/server/lib/ptyio" - "github.com/kernel/kernel-images/server/lib/wsdrain" openapi_types "github.com/oapi-codegen/runtime/types" ) @@ -660,7 +659,7 @@ func writeJSON(w http.ResponseWriter, status int, body string) { // - Server sends TextMessage with JSON for events (e.g., exit code) // // This endpoint is intentionally not defined in OpenAPI. -func (s *ApiService) HandleProcessAttachWS(w http.ResponseWriter, r *http.Request, id string, reg *wsdrain.Registry) { +func (s *ApiService) HandleProcessAttachWS(w http.ResponseWriter, r *http.Request, id string) { ctx := r.Context() log := logger.FromContext(ctx) @@ -703,9 +702,6 @@ func (s *ApiService) HandleProcessAttachWS(w http.ResponseWriter, r *http.Reques } defer wsConn.CloseNow() - untrack := reg.Track(wsConn) - defer untrack() - // Set a generous read limit for PTY data wsConn.SetReadLimit(1024 * 1024) // 1MB diff --git a/server/cmd/api/main.go b/server/cmd/api/main.go index babb8f40..fa489feb 100644 --- a/server/cmd/api/main.go +++ b/server/cmd/api/main.go @@ -14,7 +14,6 @@ import ( "syscall" "time" - "github.com/coder/websocket" "github.com/ghodss/yaml" "github.com/go-chi/chi/v5" chiMiddleware "github.com/go-chi/chi/v5/middleware" @@ -34,7 +33,6 @@ import ( "github.com/kernel/kernel-images/server/lib/scaletozero" "github.com/kernel/kernel-images/server/lib/sysmon" "github.com/kernel/kernel-images/server/lib/telemetry" - "github.com/kernel/kernel-images/server/lib/wsdrain" ) func main() { @@ -83,10 +81,7 @@ func main() { os.Exit(1) } - // ws conn tracker - wsRegistry := wsdrain.New() - - // DevTools WebSocket upstream manager: tail Chromium supervisord log + // DevTools WebSocket upstream manager: tail Chromium supervisord log const chromiumLogPath = "/var/log/supervisord/chromium" upstreamMgr := devtoolsproxy.NewUpstreamManager(chromiumLogPath, slogger) upstreamMgr.Start(ctx) @@ -176,7 +171,7 @@ func main() { // Uses WebSocket for bidirectional streaming, which works well through proxies. r.Get("/process/{process_id}/attach", func(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "process_id") - apiService.HandleProcessAttachWS(w, r, id, wsRegistry) + apiService.HandleProcessAttachWS(w, r, id) }) // Serve extension files for Chrome policy-installed extensions @@ -224,7 +219,7 @@ func main() { rDevtools.Get("/json/list", jsonTargetHandler) rDevtools.Get("/json/list/", jsonTargetHandler) rDevtools.Get("/*", func(w http.ResponseWriter, r *http.Request) { - devtoolsproxy.WebSocketProxyHandler(upstreamMgr, slogger, config.LogCDPMessages, stz, telemetrySession.Publish, wsRegistry).ServeHTTP(w, r) + devtoolsproxy.WebSocketProxyHandler(upstreamMgr, slogger, config.LogCDPMessages, stz, telemetrySession.Publish).ServeHTTP(w, r) }) srvDevtools := &http.Server{ @@ -250,7 +245,6 @@ func main() { rChromeDriver.Handle("/*", chromedriverproxy.Handler(slogger, &chromedriverproxy.Options{ ChromeDriverUpstream: config.ChromeDriverUpstreamAddr, DevToolsProxyAddr: config.DevToolsProxyAddr, - Registry: wsRegistry, })) srvChromeDriver := &http.Server{ @@ -320,12 +314,6 @@ func main() { g.Go(func() error { return apiService.Shutdown(shutdownCtx) }) - g.Go(func() error { - if n := wsRegistry.CloseAll(websocket.StatusGoingAway, "browser shutting down"); n > 0 { - slogger.Info("closed active websocket connections for shutdown", "count", n) - } - return nil - }) g.Go(func() error { upstreamMgr.Stop() return srvDevtools.Shutdown(shutdownCtx) diff --git a/server/cmd/chromium-launcher/main.go b/server/cmd/chromium-launcher/main.go index 762167e4..19f93e47 100644 --- a/server/cmd/chromium-launcher/main.go +++ b/server/cmd/chromium-launcher/main.go @@ -1,7 +1,6 @@ package main import ( - "context" "flag" "fmt" "net" @@ -167,41 +166,19 @@ func execLookPath(file string) (string, error) { return exec.LookPath(file) } -// waitForPort waits until the given port is available for binding on IPv4. -// This handles the delay after SIGKILL before the kernel releases the socket. -// We disable SO_REUSEADDR to get an accurate check matching chromium's bind behavior. +// waitForPort waits until the devtools port can be bound on IPv4. After SIGKILL +// the old listener may linger briefly; this loop waits for that to clear. +// Go's net.Listen sets SO_REUSEADDR, matching chromium's DevTools bind — so +// TIME_WAIT sockets from prior CDP client connections do not block the probe. // Only IPv4 is checked because IPv6 is disabled at the kernel level in the VM. func waitForPort(port string, timeout time.Duration) { deadline := time.Now().Add(timeout) - addrs := []string{"127.0.0.1:" + port} - - // ListenConfig with Control to disable SO_REUSEADDR for accurate port availability check - lc := &net.ListenConfig{ - Control: func(network, address string, c syscall.RawConn) error { - var sockErr error - err := c.Control(func(fd uintptr) { - // Disable SO_REUSEADDR to match chromium's behavior - sockErr = syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 0) - }) - if err != nil { - return err - } - return sockErr - }, - } + addr := "127.0.0.1:" + port - ctx := context.Background() for time.Now().Before(deadline) { - allFree := true - for _, addr := range addrs { - ln, err := lc.Listen(ctx, "tcp", addr) - if err != nil { - allFree = false - break - } + ln, err := net.Listen("tcp", addr) + if err == nil { ln.Close() - } - if allFree { return } time.Sleep(50 * time.Millisecond) diff --git a/server/lib/chromedriverproxy/proxy.go b/server/lib/chromedriverproxy/proxy.go index 612f34b6..c76186fd 100644 --- a/server/lib/chromedriverproxy/proxy.go +++ b/server/lib/chromedriverproxy/proxy.go @@ -12,7 +12,6 @@ import ( "strings" "github.com/coder/websocket" - "github.com/kernel/kernel-images/server/lib/wsdrain" "github.com/kernel/kernel-images/server/lib/wsproxy" ) @@ -26,9 +25,6 @@ const ( type Options struct { ChromeDriverUpstream string DevToolsProxyAddr string - // Registry, when set, tracks accepted WebDriver/BiDi connections so they - // are closed with a Going Away frame on server shutdown. - Registry *wsdrain.Registry } func resolveOptions(opts *Options) Options { @@ -44,8 +40,6 @@ func resolveOptions(opts *Options) Options { } if opts.DevToolsProxyAddr != "" { resolved.DevToolsProxyAddr = opts.DevToolsProxyAddr - } - resolved.Registry = opts.Registry return resolved } @@ -318,7 +312,6 @@ func proxyWebSocket(w http.ResponseWriter, r *http.Request, logger *slog.Logger, DialOptions: dialOpts, Logger: logger, Transform: transform, - Registry: cfg.Registry, }) } diff --git a/server/lib/devtoolsproxy/proxy.go b/server/lib/devtoolsproxy/proxy.go index df8b47eb..aded45a8 100644 --- a/server/lib/devtoolsproxy/proxy.go +++ b/server/lib/devtoolsproxy/proxy.go @@ -22,7 +22,6 @@ import ( "github.com/kernel/kernel-images/server/lib/events" oapi "github.com/kernel/kernel-images/server/lib/oapi" "github.com/kernel/kernel-images/server/lib/scaletozero" - "github.com/kernel/kernel-images/server/lib/wsdrain" "github.com/kernel/kernel-images/server/lib/wsproxy" ) @@ -310,7 +309,7 @@ type EventPublisher func(ev events.Event) (events.Envelope, bool) // If logCDPMessages is true, all CDP messages will be logged with their direction. // publish is invoked on accept (cdp_connect) and on teardown (cdp_disconnect); pass // nil to disable emission. -func WebSocketProxyHandler(mgr *UpstreamManager, logger *slog.Logger, logCDPMessages bool, ctrl scaletozero.Controller, publish EventPublisher, reg *wsdrain.Registry) http.Handler { +func WebSocketProxyHandler(mgr *UpstreamManager, logger *slog.Logger, logCDPMessages bool, ctrl scaletozero.Controller, publish EventPublisher) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Counts every relayed message so cdp_disconnect can report message_count. var msgCount atomic.Int64 @@ -350,9 +349,6 @@ func WebSocketProxyHandler(mgr *UpstreamManager, logger *slog.Logger, logCDPMess } clientConn.SetReadLimit(100 * 1024 * 1024) - untrack := reg.Track(clientConn) - defer untrack() - publishCdpConnect(publish) connectedAt := time.Now() diff --git a/server/lib/devtoolsproxy/proxy_test.go b/server/lib/devtoolsproxy/proxy_test.go index 5956a555..29469b8e 100644 --- a/server/lib/devtoolsproxy/proxy_test.go +++ b/server/lib/devtoolsproxy/proxy_test.go @@ -23,7 +23,6 @@ import ( "github.com/kernel/kernel-images/server/lib/events" oapi "github.com/kernel/kernel-images/server/lib/oapi" "github.com/kernel/kernel-images/server/lib/scaletozero" - "github.com/kernel/kernel-images/server/lib/wsdrain" "github.com/kernel/kernel-images/server/lib/wsproxy" ) @@ -133,7 +132,7 @@ func TestWebSocketProxyHandler_ProxiesEcho(t *testing.T) { // seed current upstream to echo server including path/query (bypass tailing) mgr.setCurrent((&url.URL{Scheme: u.Scheme, Host: u.Host, Path: u.Path, RawQuery: u.RawQuery}).String()) - proxy := WebSocketProxyHandler(mgr, logger, false, scaletozero.NewNoopController(), nil, nil) + proxy := WebSocketProxyHandler(mgr, logger, false, scaletozero.NewNoopController(), nil) proxySrv := httptest.NewServer(proxy) defer proxySrv.Close() @@ -165,64 +164,6 @@ func TestWebSocketProxyHandler_ProxiesEcho(t *testing.T) { } } -func TestWebSocketProxyHandler_RegistryClosesClientWithGoingAway(t *testing.T) { - echoSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - c, err := websocket.Accept(w, r, &websocket.AcceptOptions{OriginPatterns: []string{"*"}}) - if err != nil { - return - } - defer c.Close(websocket.StatusNormalClosure, "") - ctx := r.Context() - for { - mt, msg, err := c.Read(ctx) - if err != nil { - return - } - if err := c.Write(ctx, mt, msg); err != nil { - return - } - } - })) - defer echoSrv.Close() - - u, _ := url.Parse(echoSrv.URL) - logger := silentLogger() - mgr := NewUpstreamManager("/dev/null", logger) - mgr.setCurrent((&url.URL{Scheme: "ws", Host: u.Host, Path: "/echo"}).String()) - - reg := wsdrain.New() - proxySrv := httptest.NewServer(WebSocketProxyHandler(mgr, logger, false, scaletozero.NewNoopController(), nil, reg)) - defer proxySrv.Close() - - pu, _ := url.Parse(proxySrv.URL) - pu.Scheme = "ws" - ctx := context.Background() - conn, _, err := websocket.Dial(ctx, pu.String(), nil) - if err != nil { - t.Fatalf("dial proxy failed: %v", err) - } - defer conn.Close(websocket.StatusInternalError, "") - - // Round-trip so the proxy session is fully established and registered. - if err := conn.Write(ctx, websocket.MessageText, []byte("ping")); err != nil { - t.Fatalf("write failed: %v", err) - } - if _, _, err := conn.Read(ctx); err != nil { - t.Fatalf("read failed: %v", err) - } - - if n := reg.CloseAll(websocket.StatusGoingAway, "shutting down"); n != 1 { - t.Fatalf("CloseAll closed %d conns, want 1", n) - } - - // The client should observe a 1001 Going Away, not the 1000 the proxy's - // own cleanup would otherwise send. - _, _, err = conn.Read(ctx) - if got := websocket.CloseStatus(err); got != websocket.StatusGoingAway { - t.Fatalf("client close status = %v (err %v), want StatusGoingAway", got, err) - } -} - func TestDialUpstreamWithRetry_RechecksCurrentAfterMissedUpdate(t *testing.T) { // Start a working websocket upstream. upstreamSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/server/lib/wsdrain/wsdrain.go b/server/lib/wsdrain/wsdrain.go deleted file mode 100644 index 0df8a093..00000000 --- a/server/lib/wsdrain/wsdrain.go +++ /dev/null @@ -1,68 +0,0 @@ -// Package wsdrain tracks live WebSocket connections so they can be closed with -// a single status code when the server shuts down. -package wsdrain - -import ( - "sync" - - "github.com/coder/websocket" -) - -// Conn is the subset of *websocket.Conn the registry needs. -type Conn interface { - Close(code websocket.StatusCode, reason string) error -} - -// Registry tracks active connections. Construct one with New. All methods are -// safe for concurrent use and tolerate a nil receiver, so callers may pass a -// nil *Registry to disable tracking. -type Registry struct { - mu sync.Mutex - conns map[Conn]struct{} -} - -func New() *Registry { - return &Registry{conns: make(map[Conn]struct{})} -} - -// Track registers conn and returns a function that removes it. The returned -// function is idempotent; call it (e.g. via defer) when the connection ends. -func (r *Registry) Track(conn Conn) func() { - if r == nil || conn == nil { - return func() {} - } - r.mu.Lock() - r.conns[conn] = struct{}{} - r.mu.Unlock() - - var once sync.Once - return func() { - once.Do(func() { - r.mu.Lock() - delete(r.conns, conn) - r.mu.Unlock() - }) - } -} - -// CloseAll closes every tracked connection with the given code and reason and -// returns how many it closed. Connections are snapshotted under the lock and -// closed outside it. Close errors are ignored: the connection is being -// discarded regardless, and the first Close wins, so a later normal-closure -// from the connection's own teardown does not override the code sent here. -func (r *Registry) CloseAll(code websocket.StatusCode, reason string) int { - if r == nil { - return 0 - } - r.mu.Lock() - conns := make([]Conn, 0, len(r.conns)) - for c := range r.conns { - conns = append(conns, c) - } - r.mu.Unlock() - - for _, c := range conns { - _ = c.Close(code, reason) - } - return len(conns) -} diff --git a/server/lib/wsdrain/wsdrain_test.go b/server/lib/wsdrain/wsdrain_test.go deleted file mode 100644 index d48b4bdb..00000000 --- a/server/lib/wsdrain/wsdrain_test.go +++ /dev/null @@ -1,56 +0,0 @@ -package wsdrain - -import ( - "testing" - - "github.com/coder/websocket" -) - -type fakeConn struct { - closes []websocket.StatusCode -} - -func (c *fakeConn) Close(code websocket.StatusCode, _ string) error { - c.closes = append(c.closes, code) - return nil -} - -func TestCloseAllClosesTrackedConns(t *testing.T) { - r := New() - a, b := &fakeConn{}, &fakeConn{} - r.Track(a) - r.Track(b) - - if n := r.CloseAll(websocket.StatusGoingAway, "bye"); n != 2 { - t.Fatalf("CloseAll returned %d, want 2", n) - } - for _, c := range []*fakeConn{a, b} { - if len(c.closes) != 1 || c.closes[0] != websocket.StatusGoingAway { - t.Fatalf("conn closed with %v, want one StatusGoingAway", c.closes) - } - } -} - -func TestUntrackRemovesConn(t *testing.T) { - r := New() - c := &fakeConn{} - untrack := r.Track(c) - untrack() - untrack() // idempotent - - if n := r.CloseAll(websocket.StatusGoingAway, "bye"); n != 0 { - t.Fatalf("CloseAll returned %d after untrack, want 0", n) - } - if len(c.closes) != 0 { - t.Fatalf("untracked conn was closed: %v", c.closes) - } -} - -func TestNilRegistryIsNoop(t *testing.T) { - var r *Registry - untrack := r.Track(&fakeConn{}) - untrack() - if n := r.CloseAll(websocket.StatusGoingAway, "bye"); n != 0 { - t.Fatalf("nil registry CloseAll returned %d, want 0", n) - } -} diff --git a/server/lib/wsproxy/wsproxy.go b/server/lib/wsproxy/wsproxy.go index 8a7e5940..8a14e490 100644 --- a/server/lib/wsproxy/wsproxy.go +++ b/server/lib/wsproxy/wsproxy.go @@ -7,7 +7,6 @@ import ( "sync" "github.com/coder/websocket" - "github.com/kernel/kernel-images/server/lib/wsdrain" ) // Conn abstracts a WebSocket connection for testing and flexibility. @@ -29,9 +28,6 @@ type ProxyOptions struct { DialOptions *websocket.DialOptions Logger *slog.Logger Transform MessageTransform - // Registry, when set, tracks the accepted client connection so it is - // closed with a Going Away frame on server shutdown. - Registry *wsdrain.Registry } // PumpExitCause names which side caused Pump to return. Callers use this to @@ -125,9 +121,6 @@ func Proxy(w http.ResponseWriter, r *http.Request, upstreamURL string, opts Prox } clientConn.SetReadLimit(100 * 1024 * 1024) - untrack := opts.Registry.Track(clientConn) - defer untrack() - upstreamConn, _, err := websocket.Dial(r.Context(), upstreamURL, opts.DialOptions) if err != nil { logger.Error("dial upstream failed", slog.String("err", err.Error()), slog.String("url", upstreamURL)) diff --git a/server/openapi.yaml b/server/openapi.yaml index 4f56b0e6..fcbc991a 100644 --- a/server/openapi.yaml +++ b/server/openapi.yaml @@ -1767,11 +1767,7 @@ components: properties: text: type: string - description: > - Human-readable error text, as the browser console would display it. - For console.error() calls, the first argument coerced to a string. - For uncaught exceptions, the prefix and error message, e.g. - "Uncaught Error: boom" or "Uncaught (in promise) TypeError: x is not a function". + description: Error message text. Present in both source paths. stack_trace: $ref: "#/components/schemas/BrowserCallStack" level: diff --git a/server/runtime/playwright-daemon.ts b/server/runtime/playwright-daemon.ts index 6ce6c61d..578d3ecf 100644 --- a/server/runtime/playwright-daemon.ts +++ b/server/runtime/playwright-daemon.ts @@ -39,19 +39,6 @@ interface ExecuteResponse { stack?: string; } -function withTimeout(promise: Promise, signal: AbortSignal): Promise { - if (signal.aborted) { - return Promise.reject(signal.reason); - } - - return Promise.race([ - promise, - new Promise((_, reject) => { - signal.addEventListener('abort', () => reject(signal.reason), { once: true }); - }), - ]); -} - async function transformCode(code: string): Promise { // Wrap in async function so top-level await/return are valid for esbuild const wrapped = `async function __userCode__() {\n${code}\n}`; @@ -101,26 +88,23 @@ async function ensureBrowserConnection(): Promise { } console.error(`[playwright-daemon] Connecting to CDP: ${CDP_ENDPOINT}`); - const connectedBrowser = await chromium.connectOverCDP(CDP_ENDPOINT); - browser = connectedBrowser; + browser = await chromium.connectOverCDP(CDP_ENDPOINT); reconnectAttempts = 0; - connectedBrowser.on('disconnected', () => { + browser.on('disconnected', () => { console.error('[playwright-daemon] Browser disconnected'); - if (browser === connectedBrowser) { - browser = null; - } + browser = null; }); console.error('[playwright-daemon] CDP connection established'); - return connectedBrowser; + return browser; } finally { connecting = false; } } -async function executeCode(request: ExecuteRequest, signal: AbortSignal): Promise { - const { id, code } = request; +async function executeCode(request: ExecuteRequest): Promise { + const { id, code, timeout_ms = 60000 } = request; try { let jsCode: string; @@ -164,9 +148,15 @@ async function executeCode(request: ExecuteRequest, signal: AbortSignal): Promis const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; const userFunction = new AsyncFunction('page', 'context', 'browser', jsCode); - signal.throwIfAborted(); - const result = await userFunction(page, context, browserInstance); + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => reject(new Error(`Execution timed out after ${timeout_ms}ms`)), timeout_ms); + }); + + const result = await Promise.race([ + userFunction(page, context, browserInstance), + timeoutPromise, + ]); return { id, @@ -209,18 +199,7 @@ function handleConnection(socket: Socket): void { continue; } - const signal = AbortSignal.timeout(request.timeout_ms ?? 60000); - let response: ExecuteResponse; - try { - response = await withTimeout(executeCode(request, signal), signal); - } catch (error: any) { - response = { - id: request.id, - success: false, - error: error.message, - stack: error.stack, - }; - } + const response = await executeCode(request); socket.write(JSON.stringify(response) + '\n'); } });