diff --git a/cmd/apps/run_local.go b/cmd/apps/run_local.go index 144fbf5354a..d9671276f0f 100644 --- a/cmd/apps/run_local.go +++ b/cmd/apps/run_local.go @@ -127,9 +127,15 @@ func setupProxy(ctx context.Context, cmd *cobra.Command, config *runlocal.Config } proxyAddr := fmt.Sprintf("localhost:%d", port) + // 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) + } + + 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 +148,12 @@ func setupProxy(ctx context.Context, cmd *cobra.Command, config *runlocal.Config return nil } +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 +238,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..94a0b56ec70 --- /dev/null +++ b/cmd/apps/run_local_test.go @@ -0,0 +1,51 @@ +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) { + 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. + 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..bfe5150ff96 100644 --- a/libs/appproxy/appproxy.go +++ b/libs/appproxy/appproxy.go @@ -33,24 +33,16 @@ 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). +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()