From 459dbcd1e59f7768190858295e131f5649a9f764 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 9 Jun 2026 22:31:24 +0200 Subject: [PATCH 1/2] Fix orphaned app process and async bind error in apps run-local If proxy setup failed, the already-started app process was never killed and kept running in the background holding the app port. The proxy listener was also bound inside a goroutine, so a taken proxy port only printed an error while the command kept running without a working proxy. Kill the app process on the proxy setup error path, and bind the proxy listener synchronously (via the new appproxy Listen/Serve split) before serving in a goroutine so bind errors fail the command. Co-authored-by: Isaac --- cmd/apps/run_local.go | 22 ++++++++++++-- cmd/apps/run_local_test.go | 53 ++++++++++++++++++++++++++++++++++ libs/appproxy/appproxy.go | 18 ++++-------- libs/appproxy/appproxy_test.go | 4 +-- 4 files changed, 81 insertions(+), 16 deletions(-) create mode 100644 cmd/apps/run_local_test.go diff --git a/cmd/apps/run_local.go b/cmd/apps/run_local.go index 144fbf5354a..e65c683be9d 100644 --- a/cmd/apps/run_local.go +++ b/cmd/apps/run_local.go @@ -127,9 +127,17 @@ func setupProxy(ctx context.Context, cmd *cobra.Command, config *runlocal.Config } proxyAddr := fmt.Sprintf("localhost:%d", port) + // Bind synchronously so that a taken port fails the command instead of only + // printing an error from the goroutine while the command keeps running + // without a working proxy. + ln, err := proxy.Listen(proxyAddr) + if err != nil { + return fmt.Errorf("failed to start app proxy: %w", err) + } + + cmdio.LogString(ctx, "To access your app go to http://"+proxyAddr) go func() { - cmdio.LogString(ctx, "To access your app go to http://"+proxyAddr) - err := proxy.ListenAndServe(proxyAddr) + err := proxy.Serve(ln) if err != nil { cmd.PrintErrln(err) } @@ -142,6 +150,15 @@ func setupProxy(ctx context.Context, cmd *cobra.Command, config *runlocal.Config return nil } +// killAppProcess stops an already-started app process. Without it, a failure +// after startAppProcess would leave the app running in the background, holding +// the app port even after the CLI exits. +func killAppProcess(appCmd *exec.Cmd) { + _ = appCmd.Process.Kill() + // Reap the process so it doesn't linger as a zombie until the CLI exits. + _ = appCmd.Wait() +} + // SIGTERM (not supported on Windows) and SIGINT (Ctrl+C, supported cross-platform) // are caught to enable graceful shutdown of the app process. func handleGracefulShutdown(appCmd *exec.Cmd) error { @@ -226,6 +243,7 @@ func newRunLocal() *cobra.Command { err = setupProxy(ctx, cmd, config, w, port, debug) if err != nil { + killAppProcess(appCmd) return err } diff --git a/cmd/apps/run_local_test.go b/cmd/apps/run_local_test.go new file mode 100644 index 00000000000..47d182fbfdc --- /dev/null +++ b/cmd/apps/run_local_test.go @@ -0,0 +1,53 @@ +package apps + +import ( + "net" + "os" + "os/exec" + "testing" + "time" + + "github.com/databricks/cli/libs/apps/runlocal" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/spf13/cobra" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestSetupProxyPortInUse(t *testing.T) { + // Occupy a port so that the proxy fails to bind to it. + ln, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + defer ln.Close() + port := ln.Addr().(*net.TCPAddr).Port + + m := mocks.NewMockWorkspaceClient(t) + m.GetMockCurrentUserAPI().EXPECT().Me(mock.Anything, mock.Anything).Return(&iam.User{UserName: "test-user"}, nil) + + config := runlocal.NewConfig("https://workspace.databricks.test", "123", t.TempDir(), runlocal.DEFAULT_HOST, runlocal.DEFAULT_PORT) + err = setupProxy(t.Context(), &cobra.Command{}, config, m.WorkspaceClient, port, false) + require.ErrorContains(t, err, "failed to start app proxy") +} + +// TestAppHelperProcess is not a real test: TestKillAppProcess re-invokes the +// test binary with -test.run targeting it to get a long-running child process. +func TestAppHelperProcess(t *testing.T) { + if os.Getenv("APPS_TEST_HELPER_PROCESS") != "1" { + t.Skip("helper process for TestKillAppProcess") + } + time.Sleep(time.Minute) +} + +func TestKillAppProcess(t *testing.T) { + appCmd := exec.Command(os.Args[0], "-test.run=^TestAppHelperProcess$") + appCmd.Env = append(os.Environ(), "APPS_TEST_HELPER_PROCESS=1") + require.NoError(t, appCmd.Start()) + + killAppProcess(appCmd) + + // A non-nil ProcessState proves the process was reaped; a non-success exit + // proves it was killed while still running rather than finishing on its own. + require.NotNil(t, appCmd.ProcessState) + require.False(t, appCmd.ProcessState.Success()) +} diff --git a/libs/appproxy/appproxy.go b/libs/appproxy/appproxy.go index d1d7c5c9df8..52d086092c2 100644 --- a/libs/appproxy/appproxy.go +++ b/libs/appproxy/appproxy.go @@ -33,24 +33,18 @@ func New(ctx context.Context, targetURL string) (*Proxy, error) { return &proxy, nil } -func (p *Proxy) listen(addr string) (net.Listener, error) { +// Listen binds the proxy to the given address (host:port, e.g. localhost:8080). +// It is separate from Serve so callers can fail fast on bind errors (e.g. the +// port is already taken) before serving in the background. +func (p *Proxy) Listen(addr string) (net.Listener, error) { return net.Listen("tcp", addr) } -func (p *Proxy) serve(ln net.Listener) error { +// Serve accepts connections on the given listener and forwards all requests to the targetURL. +func (p *Proxy) Serve(ln net.Listener) error { return p.server.Serve(ln) } -// ListenAndServe starts the proxy server on the given address (host:port, e.g. localhost:8080) -// The proxy will forward all requests to the targetURL -func (p *Proxy) ListenAndServe(addr string) error { - ln, err := p.listen(addr) - if err != nil { - return err - } - return p.serve(ln) -} - func (p *Proxy) Stop() error { return p.server.Shutdown(p.ctx) } diff --git a/libs/appproxy/appproxy_test.go b/libs/appproxy/appproxy_test.go index 3325a3046a5..2f90ce06d0e 100644 --- a/libs/appproxy/appproxy_test.go +++ b/libs/appproxy/appproxy_test.go @@ -37,11 +37,11 @@ func startProxy(t *testing.T, serverAddr string) (*Proxy, string) { proxy, err := New(t.Context(), "http://"+serverAddr) require.NoError(t, err) - ln, err := proxy.listen(fmt.Sprintf("localhost:%d", PROXY_PORT)) + ln, err := proxy.Listen(fmt.Sprintf("localhost:%d", PROXY_PORT)) require.NoError(t, err) go func() { - _ = proxy.serve(ln) + _ = proxy.Serve(ln) }() return proxy, ln.Addr().String() From 5b59f34e553fbf4505f7485b0cbc8fb88c0b34b9 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Wed, 10 Jun 2026 00:21:07 +0200 Subject: [PATCH 2/2] Remove redundant comments Co-authored-by: Isaac --- cmd/apps/run_local.go | 7 +------ cmd/apps/run_local_test.go | 4 +--- libs/appproxy/appproxy.go | 2 -- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/cmd/apps/run_local.go b/cmd/apps/run_local.go index e65c683be9d..d9671276f0f 100644 --- a/cmd/apps/run_local.go +++ b/cmd/apps/run_local.go @@ -127,9 +127,7 @@ func setupProxy(ctx context.Context, cmd *cobra.Command, config *runlocal.Config } proxyAddr := fmt.Sprintf("localhost:%d", port) - // Bind synchronously so that a taken port fails the command instead of only - // printing an error from the goroutine while the command keeps running - // without a working proxy. + // Bind synchronously so a taken port fails the command instead of only printing an error from the goroutine. ln, err := proxy.Listen(proxyAddr) if err != nil { return fmt.Errorf("failed to start app proxy: %w", err) @@ -150,9 +148,6 @@ func setupProxy(ctx context.Context, cmd *cobra.Command, config *runlocal.Config return nil } -// killAppProcess stops an already-started app process. Without it, a failure -// after startAppProcess would leave the app running in the background, holding -// the app port even after the CLI exits. func killAppProcess(appCmd *exec.Cmd) { _ = appCmd.Process.Kill() // Reap the process so it doesn't linger as a zombie until the CLI exits. diff --git a/cmd/apps/run_local_test.go b/cmd/apps/run_local_test.go index 47d182fbfdc..94a0b56ec70 100644 --- a/cmd/apps/run_local_test.go +++ b/cmd/apps/run_local_test.go @@ -16,7 +16,6 @@ import ( ) func TestSetupProxyPortInUse(t *testing.T) { - // Occupy a port so that the proxy fails to bind to it. ln, err := net.Listen("tcp", "localhost:0") require.NoError(t, err) defer ln.Close() @@ -46,8 +45,7 @@ func TestKillAppProcess(t *testing.T) { killAppProcess(appCmd) - // A non-nil ProcessState proves the process was reaped; a non-success exit - // proves it was killed while still running rather than finishing on its own. + // A non-nil ProcessState proves the process was reaped; a non-success exit proves it was killed. require.NotNil(t, appCmd.ProcessState) require.False(t, appCmd.ProcessState.Success()) } diff --git a/libs/appproxy/appproxy.go b/libs/appproxy/appproxy.go index 52d086092c2..bfe5150ff96 100644 --- a/libs/appproxy/appproxy.go +++ b/libs/appproxy/appproxy.go @@ -34,8 +34,6 @@ func New(ctx context.Context, targetURL string) (*Proxy, error) { } // Listen binds the proxy to the given address (host:port, e.g. localhost:8080). -// It is separate from Serve so callers can fail fast on bind errors (e.g. the -// port is already taken) before serving in the background. func (p *Proxy) Listen(addr string) (net.Listener, error) { return net.Listen("tcp", addr) }