From 54b3c705675de1b4ab03bc40ee9d5db346aa3fe8 Mon Sep 17 00:00:00 2001 From: et-nik Date: Wed, 2 Sep 2026 15:21:24 +0200 Subject: [PATCH 1/2] installation fix --- .../actions/daemon/install/daemon_install.go | 4 +- .../panel/install/checkers_internal_test.go | 96 +++++++++++-- internal/actions/panel/install/checkers_v4.go | 69 +++++---- .../panel/install/health_internal_test.go | 133 ++++++++++++++++++ internal/actions/panel/install/health_v4.go | 67 +++++++++ .../actions/panel/install/panel_install_v4.go | 87 ++++++------ .../install/stop_previous_v4_internal_test.go | 95 +++++++++++++ internal/pkg/panel/install.go | 5 + internal/pkg/panel/install_internal_test.go | 29 ++++ pkg/panel/install.go | 13 +- pkg/utils/fs.go | 29 ++++ pkg/utils/fs_linux_test.go | 51 +++++++ pkg/utils/fs_test.go | 62 ++++++++ 13 files changed, 652 insertions(+), 88 deletions(-) create mode 100644 internal/actions/panel/install/health_internal_test.go create mode 100644 internal/actions/panel/install/health_v4.go create mode 100644 internal/actions/panel/install/stop_previous_v4_internal_test.go create mode 100644 internal/pkg/panel/install_internal_test.go create mode 100644 pkg/utils/fs_linux_test.go diff --git a/internal/actions/daemon/install/daemon_install.go b/internal/actions/daemon/install/daemon_install.go index cecbe0a..c7a44c0 100644 --- a/internal/actions/daemon/install/daemon_install.go +++ b/internal/actions/daemon/install/daemon_install.go @@ -641,9 +641,9 @@ func installDaemonBinaries( return state, errors.WithMessage(err, "failed to stat file") } - err = utils.Move(fp, state.DaemonFilePath) + err = utils.ReplaceFile(fp, state.DaemonFilePath, 0755) if err != nil { - return state, errors.WithMessage(err, "failed to move gameap-daemon binaries") + return state, errors.WithMessage(err, "failed to install gameap-daemon binary") } binariesInstalled = true diff --git a/internal/actions/panel/install/checkers_internal_test.go b/internal/actions/panel/install/checkers_internal_test.go index 2eb3b23..c15e142 100644 --- a/internal/actions/panel/install/checkers_internal_test.go +++ b/internal/actions/panel/install/checkers_internal_test.go @@ -3,12 +3,10 @@ package install import ( "context" "net" - "net/http" - "net/http/httptest" - "strings" "testing" "github.com/gameap/gameapctl/internal/pkg/gameapctl" + "github.com/gameap/gameapctl/pkg/gameap" "github.com/gameap/gameapctl/pkg/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -191,26 +189,102 @@ func Test_filterAndCheckHostV4(t *testing.T) { } func Test_existingPanelDetected_RequiresPreviousInstallationOnTheSamePort(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) + port := servePanelHealth(t, panelHealthHandler) + + useTemporaryStateDirectory(t) + ctx := context.Background() + + // A service answering /api/health without a recorded installation is not our panel. + assert.False(t, existingPanelDetected(ctx, panelInstallStateV4{Port: port})) + + require.NoError(t, gameapctl.SavePanelInstallState(ctx, gameapctl.PanelInstallState{ + Version: "v4", + Port: port, })) - t.Cleanup(server.Close) - _, port, err := net.SplitHostPort(strings.TrimPrefix(server.URL, "http://")) - require.NoError(t, err) + assert.True(t, existingPanelDetected(ctx, panelInstallStateV4{Port: port})) + assert.False(t, existingPanelDetected(ctx, panelInstallStateV4{Port: "1"})) +} + +func Test_existingPanelDetected_RejectsSPAFallback(t *testing.T) { + port := servePanelHealth(t, spaFallbackHandler) useTemporaryStateDirectory(t) ctx := context.Background() - // A foreign service answering /health with 200 is not a GameAP panel. - assert.False(t, existingPanelDetected(ctx, port)) + require.NoError(t, gameapctl.SavePanelInstallState(ctx, gameapctl.PanelInstallState{ + Version: "v4", + Port: port, + })) + + // index.html with 200 on every path is not a ready panel. + assert.False(t, existingPanelDetected(ctx, panelInstallStateV4{Port: port})) +} + +func Test_existingPanelDetected_RejectsOtherScope(t *testing.T) { + port := servePanelHealth(t, panelHealthHandler) + + useTemporaryStateDirectory(t) + ctx := context.Background() require.NoError(t, gameapctl.SavePanelInstallState(ctx, gameapctl.PanelInstallState{ Version: "v4", + Scope: gameap.ScopeUser, Port: port, })) - assert.True(t, existingPanelDetected(ctx, port)) + assert.False(t, existingPanelDetected(ctx, panelInstallStateV4{Scope: gameap.ScopeSystem, Port: port})) + assert.True(t, existingPanelDetected(ctx, panelInstallStateV4{Scope: gameap.ScopeUser, Port: port})) +} + +func Test_existingPanelDetected_ProbesPreviousHost(t *testing.T) { + port := servePanelHealth(t, panelHealthHandler) + + useTemporaryStateDirectory(t) + ctx := context.Background() + + require.NoError(t, gameapctl.SavePanelInstallState(ctx, gameapctl.PanelInstallState{ + Version: "v4", + Host: "127.0.0.1", + HostIP: "127.0.0.1", + Port: port, + })) + + assert.True(t, existingPanelDetected(ctx, panelInstallStateV4{Port: port})) +} + +func Test_panelProbeHosts(t *testing.T) { + tests := []struct { + name string + state gameapctl.PanelInstallState + expected []string + }{ + { + name: "empty_state_falls_back_to_loopback", + state: gameapctl.PanelInstallState{}, + expected: []string{"127.0.0.1"}, + }, + { + name: "ip_host_is_not_duplicated", + state: gameapctl.PanelInstallState{Host: "2.29.29.94", HostIP: "2.29.29.94"}, + expected: []string{"2.29.29.94", "127.0.0.1"}, + }, + { + name: "domain_host_then_its_ip", + state: gameapctl.PanelInstallState{Host: "panel.example.com", HostIP: "10.0.0.5"}, + expected: []string{"panel.example.com", "10.0.0.5", "127.0.0.1"}, + }, + { + name: "loopback_host_once", + state: gameapctl.PanelInstallState{Host: "127.0.0.1"}, + expected: []string{"127.0.0.1"}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, panelProbeHosts(test.state)) + }) + } } func Test_checkPortAvailabilityV4_ReplacesOccupiedDefaultPort(t *testing.T) { diff --git a/internal/actions/panel/install/checkers_v4.go b/internal/actions/panel/install/checkers_v4.go index 7ef1cae..88c2182 100644 --- a/internal/actions/panel/install/checkers_v4.go +++ b/internal/actions/panel/install/checkers_v4.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "runtime" + "slices" "strings" "syscall" "time" @@ -57,7 +58,7 @@ func checkPortAvailabilityV4(ctx context.Context, state panelInstallStateV4) (pa state.Port = defaultPortForScope(state.Scope) } - if existingPanelDetected(ctx, state.Port) { + if existingPanelDetected(ctx, state) { fmt.Println("Existing GameAP panel detected on port", state.Port) return state, nil @@ -114,42 +115,62 @@ func checkPortAvailabilityV4(ctx context.Context, state panelInstallStateV4) (pa return state, nil } +const loopbackHost = "127.0.0.1" + // existingPanelDetected reports whether the panel of a previous installation already // answers on the port. This is common during re-installation, and such a port must not be -// treated as occupied. A previous installation on the very same port is required as well: -// any local service can answer /health with 200, and trusting one of those would leave the -// panel configured for a port it cannot bind. -func existingPanelDetected(ctx context.Context, port string) bool { - if !previouslyInstalledOnPort(ctx, port) { +// treated as occupied. A previous installation of the same scope on the very same port is +// required as well: any local service can answer with 200, and trusting one of those would +// leave the panel configured for a port it cannot bind. +// +// The previous panel listens on its configured HTTP_HOST, usually the public address rather +// than loopback, so that address is probed first. A panel that already redirects plain HTTP +// to HTTPS is not recognised here. +func existingPanelDetected(ctx context.Context, state panelInstallStateV4) bool { + prev, ok := previousInstallation(ctx, state.Scope) + if !ok || prev.Port != state.Port { return false } - client := &http.Client{Timeout: 2 * time.Second} - - healthURL := fmt.Sprintf("http://127.0.0.1:%s/health", port) - healthReq, err := http.NewRequestWithContext(ctx, http.MethodGet, healthURL, nil) - if err != nil { - return false + for _, host := range panelProbeHosts(prev) { + if checkPanelHealth(ctx, host, state.Port, panelProbeTimeout) == nil { + return true + } } - resp, err := client.Do(healthReq) - if err != nil { - return false + return false +} + +// panelProbeHosts lists the addresses a previous installation may answer on, most +// specific first: the configured host, its resolved IP, then loopback. +func panelProbeHosts(prev gameapctl.PanelInstallState) []string { + candidates := []string{prev.Host, prev.HostIP, loopbackHost} + + hosts := make([]string, 0, len(candidates)) + for _, host := range candidates { + if host == "" || slices.Contains(hosts, host) { + continue + } + + hosts = append(hosts, host) } - defer func() { - _ = resp.Body.Close() - }() - return resp.StatusCode == http.StatusOK + return hosts } -func previouslyInstalledOnPort(ctx context.Context, port string) bool { - prevState, err := gameapctl.LoadPanelInstallState(ctx) - if err != nil { - return false +// previousInstallation loads the state of a previous v4 installation of the same scope. +// A panel of the other scope runs from different paths and is never replaced by this one. +func previousInstallation(ctx context.Context, scope string) (gameapctl.PanelInstallState, bool) { + prev, err := gameapctl.LoadPanelInstallState(ctx) + if err != nil || !isPrevStateV4(prev.Version) { + return gameapctl.PanelInstallState{}, false + } + + if gameap.ScopeOrDefault(prev.Scope) != gameap.ScopeOrDefault(scope) { + return gameapctl.PanelInstallState{}, false } - return isPrevStateV4(prevState.Version) && prevState.Port == port + return prev, true } func checkHTTPHostAvailabilityV4(ctx context.Context, state panelInstallStateV4) (panelInstallStateV4, error) { diff --git a/internal/actions/panel/install/health_internal_test.go b/internal/actions/panel/install/health_internal_test.go new file mode 100644 index 0000000..b6b043c --- /dev/null +++ b/internal/actions/panel/install/health_internal_test.go @@ -0,0 +1,133 @@ +package install + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const ( + healthOKBody = `{"status":"ok"}` + + testRetryDelay = time.Millisecond + testLongRetryDelay = 10 * time.Second + testCancelAfter = 50 * time.Millisecond + testPromptReturn = time.Second +) + +// servePanelHealth runs a fake panel and returns the port it listens on (127.0.0.1). +func servePanelHealth(t *testing.T, handler http.HandlerFunc) string { + t.Helper() + + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + + _, port, err := net.SplitHostPort(strings.TrimPrefix(server.URL, "http://")) + require.NoError(t, err) + + return port +} + +// panelHealthHandler answers like GameAP v4: JSON on /api/health, 404 elsewhere. +func panelHealthHandler(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/health" { + http.NotFound(w, r) + + return + } + + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(healthOKBody)) +} + +// spaFallbackHandler answers every path with index.html, as the panel does for unknown routes. +func spaFallbackHandler(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write([]byte("GameAP")) +} + +func unavailableHandler(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) +} + +func localPanelState(port string) panelInstallStateV4 { + return panelInstallStateV4{Host: "127.0.0.1", Port: port} +} + +func Test_waitForPanelHealthCheck_Succeeds(t *testing.T) { + port := servePanelHealth(t, panelHealthHandler) + + err := waitForPanelHealthCheck(context.Background(), localPanelState(port), 1, testRetryDelay) + + require.NoError(t, err) +} + +func Test_waitForPanelHealthCheck_RetriesUntilReady(t *testing.T) { + var mu sync.Mutex + calls := 0 + + port := servePanelHealth(t, func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + calls++ + attempt := calls + mu.Unlock() + + if attempt == 1 { + unavailableHandler(w, r) + + return + } + + panelHealthHandler(w, r) + }) + + err := waitForPanelHealthCheck(context.Background(), localPanelState(port), healthCheckRetries, testRetryDelay) + + require.NoError(t, err) + + mu.Lock() + defer mu.Unlock() + require.Equal(t, 2, calls) +} + +func Test_waitForPanelHealthCheck_RejectsSPAFallback(t *testing.T) { + port := servePanelHealth(t, spaFallbackHandler) + + err := waitForPanelHealthCheck(context.Background(), localPanelState(port), 1, testRetryDelay) + + require.ErrorContains(t, err, errPanelNotReady.Error()) +} + +func Test_waitForPanelHealthCheck_ReturnsPromptlyWithCancelledContext(t *testing.T) { + port := servePanelHealth(t, unavailableHandler) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + started := time.Now() + err := waitForPanelHealthCheck(ctx, localPanelState(port), healthCheckRetries, healthCheckInterval) + + require.ErrorIs(t, err, context.Canceled) + require.Less(t, time.Since(started), testPromptReturn) +} + +func Test_waitForPanelHealthCheck_StopsWaitingWhenContextIsCancelled(t *testing.T) { + port := servePanelHealth(t, unavailableHandler) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + time.AfterFunc(testCancelAfter, cancel) + + started := time.Now() + err := waitForPanelHealthCheck(ctx, localPanelState(port), healthCheckRetries, testLongRetryDelay) + + require.ErrorIs(t, err, context.Canceled) + require.Less(t, time.Since(started), testPromptReturn) +} diff --git a/internal/actions/panel/install/health_v4.go b/internal/actions/panel/install/health_v4.go new file mode 100644 index 0000000..a090749 --- /dev/null +++ b/internal/actions/panel/install/health_v4.go @@ -0,0 +1,67 @@ +package install + +import ( + "context" + "net" + "time" + + panelpkg "github.com/gameap/gameapctl/internal/pkg/panel" + "github.com/pkg/errors" +) + +const ( + healthCheckRetries = 30 + healthCheckInterval = 2 * time.Second + httpClientTimeout = 5 * time.Second + panelProbeTimeout = 2 * time.Second +) + +var errPanelNotReady = errors.New("GameAP panel failed to become ready in time") + +// checkPanelHealth asks /api/health of the panel once. Unlike the SPA fallback, which +// answers any path with 200, the endpoint reports the panel and its database as ready. +func checkPanelHealth(ctx context.Context, host, port string, timeout time.Duration) error { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + return panelpkg.CheckInstallationV4(ctx, host, port, false) +} + +// waitForPanelHealthCheck polls the panel until it answers, the context is cancelled or +// maxRetries attempts are exhausted. Only the last case is a start failure, so the +// service diagnostics are collected only then. +func waitForPanelHealthCheck( + ctx context.Context, + state panelInstallStateV4, + maxRetries int, + retryDelay time.Duration, +) error { + address := net.JoinHostPort(state.Host, state.Port) + + var lastErr error + + for i := 0; i < maxRetries; i++ { + lastErr = checkPanelHealth(ctx, state.Host, state.Port, httpClientTimeout) + if lastErr == nil { + return nil + } + + if ctx.Err() != nil { + return errors.Wrapf(ctx.Err(), "%s, %s", errPanelNotReady, address) + } + + if i == maxRetries-1 { + break + } + + select { + case <-ctx.Done(): + return errors.Wrapf(ctx.Err(), "%s, %s", errPanelNotReady, address) + case <-time.After(retryDelay): + } + } + + logPanelStartDiagnosticsOnce(ctx, state) + + return errors.WithMessagef(lastErr, "%s, %s", errPanelNotReady, address) +} diff --git a/internal/actions/panel/install/panel_install_v4.go b/internal/actions/panel/install/panel_install_v4.go index 9108d43..9162261 100644 --- a/internal/actions/panel/install/panel_install_v4.go +++ b/internal/actions/panel/install/panel_install_v4.go @@ -335,6 +335,10 @@ func HandleV4(cliCtx *cli.Context) error { return errors.WithMessage(err, "failed to check host") } + if err = stopPreviousPanelV4(ctx, state, panel.Stop); err != nil { + return err + } + state, err = checkPortAvailabilityV4(ctx, state) if err != nil { return errors.WithMessage(err, "failed to check port availability") @@ -436,11 +440,14 @@ func HandleV4(cliCtx *cli.Context) error { if state.WithDaemon { state, err = daemonInstallV4(ctx, state) - if err != nil { + switch { + case err == nil: + daemonInstalled = true + case ctx.Err() != nil: + return errors.Wrap(ctx.Err(), "installation interrupted") + default: fmt.Println("Failed to install daemon: ", err.Error()) log.Println(errors.WithMessage(err, "failed to install daemon, try to install it manually")) - } else { - daemonInstalled = true } } @@ -1349,13 +1356,8 @@ const ( daemonSetupTokenEnv = "DAEMON_SETUP_TOKEN" daemonSetupKeyEnv = "DAEMON_SETUP_KEY" setupKeyByteLen = 16 - healthCheckRetries = 30 - healthCheckInterval = 2 * time.Second - httpClientTimeout = 5 * time.Second ) -var errPanelNotReady = errors.New("GameAP panel failed to become ready in time") - func daemonInstallV4Legacy(ctx context.Context, state panelInstallStateV4) (panelInstallStateV4, error) { token := fmt.Sprintf("gameapctl%d", time.Now().UnixMilli()) @@ -1566,56 +1568,51 @@ func removeConfigEnvVar(configPath, name string) { } } -func waitForPanelHealthCheck( +// stopPreviousPanelV4 stops the panel of a previous installation. Its binary is about to +// be replaced and a running executable cannot be overwritten, while panel.Install only +// writes and enables the unit and never touches the process. Stopping before the port +// check also keeps the port: an occupant that is our own panel must not trigger the +// fallback port. +func stopPreviousPanelV4( ctx context.Context, state panelInstallStateV4, - maxRetries int, - retryDelay time.Duration, + stop func(context.Context, ...panel.Options) error, ) error { - client := &http.Client{ - Timeout: httpClientTimeout, + if !previousPanelInstalled(state) { + return nil } - healthCheckURL := fmt.Sprintf("http://%s:%s/health", state.Host, state.Port) - - var lastErr error - - for i := 0; i < maxRetries; i++ { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthCheckURL, nil) - if err != nil { - return errors.WithMessage(err, "failed to create health check request") - } - - resp, err := client.Do(req) - switch { - case err != nil: - lastErr = err - case resp.StatusCode == http.StatusOK: - resp.Body.Close() + fmt.Println("Stopping GameAP of the previous installation ...") + log.Println("Stopping GameAP of the previous installation") - return nil - default: - lastErr = errors.Errorf("unexpected status code %d", resp.StatusCode) - } + err := stop(ctx, panel.Options{Scope: state.Scope}) + if err == nil { + return nil + } - if resp != nil { - resp.Body.Close() - } + var notFoundErr *service.NotFoundError + if errors.Is(err, panel.ErrGameAPNotInstalled) || + errors.Is(err, service.ErrInactiveService) || + errors.As(err, ¬FoundErr) { + log.Println(errors.WithMessage(err, "GameAP of the previous installation is not running")) - if i == maxRetries-1 { - logPanelStartDiagnosticsOnce(ctx, state) + return nil + } - if lastErr == nil { - lastErr = errPanelNotReady - } + return errors.WithMessage(err, "failed to stop GameAP of the previous installation") +} - return errors.WithMessagef(lastErr, "%s, %s", errPanelNotReady, healthCheckURL) - } +func previousPanelInstalled(state panelInstallStateV4) bool { + if utils.IsFileExists(state.BinaryPath) { + return true + } - time.Sleep(retryDelay) + paths, err := gameap.PanelPathsForScope(state.Scope) + if err != nil { + return false } - return nil + return utils.IsFileExists(paths.SystemdUnitPath) } func updateAdminPasswordv4(ctx context.Context, state panelInstallStateV4) (panelInstallStateV4, error) { diff --git a/internal/actions/panel/install/stop_previous_v4_internal_test.go b/internal/actions/panel/install/stop_previous_v4_internal_test.go new file mode 100644 index 0000000..62c4008 --- /dev/null +++ b/internal/actions/panel/install/stop_previous_v4_internal_test.go @@ -0,0 +1,95 @@ +package install + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/gameap/gameapctl/pkg/gameap" + "github.com/gameap/gameapctl/pkg/panel" + "github.com/gameap/gameapctl/pkg/service" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" +) + +// userScopeState points every previous-installation marker into a temporary home. +func userScopeState(t *testing.T) panelInstallStateV4 { + t.Helper() + + useTemporaryStateDirectory(t) + + return panelInstallStateV4{ + Scope: gameap.ScopeUser, + BinaryPath: filepath.Join(t.TempDir(), "gameap"), + } +} + +func Test_stopPreviousPanelV4_SkipsWhenNothingInstalled(t *testing.T) { + state := userScopeState(t) + + called := false + stop := func(context.Context, ...panel.Options) error { + called = true + + return nil + } + + require.NoError(t, stopPreviousPanelV4(context.Background(), state, stop)) + require.False(t, called) +} + +func Test_stopPreviousPanelV4_StopsWhenBinaryExists(t *testing.T) { + state := userScopeState(t) + require.NoError(t, os.WriteFile(state.BinaryPath, []byte("binary"), 0600)) + + var scopes []string + stop := func(_ context.Context, opts ...panel.Options) error { + require.Len(t, opts, 1) + scopes = append(scopes, opts[0].Scope) + + return nil + } + + require.NoError(t, stopPreviousPanelV4(context.Background(), state, stop)) + require.Equal(t, []string{gameap.ScopeUser}, scopes) +} + +func Test_stopPreviousPanelV4_ToleratesNotRunningPanel(t *testing.T) { + tests := []struct { + name string + err error + }{ + {name: "not_installed", err: panel.ErrGameAPNotInstalled}, + {name: "inactive_service", err: service.ErrInactiveService}, + {name: "service_not_found", err: service.NewNotFoundError("gameap")}, + {name: "wrapped_not_found", err: errors.WithMessage(service.NewNotFoundError("gameap"), "stop")}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + state := userScopeState(t) + require.NoError(t, os.WriteFile(state.BinaryPath, []byte("binary"), 0600)) + + stop := func(context.Context, ...panel.Options) error { + return test.err + } + + require.NoError(t, stopPreviousPanelV4(context.Background(), state, stop)) + }) + } +} + +func Test_stopPreviousPanelV4_ReturnsStopError(t *testing.T) { + state := userScopeState(t) + require.NoError(t, os.WriteFile(state.BinaryPath, []byte("binary"), 0600)) + + stopErr := errors.New("systemctl failed") + stop := func(context.Context, ...panel.Options) error { + return stopErr + } + + err := stopPreviousPanelV4(context.Background(), state, stop) + + require.ErrorIs(t, err, stopErr) + require.ErrorContains(t, err, "failed to stop GameAP of the previous installation") +} diff --git a/internal/pkg/panel/install.go b/internal/pkg/panel/install.go index 4c336d8..d4fa74c 100644 --- a/internal/pkg/panel/install.go +++ b/internal/pkg/panel/install.go @@ -11,6 +11,7 @@ import ( "net/http" "net/url" "os" + "strings" "github.com/gameap/gameapctl/pkg/gameap" "github.com/gameap/gameapctl/pkg/oscore" @@ -192,6 +193,10 @@ func CheckInstallationV4(ctx context.Context, host, port string, https bool) err } func createHealthURL(host, port string, https bool, endpoint string) string { + if strings.Contains(host, ":") { + host = "[" + strings.Trim(host, "[]") + "]" + } + hostPort := host if port != "80" && port != "443" { hostPort = host + ":" + port diff --git a/internal/pkg/panel/install_internal_test.go b/internal/pkg/panel/install_internal_test.go new file mode 100644 index 0000000..437ecae --- /dev/null +++ b/internal/pkg/panel/install_internal_test.go @@ -0,0 +1,29 @@ +package panel + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_createHealthURL(t *testing.T) { + tests := []struct { + name string + host string + port string + https bool + expected string + }{ + {name: "default_http_port", host: "127.0.0.1", port: "80", expected: "http://127.0.0.1/api/health"}, + {name: "custom_port", host: "127.0.0.1", port: "8025", expected: "http://127.0.0.1:8025/api/health"}, + {name: "https_default_port", host: "example.com", port: "443", https: true, expected: "https://example.com/api/health"}, + {name: "ipv6_custom_port", host: "::1", port: "8025", expected: "http://[::1]:8025/api/health"}, + {name: "ipv6_default_port", host: "2a01:4f9:c015:fafb::1", port: "80", expected: "http://[2a01:4f9:c015:fafb::1]/api/health"}, + {name: "bracketed_ipv6", host: "[::1]", port: "8025", expected: "http://[::1]:8025/api/health"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, createHealthURL(test.host, test.port, test.https, "/api/health")) + }) + } +} diff --git a/pkg/panel/install.go b/pkg/panel/install.go index 761c870..d918499 100644 --- a/pkg/panel/install.go +++ b/pkg/panel/install.go @@ -400,6 +400,11 @@ func downloadBinaries(ctx context.Context, config InstallConfig) (string, error) if err != nil { return "", errors.WithMessage(err, "failed to make temp dir") } + defer func() { + if removeErr := os.RemoveAll(tmpDir); removeErr != nil { + log.Printf("Failed to remove temp dir %s: %v\n", tmpDir, removeErr) + } + }() release := config.PreResolvedRelease if release == nil { @@ -438,13 +443,9 @@ func downloadBinaries(ctx context.Context, config InstallConfig) (string, error) return "", errors.WithMessage(err, "failed to stat file") } - err = utils.Move(fp, config.BinaryPath) + err = utils.ReplaceFile(fp, config.BinaryPath, 0755) if err != nil { - return "", errors.WithMessage(err, "failed to move gameap binaries") - } - - if err = os.Chmod(config.BinaryPath, 0755); err != nil { - return "", errors.Wrap(err, "failed to set executable permissions") + return "", errors.WithMessage(err, "failed to install gameap binary") } binariesInstalled = true diff --git a/pkg/utils/fs.go b/pkg/utils/fs.go index 8e9a4d0..1cf7fd8 100644 --- a/pkg/utils/fs.go +++ b/pkg/utils/fs.go @@ -74,6 +74,35 @@ func Copy(src string, dst string) error { return copy.Copy(src, dst) } +// ReplaceFile moves src over dst without truncating dst in place: the file is staged +// next to dst and renamed into it, so a process still executing dst keeps its old +// inode and no reader ever sees a half-written file. +func ReplaceFile(src, dst string, mode os.FileMode) error { + staging := dst + ".new" + + if err := os.Remove(staging); err != nil && !errors.Is(err, fs.ErrNotExist) { + return errors.Wrapf(err, "failed to remove stale staging file %s", staging) + } + + if err := Move(src, staging); err != nil { + return errors.WithMessagef(err, "failed to stage %s", dst) + } + + if err := os.Chmod(staging, mode); err != nil { + _ = os.Remove(staging) + + return errors.Wrapf(err, "failed to set permissions on %s", staging) + } + + if err := os.Rename(staging, dst); err != nil { + _ = os.Remove(staging) + + return errors.Wrapf(err, "failed to replace %s", dst) + } + + return nil +} + func WriteContentsToFile(contents []byte, path string) error { file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0644) if err != nil { diff --git a/pkg/utils/fs_linux_test.go b/pkg/utils/fs_linux_test.go new file mode 100644 index 0000000..aed7c01 --- /dev/null +++ b/pkg/utils/fs_linux_test.go @@ -0,0 +1,51 @@ +//go:build linux + +package utils_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "syscall" + "testing" + + "github.com/gameap/gameapctl/pkg/utils" + "github.com/stretchr/testify/require" +) + +// A binary that is being executed cannot be opened for writing (ETXTBSY), which is how +// a re-installation over a running panel used to fail. Renaming a staged file over it works. +func Test_ReplaceFile_ReplacesRunningExecutable(t *testing.T) { + sleepBinary, err := exec.LookPath("sleep") + if err != nil { + t.Skip("sleep binary is not available") + } + + dir := t.TempDir() + running := filepath.Join(dir, "running") + + original, err := os.ReadFile(sleepBinary) + require.NoError(t, err) + require.NoError(t, os.WriteFile(running, original, 0755)) + + ctx, cancel := context.WithCancel(context.Background()) + cmd := exec.CommandContext(ctx, running, "60") + require.NoError(t, cmd.Start()) + t.Cleanup(func() { + cancel() + _ = cmd.Wait() + }) + + _, err = os.OpenFile(running, os.O_WRONLY, 0) + require.ErrorIs(t, err, syscall.ETXTBSY) + + src := filepath.Join(dir, "src") + require.NoError(t, os.WriteFile(src, []byte("replacement"), 0600)) + + require.NoError(t, utils.ReplaceFile(src, running, 0755)) + + content, err := os.ReadFile(running) + require.NoError(t, err) + require.Equal(t, "replacement", string(content)) +} diff --git a/pkg/utils/fs_test.go b/pkg/utils/fs_test.go index 51a3af4..d9c648a 100644 --- a/pkg/utils/fs_test.go +++ b/pkg/utils/fs_test.go @@ -3,6 +3,7 @@ package utils_test import ( "os" "path/filepath" + "runtime" "strconv" "strings" "testing" @@ -112,3 +113,64 @@ func Test_TailFile_NotExistingFile(t *testing.T) { require.Error(t, err) } + +func Test_ReplaceFile_ReplacesExistingFile(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "src") + dst := filepath.Join(dir, "dst") + require.NoError(t, os.WriteFile(src, []byte("new"), 0600)) + require.NoError(t, os.WriteFile(dst, []byte("old"), 0600)) + + require.NoError(t, utils.ReplaceFile(src, dst, 0755)) + + content, err := os.ReadFile(dst) + require.NoError(t, err) + require.Equal(t, "new", string(content)) + + if runtime.GOOS != "windows" { + info, err := os.Stat(dst) + require.NoError(t, err) + require.Equal(t, os.FileMode(0755), info.Mode().Perm()) + } + + require.NoFileExists(t, src) + require.NoFileExists(t, dst+".new") +} + +func Test_ReplaceFile_CreatesMissingDestination(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "src") + dst := filepath.Join(dir, "bin", "dst") + require.NoError(t, os.WriteFile(src, []byte("new"), 0600)) + + require.NoError(t, utils.ReplaceFile(src, dst, 0755)) + + content, err := os.ReadFile(dst) + require.NoError(t, err) + require.Equal(t, "new", string(content)) + require.NoFileExists(t, dst+".new") +} + +func Test_ReplaceFile_RemovesStaleStagingFile(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "src") + dst := filepath.Join(dir, "dst") + require.NoError(t, os.WriteFile(src, []byte("new"), 0600)) + require.NoError(t, os.WriteFile(dst+".new", []byte("stale"), 0600)) + + require.NoError(t, utils.ReplaceFile(src, dst, 0755)) + + content, err := os.ReadFile(dst) + require.NoError(t, err) + require.Equal(t, "new", string(content)) + require.NoFileExists(t, dst+".new") +} + +func Test_ReplaceFile_MissingSource(t *testing.T) { + dir := t.TempDir() + + err := utils.ReplaceFile(filepath.Join(dir, "missing"), filepath.Join(dir, "dst"), 0755) + + require.Error(t, err) + require.NoFileExists(t, filepath.Join(dir, "dst")) +} From 303225e95dd4179a303ef91289d17513d62927c8 Mon Sep 17 00:00:00 2001 From: et-nik Date: Wed, 2 Sep 2026 15:43:06 +0200 Subject: [PATCH 2/2] review --- .../actions/panel/install/panel_install_v4.go | 18 +++---- internal/pkg/panel/install.go | 14 ++--- internal/pkg/panel/install_internal_test.go | 2 + pkg/utils/fs.go | 23 ++++++-- pkg/utils/fs_test.go | 54 +++++++++++++++---- 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/internal/actions/panel/install/panel_install_v4.go b/internal/actions/panel/install/panel_install_v4.go index 9162261..48ed902 100644 --- a/internal/actions/panel/install/panel_install_v4.go +++ b/internal/actions/panel/install/panel_install_v4.go @@ -335,10 +335,6 @@ func HandleV4(cliCtx *cli.Context) error { return errors.WithMessage(err, "failed to check host") } - if err = stopPreviousPanelV4(ctx, state, panel.Stop); err != nil { - return err - } - state, err = checkPortAvailabilityV4(ctx, state) if err != nil { return errors.WithMessage(err, "failed to check port availability") @@ -422,6 +418,10 @@ func HandleV4(cliCtx *cli.Context) error { saveStateCheckpointV4(cliCtx.Context, state) + if err = stopPreviousPanelV4(ctx, state, panel.Stop); err != nil { + return err + } + fmt.Println("Installing GameAP ...") if state.FromGithub { @@ -1568,11 +1568,11 @@ func removeConfigEnvVar(configPath, name string) { } } -// stopPreviousPanelV4 stops the panel of a previous installation. Its binary is about to -// be replaced and a running executable cannot be overwritten, while panel.Install only -// writes and enables the unit and never touches the process. Stopping before the port -// check also keeps the port: an occupant that is our own panel must not trigger the -// fallback port. +// stopPreviousPanelV4 stops the panel of a previous installation right before its binary +// is replaced: a running executable cannot be overwritten, and panel.Install only writes +// and enables the unit, it never touches the process. It runs after every preflight step, +// so a failure in those leaves the previous panel up; the port check recognises the +// running panel through existingPanelDetected. func stopPreviousPanelV4( ctx context.Context, state panelInstallStateV4, diff --git a/internal/pkg/panel/install.go b/internal/pkg/panel/install.go index d4fa74c..ba100db 100644 --- a/internal/pkg/panel/install.go +++ b/internal/pkg/panel/install.go @@ -197,17 +197,17 @@ func createHealthURL(host, port string, https bool, endpoint string) string { host = "[" + strings.Trim(host, "[]") + "]" } - hostPort := host - if port != "80" && port != "443" { - hostPort = host + ":" + port + scheme, defaultPort := "http", "80" + if https { + scheme, defaultPort = "https", "443" } - u := "http://" + hostPort + endpoint - if https { - u = "https://" + hostPort + endpoint + hostPort := host + if port != defaultPort { + hostPort = host + ":" + port } - return u + return scheme + "://" + hostPort + endpoint } // localHTTPSClient deliberately does not verify the panel's certificate. An diff --git a/internal/pkg/panel/install_internal_test.go b/internal/pkg/panel/install_internal_test.go index 437ecae..03a4c58 100644 --- a/internal/pkg/panel/install_internal_test.go +++ b/internal/pkg/panel/install_internal_test.go @@ -17,6 +17,8 @@ func Test_createHealthURL(t *testing.T) { {name: "default_http_port", host: "127.0.0.1", port: "80", expected: "http://127.0.0.1/api/health"}, {name: "custom_port", host: "127.0.0.1", port: "8025", expected: "http://127.0.0.1:8025/api/health"}, {name: "https_default_port", host: "example.com", port: "443", https: true, expected: "https://example.com/api/health"}, + {name: "http_on_443_keeps_port", host: "127.0.0.1", port: "443", expected: "http://127.0.0.1:443/api/health"}, + {name: "https_on_80_keeps_port", host: "example.com", port: "80", https: true, expected: "https://example.com:80/api/health"}, {name: "ipv6_custom_port", host: "::1", port: "8025", expected: "http://[::1]:8025/api/health"}, {name: "ipv6_default_port", host: "2a01:4f9:c015:fafb::1", port: "80", expected: "http://[2a01:4f9:c015:fafb::1]/api/health"}, {name: "bracketed_ipv6", host: "[::1]", port: "8025", expected: "http://[::1]:8025/api/health"}, diff --git a/pkg/utils/fs.go b/pkg/utils/fs.go index 1cf7fd8..d182b67 100644 --- a/pkg/utils/fs.go +++ b/pkg/utils/fs.go @@ -75,16 +75,29 @@ func Copy(src string, dst string) error { } // ReplaceFile moves src over dst without truncating dst in place: the file is staged -// next to dst and renamed into it, so a process still executing dst keeps its old -// inode and no reader ever sees a half-written file. +// under a unique name next to dst and renamed into it, so a process still executing dst +// keeps its old inode and no reader ever sees a half-written file. func ReplaceFile(src, dst string, mode os.FileMode) error { - staging := dst + ".new" + dstDir := filepath.Dir(dst) + if err := os.MkdirAll(dstDir, 0755); err != nil { + return errors.Wrapf(err, "failed to create destination directory %s", dstDir) + } - if err := os.Remove(staging); err != nil && !errors.Is(err, fs.ErrNotExist) { - return errors.Wrapf(err, "failed to remove stale staging file %s", staging) + stagingFile, err := os.CreateTemp(dstDir, filepath.Base(dst)+".*.new") + if err != nil { + return errors.Wrapf(err, "failed to create staging file for %s", dst) + } + + staging := stagingFile.Name() + if err := stagingFile.Close(); err != nil { + _ = os.Remove(staging) + + return errors.Wrapf(err, "failed to close staging file %s", staging) } if err := Move(src, staging); err != nil { + _ = os.Remove(staging) + return errors.WithMessagef(err, "failed to stage %s", dst) } diff --git a/pkg/utils/fs_test.go b/pkg/utils/fs_test.go index d9c648a..4eb25fe 100644 --- a/pkg/utils/fs_test.go +++ b/pkg/utils/fs_test.go @@ -6,6 +6,7 @@ import ( "runtime" "strconv" "strings" + "sync" "testing" "github.com/gameap/gameapctl/pkg/utils" @@ -134,7 +135,7 @@ func Test_ReplaceFile_ReplacesExistingFile(t *testing.T) { } require.NoFileExists(t, src) - require.NoFileExists(t, dst+".new") + requireNoStagingFiles(t, dir) } func Test_ReplaceFile_CreatesMissingDestination(t *testing.T) { @@ -148,22 +149,46 @@ func Test_ReplaceFile_CreatesMissingDestination(t *testing.T) { content, err := os.ReadFile(dst) require.NoError(t, err) require.Equal(t, "new", string(content)) - require.NoFileExists(t, dst+".new") + requireNoStagingFiles(t, filepath.Dir(dst)) } -func Test_ReplaceFile_RemovesStaleStagingFile(t *testing.T) { +func Test_ReplaceFile_ConcurrentReplacements(t *testing.T) { dir := t.TempDir() - src := filepath.Join(dir, "src") dst := filepath.Join(dir, "dst") - require.NoError(t, os.WriteFile(src, []byte("new"), 0600)) - require.NoError(t, os.WriteFile(dst+".new", []byte("stale"), 0600)) + require.NoError(t, os.WriteFile(dst, []byte("old"), 0600)) - require.NoError(t, utils.ReplaceFile(src, dst, 0755)) + const writers = 8 - content, err := os.ReadFile(dst) + contents := make(map[string]struct{}, writers) + for i := 0; i < writers; i++ { + contents[strconv.Itoa(i)] = struct{}{} + } + + errs := make(chan error, writers) + + var wg sync.WaitGroup + for content := range contents { + src := filepath.Join(dir, "src-"+content) + require.NoError(t, os.WriteFile(src, []byte(content), 0600)) + + wg.Add(1) + go func() { + defer wg.Done() + + errs <- utils.ReplaceFile(src, dst, 0755) + }() + } + wg.Wait() + close(errs) + + for err := range errs { + require.NoError(t, err) + } + + got, err := os.ReadFile(dst) require.NoError(t, err) - require.Equal(t, "new", string(content)) - require.NoFileExists(t, dst+".new") + require.Contains(t, contents, string(got)) + requireNoStagingFiles(t, dir) } func Test_ReplaceFile_MissingSource(t *testing.T) { @@ -173,4 +198,13 @@ func Test_ReplaceFile_MissingSource(t *testing.T) { require.Error(t, err) require.NoFileExists(t, filepath.Join(dir, "dst")) + requireNoStagingFiles(t, dir) +} + +func requireNoStagingFiles(t *testing.T, dir string) { + t.Helper() + + leftovers, err := filepath.Glob(filepath.Join(dir, "*.new")) + require.NoError(t, err) + require.Empty(t, leftovers) }