diff --git a/cmd/thv/app/commands.go b/cmd/thv/app/commands.go index 05ecd3593e..dbe0f33f2d 100644 --- a/cmd/thv/app/commands.go +++ b/cmd/thv/app/commands.go @@ -122,6 +122,12 @@ func IsInformationalCommand(args []string) bool { "ai-plugin": true, "vmcp": true, "llm": true, + + // Help flags: running "thv --help" (flag-first) or "thv help" must not + // trigger container runtime startup or migrations. + "--help": true, + "-h": true, + "help": true, } return informationalCommands[command] diff --git a/pkg/migration/migration.go b/pkg/migration/migration.go index 8b9d781a32..9244d5d018 100644 --- a/pkg/migration/migration.go +++ b/pkg/migration/migration.go @@ -6,6 +6,7 @@ package migration import ( "context" + "errors" "log/slog" "github.com/stacklok/toolhive/pkg/container/runtime" @@ -18,6 +19,8 @@ import ( // In Kubernetes environments this is always a no-op: MCPGroup CRDs are // operator/user-managed resources and the caller's service account may not // have create permission on them. +// When multiple processes race to create the group, the loser's conflict +// error is treated as success because the desired end state (group exists) holds. func EnsureDefaultGroupExists() error { if runtime.IsKubernetesRuntime() { return nil @@ -40,5 +43,13 @@ func ensureDefaultGroupExists(ctx context.Context) error { } slog.Debug("creating default group", "name", groups.DefaultGroupName) - return groupManager.Create(ctx, groups.DefaultGroupName) + if err := groupManager.Create(ctx, groups.DefaultGroupName); err != nil { + // Another process may have won the creation race; the group now + // exists, which is the desired end state. + if errors.Is(err, groups.ErrGroupAlreadyExists) { + return nil + } + return err + } + return nil } diff --git a/pkg/migration/migration_test.go b/pkg/migration/migration_test.go new file mode 100644 index 0000000000..d9283698c9 --- /dev/null +++ b/pkg/migration/migration_test.go @@ -0,0 +1,86 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package migration + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/adrg/xdg" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/groups" +) + +// setupIsolatedState points XDG_STATE_HOME at a fresh temp directory so tests +// never touch the developer's real state dir. xdg caches StateHome at package +// init, so t.Setenv alone is not enough: xdg.Reload() must be called to re-read +// the env var, and again on cleanup to restore the cached value. +func setupIsolatedState(t *testing.T) string { + t.Helper() + tmpBase := t.TempDir() + t.Setenv("XDG_STATE_HOME", tmpBase) + xdg.Reload() + t.Cleanup(xdg.Reload) + return tmpBase +} + +//nolint:paralleltest // t.Setenv is incompatible with t.Parallel +func Test_EnsureDefaultGroupExistsConcurrent(t *testing.T) { + stateHome := setupIsolatedState(t) + + const goroutines = 8 + errCh := make(chan error, goroutines) + var wg sync.WaitGroup + wg.Add(goroutines) + for range goroutines { + go func() { + defer wg.Done() + errCh <- ensureDefaultGroupExists(context.Background()) + }() + } + + done := make(chan struct{}) + go func() { + wg.Wait() + close(done) + }() + + select { + case <-done: + case <-time.After(60 * time.Second): + t.Fatal("timed out waiting for concurrent ensureDefaultGroupExists calls") + } + + for range goroutines { + select { + case err := <-errCh: + assert.NoError(t, err) + default: + t.Fatal("not all goroutines reported a result") + } + } + + groupPath := filepath.Join(stateHome, "toolhive", "groups", string(groups.DefaultGroupName)+".json") + data, err := os.ReadFile(groupPath) //nolint:gosec // path is built from a test-controlled temp dir + require.NoError(t, err, "default group file should exist and be readable") + + var group groups.Group + require.NoError(t, json.Unmarshal(data, &group), "group file should contain complete valid JSON") + assert.Equal(t, string(groups.DefaultGroupName), group.Name) +} + +//nolint:paralleltest // t.Setenv is incompatible with t.Parallel +func Test_EnsureDefaultGroupExistsIdempotent(t *testing.T) { + setupIsolatedState(t) + + require.NoError(t, ensureDefaultGroupExists(context.Background())) + require.NoError(t, ensureDefaultGroupExists(context.Background())) +} diff --git a/test/e2e/helpers.go b/test/e2e/helpers.go index 1dccff283a..a77acdaf1b 100644 --- a/test/e2e/helpers.go +++ b/test/e2e/helpers.go @@ -366,9 +366,9 @@ func DebugServerState(config *TestConfig, serverName string) { // CheckTHVBinaryAvailable checks if the thv binary is available func CheckTHVBinaryAvailable(config *TestConfig) error { - _, _, err := NewTHVCommand(config, "--help").Run() + _, stderr, err := NewTHVCommand(config, "--help").Run() if err != nil { - return fmt.Errorf("thv binary not available at %s: %w", config.THVBinary, err) + return fmt.Errorf("thv binary not available at %s: %w (stderr: %s)", config.THVBinary, err, strings.TrimSpace(stderr)) } return nil }