From 1bb96b85287c778fb5c12a5c8547c1ca1629a1ea Mon Sep 17 00:00:00 2001 From: Vincent Vielle Date: Thu, 8 May 2025 15:49:57 +0200 Subject: [PATCH 01/24] fix: resolve flake test on manager (#17702) Fixes coder/internal#544 --------- Co-authored-by: Mathias Fredriksson --- coderd/notifications/manager.go | 110 +++++++++++++-------------- coderd/notifications/manager_test.go | 22 ++++++ 2 files changed, 74 insertions(+), 58 deletions(-) diff --git a/coderd/notifications/manager.go b/coderd/notifications/manager.go index ee85bd2d7a3c4..1a2c418a014bb 100644 --- a/coderd/notifications/manager.go +++ b/coderd/notifications/manager.go @@ -44,7 +44,6 @@ type Manager struct { store Store log slog.Logger - notifier *notifier handlers map[database.NotificationMethod]Handler method database.NotificationMethod helpers template.FuncMap @@ -53,11 +52,13 @@ type Manager struct { success, failure chan dispatchResult - runOnce sync.Once - stopOnce sync.Once - doneOnce sync.Once - stop chan any - done chan any + mu sync.Mutex // Protects following. + closed bool + notifier *notifier + + runOnce sync.Once + stop chan any + done chan any // clock is for testing only clock quartz.Clock @@ -138,7 +139,7 @@ func (m *Manager) WithHandlers(reg map[database.NotificationMethod]Handler) { // Manager requires system-level permissions to interact with the store. // Run is only intended to be run once. func (m *Manager) Run(ctx context.Context) { - m.log.Info(ctx, "started") + m.log.Debug(ctx, "notification manager started") m.runOnce.Do(func() { // Closes when Stop() is called or context is canceled. @@ -155,31 +156,26 @@ func (m *Manager) Run(ctx context.Context) { // events, creating a notifier, and publishing bulk dispatch result updates to the store. func (m *Manager) loop(ctx context.Context) error { defer func() { - m.doneOnce.Do(func() { - close(m.done) - }) - m.log.Info(context.Background(), "notification manager stopped") + close(m.done) + m.log.Debug(context.Background(), "notification manager stopped") }() - // Caught a terminal signal before notifier was created, exit immediately. - select { - case <-m.stop: - m.log.Warn(ctx, "gracefully stopped") - return xerrors.Errorf("gracefully stopped") - case <-ctx.Done(): - m.log.Error(ctx, "ungracefully stopped", slog.Error(ctx.Err())) - return xerrors.Errorf("notifications: %w", ctx.Err()) - default: + m.mu.Lock() + if m.closed { + m.mu.Unlock() + return xerrors.New("manager already closed") } var eg errgroup.Group - // Create a notifier to run concurrently, which will handle dequeueing and dispatching notifications. m.notifier = newNotifier(ctx, m.cfg, uuid.New(), m.log, m.store, m.handlers, m.helpers, m.metrics, m.clock) eg.Go(func() error { + // run the notifier which will handle dequeueing and dispatching notifications. return m.notifier.run(m.success, m.failure) }) + m.mu.Unlock() + // Periodically flush notification state changes to the store. eg.Go(func() error { // Every interval, collect the messages in the channels and bulk update them in the store. @@ -355,48 +351,46 @@ func (m *Manager) syncUpdates(ctx context.Context) { // Stop stops the notifier and waits until it has stopped. func (m *Manager) Stop(ctx context.Context) error { - var err error - m.stopOnce.Do(func() { - select { - case <-ctx.Done(): - err = ctx.Err() - return - default: - } + m.mu.Lock() + defer m.mu.Unlock() - m.log.Info(context.Background(), "graceful stop requested") + if m.closed { + return nil + } + m.closed = true - // If the notifier hasn't been started, we don't need to wait for anything. - // This is only really during testing when we want to enqueue messages only but not deliver them. - if m.notifier == nil { - m.doneOnce.Do(func() { - close(m.done) - }) - } else { - m.notifier.stop() - } + m.log.Debug(context.Background(), "graceful stop requested") + + // If the notifier hasn't been started, we don't need to wait for anything. + // This is only really during testing when we want to enqueue messages only but not deliver them. + if m.notifier != nil { + m.notifier.stop() + } - // Signal the stop channel to cause loop to exit. - close(m.stop) + // Signal the stop channel to cause loop to exit. + close(m.stop) - // Wait for the manager loop to exit or the context to be canceled, whichever comes first. - select { - case <-ctx.Done(): - var errStr string - if ctx.Err() != nil { - errStr = ctx.Err().Error() - } - // For some reason, slog.Error returns {} for a context error. - m.log.Error(context.Background(), "graceful stop failed", slog.F("err", errStr)) - err = ctx.Err() - return - case <-m.done: - m.log.Info(context.Background(), "gracefully stopped") - return - } - }) + if m.notifier == nil { + return nil + } - return err + m.mu.Unlock() // Unlock to avoid blocking loop. + defer m.mu.Lock() // Re-lock the mutex due to earlier defer. + + // Wait for the manager loop to exit or the context to be canceled, whichever comes first. + select { + case <-ctx.Done(): + var errStr string + if ctx.Err() != nil { + errStr = ctx.Err().Error() + } + // For some reason, slog.Error returns {} for a context error. + m.log.Error(context.Background(), "graceful stop failed", slog.F("err", errStr)) + return ctx.Err() + case <-m.done: + m.log.Debug(context.Background(), "gracefully stopped") + return nil + } } type dispatchResult struct { diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index 3eaebef7c9d0f..e9c309f0a09d3 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -182,6 +182,28 @@ func TestStopBeforeRun(t *testing.T) { }, testutil.WaitShort, testutil.IntervalFast) } +func TestRunStopRace(t *testing.T) { + t.Parallel() + + // SETUP + + // nolint:gocritic // Unit test. + ctx := dbauthz.AsSystemRestricted(testutil.Context(t, testutil.WaitMedium)) + store, ps := dbtestutil.NewDB(t) + logger := testutil.Logger(t) + + // GIVEN: a standard manager + mgr, err := notifications.NewManager(defaultNotificationsConfig(database.NotificationMethodSmtp), store, ps, defaultHelpers(), createMetrics(), logger.Named("notifications-manager")) + require.NoError(t, err) + + // Start Run and Stop after each other (run does "go loop()"). + // This is to catch a (now fixed) race condition where the manager + // would be accessed/stopped while it was being created/starting up. + mgr.Run(ctx) + err = mgr.Stop(ctx) + require.NoError(t, err) +} + type syncInterceptor struct { notifications.Store From c5c3a54fcaaed37a979056a859d518ea204334dd Mon Sep 17 00:00:00 2001 From: brettkolodny Date: Thu, 8 May 2025 10:10:52 -0400 Subject: [PATCH 02/24] fix: create ssh directory if it doesn't already exist when running `coder config-ssh` (#17711) Closes [coder/internal#623](https://github.com/coder/internal/issues/623) > [!WARNING] > PR co-authored by Claude Code --- cli/configssh.go | 5 +++++ cli/configssh_test.go | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/cli/configssh.go b/cli/configssh.go index 65f36697d873f..e3e168d2b198c 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -440,6 +440,11 @@ func (r *RootCmd) configSSH() *serpent.Command { } if !bytes.Equal(configRaw, configModified) { + sshDir := filepath.Dir(sshConfigFile) + if err := os.MkdirAll(sshDir, 0700); err != nil { + return xerrors.Errorf("failed to create directory %q: %w", sshDir, err) + } + err = atomic.WriteFile(sshConfigFile, bytes.NewReader(configModified)) if err != nil { return xerrors.Errorf("write ssh config failed: %w", err) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 72faaa00c1ca0..60c93b8e94f4b 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -169,6 +169,47 @@ func TestConfigSSH(t *testing.T) { <-copyDone } +func TestConfigSSH_MissingDirectory(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("See coder/internal#117") + } + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + + // Create a temporary directory but don't create .ssh subdirectory + tmpdir := t.TempDir() + sshConfigPath := filepath.Join(tmpdir, ".ssh", "config") + + // Run config-ssh with a non-existent .ssh directory + args := []string{ + "config-ssh", + "--ssh-config-file", sshConfigPath, + "--yes", // Skip confirmation prompts + } + inv, root := clitest.New(t, args...) + clitest.SetupConfig(t, client, root) + + err := inv.Run() + require.NoError(t, err, "config-ssh should succeed with non-existent directory") + + // Verify that the .ssh directory was created + sshDir := filepath.Dir(sshConfigPath) + _, err = os.Stat(sshDir) + require.NoError(t, err, ".ssh directory should exist") + + // Verify that the config file was created + _, err = os.Stat(sshConfigPath) + require.NoError(t, err, "config file should exist") + + // Check that the directory has proper permissions (0700) + sshDirInfo, err := os.Stat(sshDir) + require.NoError(t, err) + require.Equal(t, os.FileMode(0700), sshDirInfo.Mode().Perm(), "directory should have 0700 permissions") +} + func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { t.Parallel() From 0b141c47cb3ed9faebc7c44798a9324569e9e4bb Mon Sep 17 00:00:00 2001 From: Jaayden Halko Date: Thu, 8 May 2025 15:12:05 +0100 Subject: [PATCH 03/24] chore: add DynamicParameter stories (#17710) resolves coder/preview#112 - Add stories for DynamicParameter component - fix bug with displaying immutable badge and required asterisk --- .../DynamicParameter.stories.tsx | 197 ++++++++++++++++++ .../DynamicParameter/DynamicParameter.tsx | 4 +- site/src/testHelpers/entities.ts | 19 ++ 3 files changed, 218 insertions(+), 2 deletions(-) create mode 100644 site/src/modules/workspaces/DynamicParameter/DynamicParameter.stories.tsx diff --git a/site/src/modules/workspaces/DynamicParameter/DynamicParameter.stories.tsx b/site/src/modules/workspaces/DynamicParameter/DynamicParameter.stories.tsx new file mode 100644 index 0000000000000..03aef9e6363bf --- /dev/null +++ b/site/src/modules/workspaces/DynamicParameter/DynamicParameter.stories.tsx @@ -0,0 +1,197 @@ +import type { Meta, StoryObj } from "@storybook/react"; +import { MockPreviewParameter } from "testHelpers/entities"; +import { DynamicParameter } from "./DynamicParameter"; + +const meta: Meta = { + title: "modules/workspaces/DynamicParameter", + component: DynamicParameter, + parameters: { + layout: "centered", + }, +}; + +export default meta; +type Story = StoryObj; + +export const TextInput: Story = { + args: { + parameter: { + ...MockPreviewParameter, + }, + }, +}; + +export const TextArea: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "textarea", + }, + }, +}; + +export const Checkbox: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "checkbox", + type: "bool", + }, + }, +}; + +export const Switch: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "switch", + type: "bool", + }, + }, +}; + +export const Dropdown: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "dropdown", + type: "string", + options: [ + { + name: "Option 1", + value: { valid: true, value: "option1" }, + description: "this is option 1", + icon: "", + }, + { + name: "Option 2", + value: { valid: true, value: "option2" }, + description: "this is option 2", + icon: "", + }, + { + name: "Option 3", + value: { valid: true, value: "option3" }, + description: "this is option 3", + icon: "", + }, + ], + }, + }, +}; + +export const MultiSelect: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "multi-select", + type: "list(string)", + options: [ + { + name: "Red", + value: { valid: true, value: "red" }, + description: "this is red", + icon: "", + }, + { + name: "Green", + value: { valid: true, value: "green" }, + description: "this is green", + icon: "", + }, + { + name: "Blue", + value: { valid: true, value: "blue" }, + description: "this is blue", + icon: "", + }, + { + name: "Purple", + value: { valid: true, value: "purple" }, + description: "this is purple", + icon: "", + }, + ], + }, + }, +}; + +export const Radio: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "radio", + type: "string", + options: [ + { + name: "Small", + value: { valid: true, value: "small" }, + description: "this is small", + icon: "", + }, + { + name: "Medium", + value: { valid: true, value: "medium" }, + description: "this is medium", + icon: "", + }, + { + name: "Large", + value: { valid: true, value: "large" }, + description: "this is large", + icon: "", + }, + ], + }, + }, +}; + +export const Slider: Story = { + args: { + parameter: { + ...MockPreviewParameter, + form_type: "slider", + type: "number", + }, + }, +}; + +export const Disabled: Story = { + args: { + parameter: { + ...MockPreviewParameter, + value: { valid: true, value: "disabled value" }, + }, + disabled: true, + }, +}; + +export const Preset: Story = { + args: { + parameter: { + ...MockPreviewParameter, + value: { valid: true, value: "preset value" }, + }, + isPreset: true, + }, +}; + +export const Immutable: Story = { + args: { + parameter: { + ...MockPreviewParameter, + mutable: false, + }, + }, +}; + +export const AllBadges: Story = { + args: { + parameter: { + ...MockPreviewParameter, + value: { valid: true, value: "us-west-2" }, + mutable: false, + }, + isPreset: true, + }, +}; diff --git a/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx b/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx index 5f8e875dbebcf..8870602f7dcd1 100644 --- a/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx +++ b/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx @@ -97,11 +97,11 @@ const ParameterLabel: FC = ({ parameter, isPreset }) => {