From 8d1a04e2b888a8d270f15f19d6c0448eb1042bde Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 17:41:17 -0400 Subject: [PATCH 1/7] fix: align reload, shutdown and resource ownership across both run modes (Phase 0) Pre-migration correctness pass. These are the bugs the service/TUI boundary would otherwise cement in, most of them caused by interactive and headless mode implementing the same behaviour twice. - run_interactive.go discarded the error from LoadConfiguration, so a config that failed to parse or validate handed a nil *UncorsConfig to proxy.Restart, which BuildRuntime dereferenced at runtime.go:44. PanicInterceptor re-panics in non-release builds, so a YAML typo killed the TUI. Headless already handled this correctly. The loader now returns an error and both reload paths report it and keep the running generation. - handleServerError quit without shutting the proxy down, stranding the generation a failed start left behind; it now goes through shutdownCmd. - The TUI's shutdown grace period was 5s against 15s in cli and server. - Container.closers was declared and iterated but never appended to, so container.Close() was a no-op. Server and RequestTracker now register themselves; Close releases in reverse creation order and is idempotent, so the server stops before the sink it emits into. - Watcher.Watch used a check-then-set guard that Close never released, making a Watcher permanently "watching". It now claims atomically and Close releases the claim. Making the watcher reusable exposed a data race on the fsnotify handle between a previous run goroutine and a new Watch, so run now owns the watcher it was handed. Co-Authored-By: Claude Opus 5 --- internal/cli/run_interactive.go | 8 +-- internal/config/watcher.go | 64 +++++++++++++++++------- internal/config/watcher_internal_test.go | 2 +- internal/config/watcher_test.go | 19 +++++++ internal/di/container.go | 35 +++++++++++-- internal/di/container_internal_test.go | 25 ++++++++- internal/di/factories.go | 19 ++++++- internal/uncors_app/app.go | 26 +++++++--- internal/uncors_app/app_internal_test.go | 60 +++++++++++++++++++--- 9 files changed, 217 insertions(+), 41 deletions(-) diff --git a/internal/cli/run_interactive.go b/internal/cli/run_interactive.go index 1174e1a1..ba45c636 100644 --- a/internal/cli/run_interactive.go +++ b/internal/cli/run_interactive.go @@ -20,10 +20,12 @@ func runInteractive( container, cfgPath, cfg, - func() *config.UncorsConfig { - reloaded, _, _ := config.LoadConfiguration(container.Fs(), container.Version(), container.Args()) + // The error matters: a config that fails to parse or validate must leave + // the running generation untouched, exactly as headless mode does. + func() (*config.UncorsConfig, error) { + reloaded, _, err := config.LoadConfiguration(container.Fs(), container.Version(), container.Args()) - return reloaded + return reloaded, err }, ) diff --git a/internal/config/watcher.go b/internal/config/watcher.go index aa4f7e62..01c1f4da 100644 --- a/internal/config/watcher.go +++ b/internal/config/watcher.go @@ -7,6 +7,7 @@ import ( "log" "os" "path/filepath" + "sync" "sync/atomic" "time" @@ -19,8 +20,10 @@ var errAlreadyWatching = errors.New("watcher is already watching") type Watcher struct { filePath string - fsWatcher *fsnotify.Watcher isWatching atomic.Bool + + mu sync.Mutex + fsWatcher *fsnotify.Watcher } func NewWatcher(filePath string) *Watcher { @@ -29,15 +32,47 @@ func NewWatcher(filePath string) *Watcher { } } +// Watch starts delivering debounced change notifications for the configured +// file. It is a no-op when no config file is in use. func (w *Watcher) Watch(ctx context.Context, onChange func()) error { - if w.isWatching.Load() { + if w.filePath == "" { + return nil + } + + // Claim the watcher atomically: two concurrent Watch calls must not both + // get past this point and leak an fsnotify watcher between them. + if !w.isWatching.CompareAndSwap(false, true) { return errAlreadyWatching } - if w.filePath == "" { - return nil + err := w.start(ctx, onChange) + if err != nil { + w.isWatching.Store(false) + + return err + } + + return nil +} + +// Close stops the watcher and releases the claim taken by Watch, so a closed +// Watcher reports its true state rather than staying permanently "watching". +func (w *Watcher) Close() error { + w.isWatching.Store(false) + + w.mu.Lock() + fsWatcher := w.fsWatcher + w.fsWatcher = nil + w.mu.Unlock() + + if fsWatcher != nil { + return fsWatcher.Close() } + return nil +} + +func (w *Watcher) start(ctx context.Context, onChange func()) error { _, err := os.Stat(w.filePath) if err != nil { return fmt.Errorf("failed to watch config file '%s': %w", w.filePath, err) @@ -62,23 +97,18 @@ func (w *Watcher) Watch(ctx context.Context, onChange func()) error { ) } + w.mu.Lock() w.fsWatcher = fsWatcher - w.isWatching.Store(true) + w.mu.Unlock() - go w.run(ctx, onChange) - - return nil -} - -func (w *Watcher) Close() error { - if w.fsWatcher != nil { - return w.fsWatcher.Close() - } + // run owns the watcher it was handed rather than reading the field, so a + // Close followed by a fresh Watch cannot race the previous run goroutine. + go w.run(ctx, fsWatcher, onChange) return nil } -func (w *Watcher) run(ctx context.Context, onChange func()) { +func (w *Watcher) run(ctx context.Context, fsWatcher *fsnotify.Watcher, onChange func()) { var debounce *time.Timer defer func() { @@ -92,14 +122,14 @@ func (w *Watcher) run(ctx context.Context, onChange func()) { case <-ctx.Done(): return - case event, ok := <-w.fsWatcher.Events: + case event, ok := <-fsWatcher.Events: if !ok { return } w.handleEvent(event, &debounce, onChange) - case err, ok := <-w.fsWatcher.Errors: + case err, ok := <-fsWatcher.Errors: if !ok { return } diff --git a/internal/config/watcher_internal_test.go b/internal/config/watcher_internal_test.go index 0bd23973..013d888c 100644 --- a/internal/config/watcher_internal_test.go +++ b/internal/config/watcher_internal_test.go @@ -51,7 +51,7 @@ func runAndWait(ctx context.Context, watcher *Watcher, onChange func()) <-chan s go func() { defer close(exited) - watcher.run(ctx, onChange) + watcher.run(ctx, watcher.fsWatcher, onChange) }() return exited diff --git a/internal/config/watcher_test.go b/internal/config/watcher_test.go index 330c58a9..03e32c5d 100644 --- a/internal/config/watcher_test.go +++ b/internal/config/watcher_test.go @@ -224,3 +224,22 @@ func TestNewConfigWatcher(t *testing.T) { assert.False(t, waitForCall(called, 100*time.Millisecond), "onChange was called after context cancelled") }) } + +func TestWatcherCanBeRestartedAfterClose(t *testing.T) { + ctx := t.Context() + + path := filepath.Join(t.TempDir(), "config.yaml") + require.NoError(t, os.WriteFile(path, []byte("mappings: []"), 0o600)) + + watcher := config.NewWatcher(path) + + require.NoError(t, watcher.Watch(ctx, func() {})) + require.ErrorContains(t, watcher.Watch(ctx, func() {}), "already watching") + + require.NoError(t, watcher.Close()) + + // Close releases the claim, so the watcher reports its true state instead of + // staying permanently "watching". + require.NoError(t, watcher.Watch(ctx, func() {})) + require.NoError(t, watcher.Close()) +} diff --git a/internal/di/container.go b/internal/di/container.go index 08a0f1d9..efc1ac96 100644 --- a/internal/di/container.go +++ b/internal/di/container.go @@ -3,6 +3,8 @@ package di import ( "errors" "io" + "slices" + "sync" "github.com/evg4b/uncors/internal/commands" "github.com/evg4b/uncors/internal/contracts" @@ -24,7 +26,8 @@ type Container struct { server factory[*server.Server] proxy factory[*Proxy] - closers []io.Closer + closersMu sync.Mutex + closers []io.Closer } type ContainerOption = func(c *Container) @@ -64,7 +67,7 @@ func NewContainer(options ...ContainerOption) *Container { container = helpers.ApplyOptions(container, options) container.cliOutput = newFactory(container.newCliOutput) - container.requestTracker = newFactory(server.NewRequestTracker) + container.requestTracker = newFactory(container.newRequestTracker) container.generateCertsCommand = newFactory(container.newGenerateCertsCommand) container.hostCertManager = newFactory(container.newHostCertManager) container.server = newFactory(container.newServer) @@ -73,10 +76,18 @@ func NewContainer(options ...ContainerOption) *Container { return container } +// Close releases every process-lifetime resource the container built, in +// reverse creation order so that a resource is never closed before the ones +// depending on it. It is safe to call more than once. func (c *Container) Close() error { - var errs []error + c.closersMu.Lock() + closers := c.closers + c.closers = nil + c.closersMu.Unlock() - for _, closer := range c.closers { + errs := make([]error, 0, len(closers)) + + for _, closer := range slices.Backward(closers) { err := closer.Close() if err != nil { errs = append(errs, err) @@ -85,3 +96,19 @@ func (c *Container) Close() error { return errors.Join(errs...) } + +// registerCloser binds a process-lifetime resource to the container, so that +// Close releases it. Factories are built lazily and potentially from different +// goroutines, so the list is guarded. +func (c *Container) registerCloser(closer io.Closer) { + c.closersMu.Lock() + defer c.closersMu.Unlock() + + c.closers = append(c.closers, closer) +} + +// closerFunc adapts a plain shutdown function to io.Closer, for resources whose +// own Close reports nothing. +type closerFunc func() error + +func (f closerFunc) Close() error { return f() } diff --git a/internal/di/container_internal_test.go b/internal/di/container_internal_test.go index 96861f98..214e806d 100644 --- a/internal/di/container_internal_test.go +++ b/internal/di/container_internal_test.go @@ -41,6 +41,27 @@ func TestContainerCloseError(t *testing.T) { }) } -type closerFunc func() error +func TestContainerClosesProcessLifetimeResources(t *testing.T) { + t.Run("closes the request tracker it built", func(t *testing.T) { + container := NewContainer() + tracker := container.RequestTracker() + + require.NoError(t, container.Close()) + + _, open := <-tracker.Events() + require.False(t, open, "Close must close the tracker's event channel") + }) + + t.Run("is idempotent", func(t *testing.T) { + container := NewContainer() + container.RequestTracker() + container.Server() + + require.NoError(t, container.Close()) + require.NoError(t, container.Close()) + }) -func (f closerFunc) Close() error { return f() } + t.Run("closes nothing it did not build", func(t *testing.T) { + require.NoError(t, NewContainer().Close()) + }) +} diff --git a/internal/di/factories.go b/internal/di/factories.go index 0bb7d4c9..b601daa1 100644 --- a/internal/di/factories.go +++ b/internal/di/factories.go @@ -31,5 +31,22 @@ func (c *Container) Proxy() *Proxy { } func (c *Container) newServer() *server.Server { - return server.New(c.HostCertManager(), c.RequestTracker()) + // RequestTracker is resolved first and therefore registered first, so the + // reverse-order Close stops the server before the sink it emits into. + instance := server.New(c.HostCertManager(), c.RequestTracker()) + c.registerCloser(instance) + + return instance +} + +func (c *Container) newRequestTracker() *server.RequestTracker { + tracker := server.NewRequestTracker() + + c.registerCloser(closerFunc(func() error { + tracker.Close() + + return nil + })) + + return tracker } diff --git a/internal/uncors_app/app.go b/internal/uncors_app/app.go index 907eb65d..7d26ed85 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -18,7 +18,7 @@ import ( const ( outputChannelSize = 1000 - shutdownTimeout = 5 * time.Second + shutdownTimeout = 15 * time.Second versionCheckDelay = 50 * time.Millisecond memTickInterval = 2 * time.Second bytesPerMegabyte = 1024 * 1024 @@ -38,7 +38,7 @@ type UncorsApp struct { cancel context.CancelFunc cfg *config.UncorsConfig - loadConfig func() *config.UncorsConfig + loadConfig func() (*config.UncorsConfig, error) configPath string watcher *config.Watcher @@ -70,7 +70,7 @@ func NewUncorsApp( container *di.Container, configPath string, cfg *config.UncorsConfig, - loadConfig func() *config.UncorsConfig, + loadConfig func() (*config.UncorsConfig, error), ) *UncorsApp { outputCh := make(chan string, outputChannelSize) output := newTuiOutput(outputCh) @@ -275,7 +275,12 @@ func (m *UncorsApp) handleServerStarted() tea.Cmd { m.output.Errorf("Config reloading error: %v", value) }) - newCfg := m.loadConfig() + newCfg, loadErr := m.loadConfig() + if loadErr != nil { + m.output.Errorf("Failed to reload config: %v", loadErr) + + return + } err := m.proxy.Restart(m.appContext(), newCfg) if err != nil { @@ -293,9 +298,11 @@ func (m *UncorsApp) handleServerStarted() tea.Cmd { } func (m *UncorsApp) handleServerError(msg serverErrMsg) tea.Cmd { - m.historyWidget.Update(outputLineMsg(msg.err.Error())) + m.historyWidget, _ = m.historyWidget.Update(outputLineMsg(msg.err.Error())) - return tea.Quit + // Quitting straight away would strand the generation the failed start left + // behind, so go through the normal shutdown path instead. + return m.shutdownCmd() } func (m *UncorsApp) handleRequestEvent(event requestEventMsg) { @@ -387,7 +394,12 @@ func (m *UncorsApp) restartCmd() tea.Cmd { m.output.Errorf("Restart error: %v", value) }) - newCfg := m.loadConfig() + newCfg, loadErr := m.loadConfig() + if loadErr != nil { + m.output.Errorf("Failed to reload config: %v", loadErr) + + return restartMsg{} + } err := m.proxy.Restart(m.appContext(), newCfg) if err != nil { diff --git a/internal/uncors_app/app_internal_test.go b/internal/uncors_app/app_internal_test.go index 013c0807..cc6da6e1 100644 --- a/internal/uncors_app/app_internal_test.go +++ b/internal/uncors_app/app_internal_test.go @@ -4,6 +4,7 @@ import ( "errors" "net/url" "os" + "strings" "testing" "time" @@ -13,6 +14,7 @@ import ( "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/server" + "github.com/evg4b/uncors/testing/hosts" "github.com/evg4b/uncors/testing/testutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -38,10 +40,10 @@ func newTestApp(t *testing.T) (*UncorsApp, *int) { container, "", // no config file — watcher is not created uncorsConfig, - func() *config.UncorsConfig { + func() (*config.UncorsConfig, error) { loadCalls++ - return uncorsConfig + return uncorsConfig, nil }, ) @@ -320,7 +322,7 @@ func TestHandleServerStartedWithConfigPath(t *testing.T) { container := di.NewContainer() defer testutils.Close(t, container) - app := NewUncorsApp(container, tmpFile.Name(), cfg, func() *config.UncorsConfig { return cfg }) + app := NewUncorsApp(container, tmpFile.Name(), cfg, func() (*config.UncorsConfig, error) { return cfg, nil }) defer func() { app.cancel() @@ -348,7 +350,8 @@ func TestHandleServerStartedWithConfigPath(t *testing.T) { container := di.NewContainer() defer testutils.Close(t, container) - app := NewUncorsApp(container, "/nonexistent/path/config.yaml", cfg, func() *config.UncorsConfig { return cfg }) + loader := func() (*config.UncorsConfig, error) { return cfg, nil } + app := NewUncorsApp(container, "/nonexistent/path/config.yaml", cfg, loader) defer func() { app.cancel() @@ -403,13 +406,13 @@ func TestHandleServerStartedCallbackOnFileChange(t *testing.T) { container := di.NewContainer() defer testutils.Close(t, container) - app := NewUncorsApp(container, tmpFile.Name(), cfg, func() *config.UncorsConfig { + app := NewUncorsApp(container, tmpFile.Name(), cfg, func() (*config.UncorsConfig, error) { select { case called <- struct{}{}: default: } - return cfg + return cfg, nil }) defer func() { @@ -473,3 +476,48 @@ func TestHandleShutdownWithWatcher(t *testing.T) { require.NoError(t, err) } } + +// A config that fails to parse or validate must leave the running generation +// serving, exactly as headless mode does. Before this was fixed the failing +// load produced a nil config, which BuildRuntime dereferenced. +func TestReloadWithFailingConfigLoadKeepsServing(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := &config.UncorsConfig{ + Mappings: config.Mappings{ + {From: hosts.Localhost.HTTPPort(port), To: hosts.Localhost.HTTP()}, + }, + } + + container := di.NewContainer() + defer testutils.Close(t, container) + + loadCalls := 0 + app := NewUncorsApp(container, "", cfg, func() (*config.UncorsConfig, error) { + loadCalls++ + + return nil, errBoom + }) + + defer cleanupTestApp(t, app) + + require.NoError(t, app.proxy.Start(app.appContext(), cfg)) + + require.NotPanics(t, func() { + msg := app.restartCmd()() + + assert.IsType(t, restartMsg{}, msg) + }) + + assert.Equal(t, 1, loadCalls) + assert.False(t, testutils.IsPortFree(port), "the previous generation must still be bound") + + var reported bool + + for len(app.outputCh) > 0 { + if strings.Contains(<-app.outputCh, "Failed to reload config") { + reported = true + } + } + + assert.True(t, reported, "the load failure must be reported to the user") +} From e4c48fe0dca4253641ae1b8a6733b6100da76b78 Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 17:47:52 -0400 Subject: [PATCH 2/7] refactor: extract internal/app.Service as the single orchestration owner (Phase 1) Interactive and non-interactive mode each implemented start, config watching, reload, the version check and shutdown separately. They had already drifted (see the previous commit), so this gives that orchestration one owner that both modes drive. internal/app.Service owns the active configuration, the reload lifecycle, the config.Watcher, the version check, signal handling and the service lifetime context. It has no dependency on a terminal. - runNonInteractive shrinks to draining the request tracker and calling service.Run; awaitShutdown, watchConfig and versionCheck moved into the service verbatim in behaviour. - UncorsApp drops proxy, container, cfg, loadConfig, configPath, watcher and its private root context. handleServerStarted is now empty: the service starts watching and version-checking itself. The model sends Start, Reload and Shutdown, and renders what comes back. - Both modes now share one config loader, so a reload cannot behave differently depending on the mode. - Reloads are serialised and coalesced. The decision to stop looping and the clearing of the running flag happen under the lock that sets the pending flag, so a request arriving mid-reload is never dropped. Tests: internal/app/service_test.go covers the service running with no client attached, reload keeping the generation when the config fails, reload moving to a new port, config-file watching, idempotent shutdown, and concurrent reloads never overlapping. The watcher tests move out of the TUI package, which no longer owns a watcher. Co-Authored-By: Claude Opus 5 --- internal/app/service.go | 269 +++++++++++++++++++++++ internal/app/service_test.go | 239 ++++++++++++++++++++ internal/cli/run_non_interactive.go | 92 +------- internal/cli/run_uncors.go | 11 + internal/uncors_app/app.go | 119 +++------- internal/uncors_app/app_internal_test.go | 178 ++------------- 6 files changed, 572 insertions(+), 336 deletions(-) create mode 100644 internal/app/service.go create mode 100644 internal/app/service_test.go diff --git a/internal/app/service.go b/internal/app/service.go new file mode 100644 index 00000000..a61b0c84 --- /dev/null +++ b/internal/app/service.go @@ -0,0 +1,269 @@ +// Package app owns the uncors application runtime: the active configuration, +// the reload lifecycle, the config watcher and the proxy generations derived +// from them. +// +// It is the single implementation both run modes drive. Interactive mode wraps +// it in a Bubble Tea client; non-interactive mode calls Run and renders the +// output stream directly. Nothing in this package may depend on a terminal. +package app + +import ( + "context" + "log" + "os" + "os/signal" + "sync" + "syscall" + "time" + + "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/di" +) + +const ( + shutdownTimeout = 15 * time.Second + versionCheckDelay = 50 * time.Millisecond +) + +// Loader produces the configuration for a new generation. It reports failure +// rather than returning a nil config, because a config that fails to parse or +// validate must leave the running generation untouched. +type Loader func() (*config.UncorsConfig, error) + +// Service owns the application runtime. Its methods are safe to call from +// independent goroutines - the config watcher, a signal handler and a UI client +// all drive it concurrently. +type Service struct { + container *di.Container + proxy *di.Proxy + configPath string + load Loader + + // The service outlives every individual call, so its lifetime context is + // state rather than a parameter; clients derive their own from Context(). + ctx context.Context //nolint:containedctx // service lifetime, not request scope + cancel context.CancelFunc + + mu sync.RWMutex + cfg *config.UncorsConfig + + // reloadMu guards the coalescing state below, not the reload itself. + reloadMu sync.Mutex + reloading bool + pending bool + + watcher *config.Watcher + shutdownOne sync.Once +} + +// New creates the service for one process. cfg is the configuration the first +// generation is built from; configPath is the file to watch, empty when no +// config file is in use. +func New(container *di.Container, cfg *config.UncorsConfig, configPath string, load Loader) *Service { + ctx, cancel := context.WithCancel(context.Background()) + + return &Service{ + container: container, + proxy: container.Proxy(), + configPath: configPath, + load: load, + ctx: ctx, + cancel: cancel, + cfg: cfg, + } +} + +// Context returns the service lifetime context. It is cancelled when the +// service shuts down, so clients can use it to stop their own work. +func (s *Service) Context() context.Context { + return s.ctx +} + +// Config returns the configuration of the current generation. +func (s *Service) Config() *config.UncorsConfig { + s.mu.RLock() + defer s.mu.RUnlock() + + return s.cfg +} + +// Start brings up the first generation and, once it is serving, begins watching +// the config file and checking for a newer release. It returns when the +// listeners are bound. +func (s *Service) Start(ctx context.Context) error { + err := s.proxy.Start(ctx, s.Config()) + if err != nil { + return err + } + + s.startWatching() + + go s.checkVersion() + + return nil +} + +// Reload rebuilds the runtime from the current config file. +// +// Reloads are serialised, and requests arriving while one is running are +// coalesced into a single follow-up run rather than queueing one run each. A +// failure to load, validate or bind leaves the running generation serving. +func (s *Service) Reload() { + s.reloadMu.Lock() + + if s.reloading { + // Someone else is mid-reload; hand them the work and return. + s.pending = true + s.reloadMu.Unlock() + + return + } + + s.reloading = true + s.reloadMu.Unlock() + + for { + s.reloadOnce() + + s.reloadMu.Lock() + + if !s.pending { + // Clearing the flag under the same lock that sets it is what stops a + // request arriving here from being dropped. + s.reloading = false + s.reloadMu.Unlock() + + return + } + + s.pending = false + s.reloadMu.Unlock() + } +} + +// Wait blocks until every listener has stopped. +func (s *Service) Wait() { + s.proxy.Wait() +} + +// Run starts the service and blocks until it stops, either on a shutdown signal +// or when ctx is cancelled. It is the non-interactive entry point. +func (s *Service) Run(ctx context.Context) error { + err := s.Start(ctx) + if err != nil { + return err + } + + go s.awaitSignal(ctx) + + s.Wait() + + return nil +} + +// Shutdown stops the server and releases the active generation. It is safe to +// call more than once and from more than one goroutine. +func (s *Service) Shutdown(ctx context.Context) error { + var err error + + s.shutdownOne.Do(func() { + s.cancel() + + err = s.proxy.Shutdown(ctx) + }) + + return err +} + +// Close releases what the service itself owns. The generation and the server +// belong to the container, which closes them in turn. +func (s *Service) Close() error { + s.cancel() + + if s.watcher != nil { + return s.watcher.Close() + } + + return nil +} + +func (s *Service) reloadOnce() { + output := s.container.CliOutput() + + reloaded, err := s.load() + if err != nil { + output.Errorf("Failed to reload config: %v", err) + + return + } + + err = s.proxy.Restart(s.ctx, reloaded) + if err != nil { + output.Errorf("Failed to restart server: %v", err) + + return + } + + s.mu.Lock() + s.cfg = reloaded + s.mu.Unlock() +} + +// startWatching begins reloading on config file changes. A missing or +// unwatchable file is reported and then ignored: the server keeps serving the +// configuration it already has. +func (s *Service) startWatching() { + if s.configPath == "" { + return + } + + watcher := config.NewWatcher(s.configPath) + + err := watcher.Watch(s.ctx, s.Reload) + if err != nil { + s.container.CliOutput().Errorf("Failed to watch config file: %v", err) + + return + } + + s.watcher = watcher +} + +func (s *Service) checkVersion() { + select { + case <-time.After(versionCheckDelay): + case <-s.ctx.Done(): + return + } + + s.container. + VersionChecker(s.Config().Proxy). + CheckNewVersion(s.ctx) +} + +// awaitSignal stops the service on the first OS signal or context +// cancellation. The shutdown itself runs on a fresh context, because ctx may be +// the one that triggered it. +func (s *Service) awaitSignal(ctx context.Context) { + stop := make(chan os.Signal, 1) + signal.Notify(stop, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP) + + defer signal.Stop(stop) + + select { + case sig := <-stop: + if sig == syscall.SIGINT { + // Move past the "^C" the terminal echoed. + _, _ = s.container.CliOutput().Write([]byte("\n")) + } + + log.Println("shutdown signal received") + case <-ctx.Done(): + case <-s.ctx.Done(): + } + + shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownTimeout) + defer cancel() + + _ = s.Shutdown(shutdownCtx) +} diff --git a/internal/app/service_test.go b/internal/app/service_test.go new file mode 100644 index 00000000..68b08414 --- /dev/null +++ b/internal/app/service_test.go @@ -0,0 +1,239 @@ +package app_test + +import ( + "errors" + "net" + "net/http" + "os" + "path/filepath" + "strconv" + "sync" + "testing" + "time" + + "github.com/evg4b/uncors/internal/app" + "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/di" + "github.com/evg4b/uncors/testing/hosts" + "github.com/evg4b/uncors/testing/testutils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var errLoadFailed = errors.New("config is not valid") + +// occupy binds port for the duration of the test so the service cannot. +func occupy(t *testing.T, port int) { + t.Helper() + + listenConfig := &net.ListenConfig{} + + listener, err := listenConfig.Listen(t.Context(), "tcp4", net.JoinHostPort("127.0.0.1", strconv.Itoa(port))) + require.NoError(t, err) + + t.Cleanup(func() { _ = listener.Close() }) +} + +func configFor(port int) *config.UncorsConfig { + return &config.UncorsConfig{ + Mappings: config.Mappings{ + {From: hosts.Localhost.HTTPPort(port), To: hosts.Localhost.HTTP()}, + }, + } +} + +func newService(t *testing.T, cfg *config.UncorsConfig, path string, load app.Loader) *app.Service { + t.Helper() + + container := di.NewContainer() + service := app.New(container, cfg, path, load) + + t.Cleanup(func() { + require.NoError(t, service.Shutdown(t.Context())) + require.NoError(t, service.Close()) + require.NoError(t, container.Close()) + }) + + return service +} + +func requirePortServing(t *testing.T, port int) { + t.Helper() + + assert.False(t, testutils.IsPortFree(port), "port %d should be bound", port) +} + +// T1: the service is the whole application. Nothing here constructs a TUI. +func TestServiceRunsWithoutAClient(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { + return cfg, nil + }) + + require.NoError(t, service.Start(t.Context())) + requirePortServing(t, port) + + response, err := http.Get("http://localhost:" + strconv.Itoa(port)) //nolint:noctx // liveness probe + require.NoError(t, err) + require.NoError(t, response.Body.Close()) + + require.NoError(t, service.Shutdown(t.Context())) + service.Wait() + + assert.Eventually(t, func() bool { return testutils.IsPortFree(port) }, time.Second, 10*time.Millisecond, + "shutdown must release the listener") +} + +func TestServiceStartFailsWhenPortIsTaken(t *testing.T) { + port := testutils.GetFreePort(t) + + occupy(t, port) + + cfg := configFor(port) + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.Error(t, service.Start(t.Context())) +} + +// T2 at the service level: a config that fails to load leaves the running +// generation serving, and the failure is reported rather than swallowed. +func TestReloadKeepsGenerationWhenConfigFails(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + loads := 0 + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { + loads++ + + return nil, errLoadFailed + }) + + require.NoError(t, service.Start(t.Context())) + + require.NotPanics(t, service.Reload) + + assert.Equal(t, 1, loads) + requirePortServing(t, port) + assert.Same(t, cfg, service.Config(), "a failed reload must not swap the active config") +} + +func TestReloadMovesToThePortOfTheNewConfig(t *testing.T) { + first := testutils.GetFreePort(t) + second := testutils.GetFreePort(t) + + next := configFor(second) + service := newService(t, configFor(first), "", func() (*config.UncorsConfig, error) { + return next, nil + }) + + require.NoError(t, service.Start(t.Context())) + requirePortServing(t, first) + + service.Reload() + + requirePortServing(t, second) + assert.Same(t, next, service.Config()) + assert.Eventually(t, func() bool { return testutils.IsPortFree(first) }, time.Second, 10*time.Millisecond, + "the dropped port must be released") +} + +// T8: concurrent reloads must be serialised, and none may be lost. +func TestConcurrentReloadsAreSerialised(t *testing.T) { + const reloaders = 8 + + port := testutils.GetFreePort(t) + cfg := configFor(port) + + var ( + guard sync.Mutex + inFlight int + overlaps int + loads int + ) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { + guard.Lock() + inFlight++ + loads++ + + if inFlight > 1 { + overlaps++ + } + guard.Unlock() + + time.Sleep(time.Millisecond) + + guard.Lock() + inFlight-- + guard.Unlock() + + return cfg, nil + }) + + require.NoError(t, service.Start(t.Context())) + + var waitGroup sync.WaitGroup + + waitGroup.Add(reloaders) + + for range reloaders { + go func() { + defer waitGroup.Done() + + service.Reload() + }() + } + + waitGroup.Wait() + + guard.Lock() + defer guard.Unlock() + + assert.Zero(t, overlaps, "reloads must never run concurrently") + assert.Positive(t, loads, "coalescing must not swallow every request") + requirePortServing(t, port) +} + +func TestServiceWatchesConfigFile(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + path := filepath.Join(t.TempDir(), "uncors.yaml") + require.NoError(t, os.WriteFile(path, []byte("mappings: []"), 0o600)) + + reloaded := make(chan struct{}, 1) + service := newService(t, cfg, path, func() (*config.UncorsConfig, error) { + select { + case reloaded <- struct{}{}: + default: + } + + return cfg, nil + }) + + require.NoError(t, service.Start(t.Context())) + + require.NoError(t, os.WriteFile(path, []byte("proxy: \"\""), 0o600)) + + select { + case <-reloaded: + case <-time.After(2 * time.Second): + t.Fatal("a config file change must trigger a reload") + } +} + +func TestServiceShutdownIsIdempotent(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.NoError(t, service.Start(t.Context())) + + require.NoError(t, service.Shutdown(t.Context())) + require.NoError(t, service.Shutdown(t.Context())) + + assert.Error(t, service.Context().Err(), "shutdown must cancel the service context") +} diff --git a/internal/cli/run_non_interactive.go b/internal/cli/run_non_interactive.go index e2fd722c..d6e4ccaf 100644 --- a/internal/cli/run_non_interactive.go +++ b/internal/cli/run_non_interactive.go @@ -2,24 +2,17 @@ package cli import ( "context" - "log" - "os" - "os/signal" - "syscall" - "time" + "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/server" ) -const ( - shutdownTimeout = 15 * time.Second - versionCheckDelay = 50 * time.Millisecond -) - // runNonInteractive starts the proxy in headless mode and blocks until the -// server stops, either on a shutdown signal or when ctx is cancelled. +// server stops, either on a shutdown signal or when ctx is cancelled. It drives +// the same app.Service the interactive mode does; the only difference is that +// here the events are rendered straight to the console. func runNonInteractive( ctx context.Context, container *di.Container, @@ -27,24 +20,20 @@ func runNonInteractive( cfgPath string, ) error { output := container.CliOutput() - proxy := container.Proxy() // Headless mode has no TUI, so it must drain the request tracker itself; // without a consumer the request path can only drop activity events. tracker := container.RequestTracker() go server.RequestPrinter(tracker, output) - err := proxy.Start(ctx, cfg) + service := app.New(container, cfg, cfgPath, configLoader(container)) + defer func() { _ = service.Close() }() + + err := service.Run(ctx) if err != nil { return err } - go versionCheck(ctx, container, cfg.Proxy) - go watchConfig(ctx, container, proxy, cfgPath) - go awaitShutdown(ctx, proxy) - - proxy.Wait() - if dropped := tracker.Dropped(); dropped > 0 { output.Warnf("%d activity lines were dropped to keep the proxy responsive", dropped) } @@ -53,68 +42,3 @@ func runNonInteractive( return nil } - -// awaitShutdown stops the proxy on the first OS signal or context cancellation. -// The shutdown itself runs on a fresh context because ctx may be the one that -// triggered it. -func awaitShutdown(ctx context.Context, proxy *di.Proxy) { - stop := make(chan os.Signal, 1) - signal.Notify(stop, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP) - - defer signal.Stop(stop) - - select { - case sig := <-stop: - if sig == syscall.SIGINT { - // Move past the "^C" the terminal echoed. - _, _ = os.Stdout.WriteString("\n") - } - - log.Println("shutdown signal received") - case <-ctx.Done(): - } - - shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownTimeout) - defer cancel() - - _ = proxy.Shutdown(shutdownCtx) -} - -// watchConfig restarts the proxy on every change to the config file. It is a -// no-op when no config file is in use. -func watchConfig(ctx context.Context, container *di.Container, proxy *di.Proxy, cfgPath string) { - output := container.CliOutput() - - watcher := config.NewWatcher(cfgPath) - - err := watcher.Watch(ctx, func() { - reloaded, _, err := config.LoadConfiguration(container.Fs(), container.Version(), container.Args()) - if err != nil { - output.Error(err) - - return - } - - err = proxy.Restart(ctx, reloaded) - if err != nil { - output.Error(err) - } - }) - if err != nil { - output.Error(err) - - return - } - - <-ctx.Done() - - _ = watcher.Close() -} - -// versionCheck waits for a short delay then checks for a newer release. -func versionCheck(ctx context.Context, container *di.Container, proxy string) { - time.Sleep(versionCheckDelay) - - container.VersionChecker(proxy). - CheckNewVersion(ctx) -} diff --git a/internal/cli/run_uncors.go b/internal/cli/run_uncors.go index e7090db9..fb22a679 100644 --- a/internal/cli/run_uncors.go +++ b/internal/cli/run_uncors.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/di" "github.com/spf13/pflag" @@ -34,3 +35,13 @@ func RunUncors(ctx context.Context, container *di.Container) error { return runNonInteractive(ctx, container, uncorsConfig, cfgPath) } + +// configLoader re-reads the configuration the process was started with. Both +// run modes reload the same way, so they share one loader. +func configLoader(container *di.Container) app.Loader { + return func() (*config.UncorsConfig, error) { + reloaded, _, err := config.LoadConfiguration(container.Fs(), container.Version(), container.Args()) + + return reloaded, err + } +} diff --git a/internal/uncors_app/app.go b/internal/uncors_app/app.go index 7d26ed85..ed66fb47 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -9,6 +9,7 @@ import ( "charm.land/bubbles/v2/key" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" + "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" @@ -19,7 +20,6 @@ import ( const ( outputChannelSize = 1000 shutdownTimeout = 15 * time.Second - versionCheckDelay = 50 * time.Millisecond memTickInterval = 2 * time.Second bytesPerMegabyte = 1024 * 1024 ) @@ -27,21 +27,15 @@ const ( type UncorsApp struct { keys keyMap - proxy *di.Proxy - output *tuiOutput - tracker server.IRequestTracker - container *di.Container + // service owns the application runtime. The model only sends it commands + // and renders what comes back. + service *app.Service - outputCh chan string - appContext func() context.Context - appDone <-chan struct{} - cancel context.CancelFunc + output *tuiOutput + tracker server.IRequestTracker - cfg *config.UncorsConfig - loadConfig func() (*config.UncorsConfig, error) - configPath string - - watcher *config.Watcher + outputCh chan string + done <-chan struct{} termHeight int termWidth int @@ -63,42 +57,36 @@ type appUpdateMsg interface { update(app *UncorsApp) tea.Cmd } -// NewUncorsApp creates the interactive TUI model. configPath is the active -// config file path (empty string if no config file is used); when non-empty -// the app watches it for changes and auto-restarts the proxy on every save. +// NewUncorsApp creates the interactive TUI model over the application service. +// configPath is the active config file path (empty when no config file is in +// use); the service watches it and reloads on every save. func NewUncorsApp( container *di.Container, configPath string, cfg *config.UncorsConfig, - loadConfig func() (*config.UncorsConfig, error), + loadConfig app.Loader, ) *UncorsApp { outputCh := make(chan string, outputChannelSize) output := newTuiOutput(outputCh) - appCtx, cancel := context.WithCancel(context.Background()) - + // The sink has to be installed before anything resolves CliOutput, because + // the container caches it on first use. container.Override(di.WithCliOutput(func() contracts.Output { return output })) - keys := newKeyMap() + service := app.New(container, cfg, configPath, loadConfig) - historyWidget := NewHistoryWidget(keys) + keys := newKeyMap() return &UncorsApp{ keys: keys, - proxy: container.Proxy(), + service: service, output: output, tracker: container.RequestTracker(), - container: container, outputCh: outputCh, - appContext: func() context.Context { return appCtx }, - appDone: appCtx.Done(), - cancel: cancel, - cfg: cfg, - loadConfig: loadConfig, - configPath: configPath, - historyWidget: historyWidget, + done: service.Context().Done(), + historyWidget: NewHistoryWidget(keys), trackerWidget: NewTrackerWidget(), helpWidget: NewHelpWidget(keys), memWidget: NewMemoryWidget(), @@ -266,35 +254,12 @@ func (msg shutdownMsg) update(app *UncorsApp) tea.Cmd { return app.handleShutdown() } +// handleServerStarted runs once the listeners are bound. Config watching and +// the version check belong to the service, which started them itself. func (m *UncorsApp) handleServerStarted() tea.Cmd { - if m.configPath != "" { - watcher := config.NewWatcher(m.configPath) - - err := watcher.Watch(m.appContext(), func() { - defer helpers.PanicInterceptor(func(value any) { - m.output.Errorf("Config reloading error: %v", value) - }) - - newCfg, loadErr := m.loadConfig() - if loadErr != nil { - m.output.Errorf("Failed to reload config: %v", loadErr) - - return - } + log.Println("Server started") - err := m.proxy.Restart(m.appContext(), newCfg) - if err != nil { - m.output.Errorf("Failed to restart server: %v", err) - } - }) - if err != nil { - m.output.Errorf("Failed to watch config file: %v", err) - } else { - m.watcher = watcher - } - } - - return m.versionCheckCmd() + return nil } func (m *UncorsApp) handleServerError(msg serverErrMsg) tea.Cmd { @@ -325,10 +290,6 @@ func (m *UncorsApp) handleRestart() { func (m *UncorsApp) handleShutdown() tea.Cmd { log.Println("Handling shutdown") - if m.watcher != nil { - _ = m.watcher.Close() - } - _ = m.historyWidget.Close() return tea.Quit @@ -336,7 +297,7 @@ func (m *UncorsApp) handleShutdown() tea.Cmd { func (m *UncorsApp) startServerCmd() tea.Cmd { return func() tea.Msg { - err := m.proxy.Start(m.appContext(), m.cfg) + err := m.service.Start(m.service.Context()) if err != nil { return serverErrMsg{err: err} } @@ -354,7 +315,7 @@ func (m *UncorsApp) waitOutputCmd() tea.Cmd { } return outputLineMsg(line) - case <-m.appDone: + case <-m.done: return nil } } @@ -369,7 +330,7 @@ func (m *UncorsApp) watchEventsCmd() tea.Cmd { } return requestEventMsg(event) - case <-m.appDone: + case <-m.done: return nil } } @@ -377,12 +338,11 @@ func (m *UncorsApp) watchEventsCmd() tea.Cmd { func (m *UncorsApp) shutdownCmd() tea.Cmd { return func() tea.Msg { - m.cancel() - ctx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) defer cancel() - _ = m.proxy.Shutdown(ctx) + _ = m.service.Shutdown(ctx) + _ = m.service.Close() return shutdownMsg{} } @@ -394,29 +354,8 @@ func (m *UncorsApp) restartCmd() tea.Cmd { m.output.Errorf("Restart error: %v", value) }) - newCfg, loadErr := m.loadConfig() - if loadErr != nil { - m.output.Errorf("Failed to reload config: %v", loadErr) - - return restartMsg{} - } - - err := m.proxy.Restart(m.appContext(), newCfg) - if err != nil { - m.output.Errorf("Failed to restart: %v", err) - } + m.service.Reload() return restartMsg{} } } - -func (m *UncorsApp) versionCheckCmd() tea.Cmd { - return func() tea.Msg { - time.Sleep(versionCheckDelay) - - m.container.VersionChecker(m.cfg.Proxy). - CheckNewVersion(m.appContext()) - - return nil - } -} diff --git a/internal/uncors_app/app_internal_test.go b/internal/uncors_app/app_internal_test.go index cc6da6e1..4ee05c92 100644 --- a/internal/uncors_app/app_internal_test.go +++ b/internal/uncors_app/app_internal_test.go @@ -3,7 +3,6 @@ package uncorsapp import ( "errors" "net/url" - "os" "strings" "testing" "time" @@ -53,9 +52,8 @@ func newTestApp(t *testing.T) (*UncorsApp, *int) { func cleanupTestApp(t *testing.T, app *UncorsApp) { t.Helper() - app.cancel() - err := app.proxy.Close() - require.NoError(t, err) + require.NoError(t, app.service.Close()) + require.NoError(t, app.service.Shutdown(t.Context())) if app.historyWidget != nil && app.historyWidget.hist != nil { err := app.historyWidget.hist.Close() @@ -70,8 +68,8 @@ func TestNewUncorsAppAndKeyMap(t *testing.T) { assert.NotNil(t, app.output) assert.NotNil(t, app.tracker) assert.NotNil(t, app.historyWidget.hist) - assert.NotNil(t, app.appContext) - assert.NotNil(t, app.appDone) + assert.NotNil(t, app.service) + assert.NotNil(t, app.done) assert.True(t, app.historyWidget.autoScroll) assert.Empty(t, app.trackerWidget.pending) assert.GreaterOrEqual(t, app.memWidget.memMB, 0.0) @@ -165,8 +163,9 @@ func TestUncorsAppCommandFactoriesAndChannels(t *testing.T) { msg := app.startServerCmd()() assert.IsType(t, serverStartedMsg{}, msg) - cmd := app.handleServerStarted() - require.NotNil(t, cmd) + // Watching and the version check moved to the service, so the model has + // no follow-up command of its own. + assert.Nil(t, app.handleServerStarted()) msg = app.restartCmd()() assert.Equal(t, restartMsg{}, msg) @@ -184,7 +183,7 @@ func TestUncorsAppCommandFactoriesAndChannels(t *testing.T) { assert.Equal(t, outputLineMsg("queued"), app.waitOutputCmd()()) - app.cancel() + require.NoError(t, app.service.Close()) assert.Nil(t, app.waitOutputCmd()()) }) @@ -211,7 +210,7 @@ func TestUncorsAppCommandFactoriesAndChannels(t *testing.T) { app.watchEventsCmd()(), ) - app.cancel() + require.NoError(t, app.service.Close()) assert.Nil(t, app.watchEventsCmd()()) }) @@ -293,9 +292,8 @@ func TestUncorsAppServerErrorRestartShutdownAndFormatting(t *testing.T) { require.NotNil(t, cmd) assert.Equal(t, tea.Quit(), cmd()) - app.cancel() - err := app.proxy.Close() - require.NoError(t, err) + require.NoError(t, app.service.Close()) + require.NoError(t, app.service.Shutdown(t.Context())) }) } @@ -303,72 +301,13 @@ func TestServerStartedMsgUpdate(t *testing.T) { app, _ := newTestApp(t) defer cleanupTestApp(t, app) - model, cmd := app.Update(serverStartedMsg{}) + model, _ := app.Update(serverStartedMsg{}) require.Same(t, app, model) - require.NotNil(t, cmd) -} - -func TestHandleServerStartedWithConfigPath(t *testing.T) { - t.Run("creates watcher when config file exists", func(t *testing.T) { - tmpFile, err := os.CreateTemp(t.TempDir(), "uncors-*.yaml") - require.NoError(t, err) - - err = tmpFile.Close() - require.NoError(t, err) - - cfg := &config.UncorsConfig{Mappings: config.Mappings{}} - - container := di.NewContainer() - defer testutils.Close(t, container) - - app := NewUncorsApp(container, tmpFile.Name(), cfg, func() (*config.UncorsConfig, error) { return cfg, nil }) - - defer func() { - app.cancel() - err := app.proxy.Close() - require.NoError(t, err) - - if app.historyWidget != nil && app.historyWidget.hist != nil { - err := app.historyWidget.hist.Close() - require.NoError(t, err) - } - }() - - cmd := app.handleServerStarted() - - require.NotNil(t, cmd) - require.NotNil(t, app.watcher) - - err = app.watcher.Close() - require.NoError(t, err) - }) - - t.Run("logs error when config file does not exist", func(t *testing.T) { - cfg := &config.UncorsConfig{Mappings: config.Mappings{}} - - container := di.NewContainer() - defer testutils.Close(t, container) - - loader := func() (*config.UncorsConfig, error) { return cfg, nil } - app := NewUncorsApp(container, "/nonexistent/path/config.yaml", cfg, loader) - defer func() { - app.cancel() - err := app.proxy.Close() - require.NoError(t, err) - - if app.historyWidget != nil && app.historyWidget.hist != nil { - err := app.historyWidget.hist.Close() - require.NoError(t, err) - } - }() - - cmd := app.handleServerStarted() - - require.NotNil(t, cmd) - assert.Nil(t, app.watcher) - }) + // Watching the config file and checking for a new version belong to the + // service, so the model has nothing of its own left to do here. + assert.Nil(t, app.handleServerStarted()) } func TestHandleRequestEventWithData(t *testing.T) { @@ -392,91 +331,6 @@ func TestHandleRequestEventWithData(t *testing.T) { }) } -func TestHandleServerStartedCallbackOnFileChange(t *testing.T) { - tmpFile, err := os.CreateTemp(t.TempDir(), "uncors-*.yaml") - require.NoError(t, err) - - err = tmpFile.Close() - require.NoError(t, err) - - cfg := &config.UncorsConfig{Mappings: config.Mappings{}} - - called := make(chan struct{}, 1) - - container := di.NewContainer() - defer testutils.Close(t, container) - - app := NewUncorsApp(container, tmpFile.Name(), cfg, func() (*config.UncorsConfig, error) { - select { - case called <- struct{}{}: - default: - } - - return cfg, nil - }) - - defer func() { - // Cancel context first so any in-flight Restart fails fast. - // We deliberately skip app.proxy.Close() here: closeAll() writes - // app.closers concurrently with the Restart goroutine's read of - // app.closers, which would be a data race. - app.cancel() - - if app.watcher != nil { - err := app.watcher.Close() - require.NoError(t, err) - } - - if app.historyWidget != nil && app.historyWidget.hist != nil { - err := app.historyWidget.hist.Close() - require.NoError(t, err) - } - }() - - cmd := app.handleServerStarted() - - require.NotNil(t, cmd) - require.NotNil(t, app.watcher) - - require.NoError(t, os.WriteFile(tmpFile.Name(), []byte("proxy: \"\""), 0o600)) - - select { - case <-called: - case <-time.After(500 * time.Millisecond): - t.Fatal("onChange callback was not invoked within timeout") - } -} - -func TestHandleShutdownWithWatcher(t *testing.T) { - tmpFile, err := os.CreateTemp(t.TempDir(), "uncors-*.yaml") - require.NoError(t, err) - - err = tmpFile.Close() - require.NoError(t, err) - - ctx := t.Context() - - watcher := config.NewWatcher(tmpFile.Name()) - err = watcher.Watch(ctx, func() {}) - require.NoError(t, err) - - app, _ := newTestApp(t) - app.watcher = watcher - - cmd := app.handleShutdown() - require.NotNil(t, cmd) - assert.Equal(t, tea.Quit(), cmd()) - - app.cancel() - err = app.proxy.Close() - require.NoError(t, err) - - if app.historyWidget != nil && app.historyWidget.hist != nil { - err := app.historyWidget.hist.Close() - require.NoError(t, err) - } -} - // A config that fails to parse or validate must leave the running generation // serving, exactly as headless mode does. Before this was fixed the failing // load produced a nil config, which BuildRuntime dereferenced. @@ -500,7 +354,7 @@ func TestReloadWithFailingConfigLoadKeepsServing(t *testing.T) { defer cleanupTestApp(t, app) - require.NoError(t, app.proxy.Start(app.appContext(), cfg)) + require.NoError(t, app.service.Start(app.service.Context())) require.NotPanics(t, func() { msg := app.restartCmd()() From 9a50b282b06dd0a2a5f7aa5211afdee49aa1921d Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 17:54:15 -0400 Subject: [PATCH 3/7] refactor: give the service a structured event stream (Phase 2) Service code no longer decides what the console looks like. di.Proxy printed the logo, the disclaimer, the mappings box and the restart messages from inside the generation-transition code, which meant the dependency-injection layer knew what a terminal was. - internal/app now emits LifecycleEvent and LogEvent. The set is small and matches what the application already communicated: starting, started, start-failed, reloading, reloaded, reload-failed, stopping, stopped. - Lifecycle is recorded as Status as well as notified. Notifications are dropped under pressure like any other event, which is only safe because the latest state stays readable - a client that misses a notification can still read the truth. - Log events are dropped and counted, mirroring RequestTracker. Presentation must never be able to stall the service. - internal/render is the single place that turns an event into console output. Both modes use it, so neither can drift from the other in what it reports. - di.Proxy prints nothing at all now. Verified the headless output is byte-identical to the previous commit by running both binaries through start, reload, a rejected config and SIGTERM, stripping ANSI and normalising ports. That comparison caught one real regression: Reloading was announced before the config had loaded, so a rejected config claimed the server was restarting. It is now emitted only once the config is known to be good. Co-Authored-By: Claude Opus 5 --- internal/app/events.go | 155 ++++++++++++++++++ internal/app/events_test.go | 150 +++++++++++++++++ internal/app/service.go | 59 +++++-- internal/cli/run_non_interactive.go | 22 ++- internal/di/proxy.go | 18 +- internal/render/render.go | 88 ++++++++++ internal/render/render_test.go | 116 +++++++++++++ internal/uncors_app/app.go | 32 +++- internal/uncors_app/app_internal_test.go | 22 +-- .../internal/render/render_test.snap | 66 ++++++++ 10 files changed, 681 insertions(+), 47 deletions(-) create mode 100644 internal/app/events.go create mode 100644 internal/app/events_test.go create mode 100644 internal/render/render.go create mode 100644 internal/render/render_test.go create mode 100644 testing/snapshots/internal/render/render_test.snap diff --git a/internal/app/events.go b/internal/app/events.go new file mode 100644 index 00000000..2b55eca1 --- /dev/null +++ b/internal/app/events.go @@ -0,0 +1,155 @@ +package app + +import ( + "sync" + "sync/atomic" + + "github.com/evg4b/uncors/internal/config" +) + +const eventsBufferSize = 1000 + +// Event is something the service reports to whoever is presenting it. The set +// is deliberately small: it covers what the application already communicated, +// and nothing more. +type Event interface { + isEvent() +} + +// LifecycleState is the state of the server as the service understands it. +type LifecycleState int + +const ( + StateStarting LifecycleState = iota + StateStarted + StateStartFailed + StateReloading + StateReloaded + StateReloadFailed + StateStopping + StateStopped +) + +// LifecycleEvent reports a change of server state. Mappings carries the +// configuration of the generation the state refers to, so a presenter never +// has to reach back into the service to describe what is serving. +type LifecycleEvent struct { + State LifecycleState + Mappings config.Mappings + Err error + + // Interrupted marks a stop caused by SIGINT, where the terminal has already + // echoed "^C" and the presenter may need to move past it. + Interrupted bool +} + +func (LifecycleEvent) isEvent() {} + +// Level classifies a LogEvent for presentation. It carries no rendering. +type Level int + +const ( + LevelInfo Level = iota + LevelWarn + LevelError +) + +// LogEvent is a message from the service addressed to the user. +type LogEvent struct { + Level Level + Prefix string + Message string +} + +func (LogEvent) isEvent() {} + +// Status is the latest lifecycle state, always readable regardless of whether +// the notification for it was delivered. +type Status struct { + State LifecycleState + Mappings config.Mappings + Err error +} + +// emitter fans service events out to the single presenting client. +// +// Log events are dropped when the client cannot keep up, and counted, exactly +// as request activity is: presentation must never be able to stall the +// service. Lifecycle notifications are dropped under the same pressure, which +// is safe only because the latest state is also recorded in status - a client +// that misses a notification can still read the truth. +type emitter struct { + events chan Event + + mu sync.RWMutex + status Status + + // sendMu guards closed together with the send itself, so Close can never + // race a send onto an already-closed channel. + sendMu sync.RWMutex + closed bool + dropped atomic.Uint64 +} + +func newEmitter() *emitter { + return &emitter{events: make(chan Event, eventsBufferSize)} +} + +func (e *emitter) Events() <-chan Event { + return e.events +} + +func (e *emitter) Status() Status { + e.mu.RLock() + defer e.mu.RUnlock() + + return e.status +} + +func (e *emitter) Dropped() uint64 { + return e.dropped.Load() +} + +func (e *emitter) EmitLifecycle(event LifecycleEvent) { + e.mu.Lock() + e.status = Status{State: event.State, Mappings: event.Mappings, Err: event.Err} + e.mu.Unlock() + + e.send(event) +} + +func (e *emitter) EmitLog(level Level, message string) { + e.send(LogEvent{Level: level, Message: message}) +} + +// Close stops delivery. It is safe to call concurrently with an emit and safe +// to call more than once. +func (e *emitter) Close() { + e.sendMu.Lock() + defer e.sendMu.Unlock() + + if e.closed { + return + } + + e.closed = true + + close(e.events) +} + +func (e *emitter) send(event Event) { + e.sendMu.RLock() + defer e.sendMu.RUnlock() + + if e.closed { + e.dropped.Add(1) + + return + } + + select { + case e.events <- event: + default: + e.dropped.Add(1) + } +} diff --git a/internal/app/events_test.go b/internal/app/events_test.go new file mode 100644 index 00000000..c8763270 --- /dev/null +++ b/internal/app/events_test.go @@ -0,0 +1,150 @@ +package app_test + +import ( + "sync" + "testing" + + "github.com/evg4b/uncors/internal/app" + "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/di" + "github.com/evg4b/uncors/testing/testutils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func drain(t *testing.T, service *app.Service) []app.Event { + t.Helper() + + events := make([]app.Event, 0) + + for { + select { + case event := <-service.Events(): + events = append(events, event) + default: + return events + } + } +} + +func statesOf(events []app.Event) []app.LifecycleState { + states := make([]app.LifecycleState, 0, len(events)) + + for _, event := range events { + if lifecycle, ok := event.(app.LifecycleEvent); ok { + states = append(states, lifecycle.State) + } + } + + return states +} + +func TestServiceEmitsLifecycle(t *testing.T) { + t.Run("a successful start reports starting then started with its mappings", func(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.NoError(t, service.Start(t.Context())) + + events := drain(t, service) + assert.Equal(t, []app.LifecycleState{app.StateStarting, app.StateStarted}, statesOf(events)) + + started, ok := events[1].(app.LifecycleEvent) + require.True(t, ok) + assert.Equal(t, cfg.Mappings, started.Mappings, + "the event must describe the generation without the client asking the service") + }) + + t.Run("a failed start reports the error", func(t *testing.T) { + port := testutils.GetFreePort(t) + + occupy(t, port) + + cfg := configFor(port) + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.Error(t, service.Start(t.Context())) + assert.Equal(t, app.StateStartFailed, service.Status().State) + require.Error(t, service.Status().Err) + }) + + t.Run("a failed reload reports the error and keeps the old mappings", func(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return nil, errLoadFailed }) + + require.NoError(t, service.Start(t.Context())) + + service.Reload() + + assert.Equal(t, app.StateReloadFailed, service.Status().State) + require.ErrorIs(t, service.Status().Err, errLoadFailed) + assert.Same(t, cfg, service.Config()) + }) +} + +// Status must stay truthful even when nobody is draining the stream, because +// that is the whole reason lifecycle is state rather than only a notification. +func TestStatusSurvivesAnUndrainedStream(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.NoError(t, service.Start(t.Context())) + + for range 3000 { + service.Reload() + } + + assert.Positive(t, service.DroppedEvents(), "an undrained stream must drop and count, not block") + assert.Equal(t, app.StateReloaded, service.Status().State) +} + +// Emitting must never block the caller, whatever the consumer is doing. +func TestEmittingNeverBlocks(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + service := newService(t, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.NoError(t, service.Start(t.Context())) + + var waitGroup sync.WaitGroup + + waitGroup.Go(func() { + for range 500 { + service.Reload() + } + }) + + done := make(chan struct{}) + + go func() { + waitGroup.Wait() + close(done) + }() + + select { + case <-done: + case <-t.Context().Done(): + t.Fatal("emitting blocked") + } +} + +func TestClosedServiceStopsDelivery(t *testing.T) { + cfg := configFor(testutils.GetFreePort(t)) + + container := di.NewContainer() + service := app.New(container, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.NoError(t, service.Close()) + + _, open := <-service.Events() + assert.False(t, open, "Close must close the event stream so consumers terminate") + + require.NoError(t, container.Close()) +} diff --git a/internal/app/service.go b/internal/app/service.go index a61b0c84..d3746a15 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -9,6 +9,7 @@ package app import ( "context" + "fmt" "log" "os" "os/signal" @@ -52,6 +53,7 @@ type Service struct { reloading bool pending bool + events *emitter watcher *config.Watcher shutdownOne sync.Once } @@ -70,9 +72,28 @@ func New(container *di.Container, cfg *config.UncorsConfig, configPath string, l ctx: ctx, cancel: cancel, cfg: cfg, + events: newEmitter(), } } +// Events returns the service's event stream. It has a single consumer: the TUI +// in interactive mode, the console renderer otherwise. +func (s *Service) Events() <-chan Event { + return s.events.Events() +} + +// Status returns the latest lifecycle state. It is always current, even when +// the notification carrying it was dropped. +func (s *Service) Status() Status { + return s.events.Status() +} + +// DroppedEvents reports how many events were discarded because the presenter +// could not keep up. +func (s *Service) DroppedEvents() uint64 { + return s.events.Dropped() +} + // Context returns the service lifetime context. It is cancelled when the // service shuts down, so clients can use it to stop their own work. func (s *Service) Context() context.Context { @@ -91,11 +112,19 @@ func (s *Service) Config() *config.UncorsConfig { // the config file and checking for a newer release. It returns when the // listeners are bound. func (s *Service) Start(ctx context.Context) error { - err := s.proxy.Start(ctx, s.Config()) + cfg := s.Config() + + s.events.EmitLifecycle(LifecycleEvent{State: StateStarting, Mappings: cfg.Mappings}) + + err := s.proxy.Start(ctx, cfg) if err != nil { + s.events.EmitLifecycle(LifecycleEvent{State: StateStartFailed, Err: err}) + return err } + s.events.EmitLifecycle(LifecycleEvent{State: StateStarted, Mappings: cfg.Mappings}) + s.startWatching() go s.checkVersion() @@ -170,6 +199,8 @@ func (s *Service) Shutdown(ctx context.Context) error { s.cancel() err = s.proxy.Shutdown(ctx) + + s.events.EmitLifecycle(LifecycleEvent{State: StateStopped, Err: err}) }) return err @@ -179,6 +210,7 @@ func (s *Service) Shutdown(ctx context.Context) error { // belong to the container, which closes them in turn. func (s *Service) Close() error { s.cancel() + s.events.Close() if s.watcher != nil { return s.watcher.Close() @@ -188,18 +220,20 @@ func (s *Service) Close() error { } func (s *Service) reloadOnce() { - output := s.container.CliOutput() - reloaded, err := s.load() if err != nil { - output.Errorf("Failed to reload config: %v", err) + s.events.EmitLifecycle(LifecycleEvent{State: StateReloadFailed, Err: err}) return } + // Announced only once the config is known to be good, so a rejected config + // never claims the server is restarting. + s.events.EmitLifecycle(LifecycleEvent{State: StateReloading}) + err = s.proxy.Restart(s.ctx, reloaded) if err != nil { - output.Errorf("Failed to restart server: %v", err) + s.events.EmitLifecycle(LifecycleEvent{State: StateReloadFailed, Err: err}) return } @@ -207,6 +241,8 @@ func (s *Service) reloadOnce() { s.mu.Lock() s.cfg = reloaded s.mu.Unlock() + + s.events.EmitLifecycle(LifecycleEvent{State: StateReloaded, Mappings: reloaded.Mappings}) } // startWatching begins reloading on config file changes. A missing or @@ -221,7 +257,7 @@ func (s *Service) startWatching() { err := watcher.Watch(s.ctx, s.Reload) if err != nil { - s.container.CliOutput().Errorf("Failed to watch config file: %v", err) + s.events.EmitLog(LevelError, fmt.Sprintf("Failed to watch config file: %v", err)) return } @@ -250,18 +286,21 @@ func (s *Service) awaitSignal(ctx context.Context) { defer signal.Stop(stop) + interrupted := false + select { case sig := <-stop: - if sig == syscall.SIGINT { - // Move past the "^C" the terminal echoed. - _, _ = s.container.CliOutput().Write([]byte("\n")) - } + interrupted = sig == syscall.SIGINT log.Println("shutdown signal received") case <-ctx.Done(): case <-s.ctx.Done(): } + // Interrupted tells the presenter the terminal already echoed "^C"; whether + // that needs moving past is its decision, not the service's. + s.events.EmitLifecycle(LifecycleEvent{State: StateStopping, Interrupted: interrupted}) + shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownTimeout) defer cancel() diff --git a/internal/cli/run_non_interactive.go b/internal/cli/run_non_interactive.go index d6e4ccaf..a706dd4b 100644 --- a/internal/cli/run_non_interactive.go +++ b/internal/cli/run_non_interactive.go @@ -5,14 +5,16 @@ import ( "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" + "github.com/evg4b/uncors/internal/render" "github.com/evg4b/uncors/internal/server" ) // runNonInteractive starts the proxy in headless mode and blocks until the // server stops, either on a shutdown signal or when ctx is cancelled. It drives -// the same app.Service the interactive mode does; the only difference is that -// here the events are rendered straight to the console. +// the same app.Service interactive mode does; the only difference is that here +// the events are rendered straight to the console. func runNonInteractive( ctx context.Context, container *di.Container, @@ -29,16 +31,26 @@ func runNonInteractive( service := app.New(container, cfg, cfgPath, configLoader(container)) defer func() { _ = service.Close() }() + // The renderer has to be draining before the service starts, or the startup + // banner has nowhere to go. + renderer := render.New(output, container.Version()) + go renderer.Consume(service.Events()) + err := service.Run(ctx) if err != nil { return err } - if dropped := tracker.Dropped(); dropped > 0 { - output.Warnf("%d activity lines were dropped to keep the proxy responsive", dropped) - } + reportDropped(output, tracker.Dropped(), "activity lines") + reportDropped(output, service.DroppedEvents(), "service events") output.Info("Server was stopped") return nil } + +func reportDropped(output contracts.WarnOutput, dropped uint64, what string) { + if dropped > 0 { + output.Warnf("%d %s were dropped to keep the proxy responsive", dropped, what) + } +} diff --git a/internal/di/proxy.go b/internal/di/proxy.go index 84e3a1dc..f4b636b9 100644 --- a/internal/di/proxy.go +++ b/internal/di/proxy.go @@ -6,11 +6,11 @@ import ( "sync" "github.com/evg4b/uncors/internal/config" - "github.com/evg4b/uncors/internal/tui" ) // Proxy serves one configuration generation on the container's server and owns -// the transition between generations. Releasing a generation is what flushes +// the transition between generations. It reports nothing to the console: +// describing what is happening belongs to whoever is presenting the service. Releasing a generation is what flushes // its HAR writers and frees its response cache, so exactly one generation must // be alive per running set of targets. // @@ -28,15 +28,6 @@ func (c *Container) newProxy() *Proxy { } func (p *Proxy) Start(ctx context.Context, uncorsConfig *config.UncorsConfig) error { - output := p.container.CliOutput() - - tui.PrintLogo(output, p.container.Version()) - output.Print("") - output.WarnBox(tui.DisclaimerMessage) - output.Print("") - output.InfoBox(uncorsConfig.Mappings.String()) - output.Print("") - runtime, err := p.container.BuildRuntime(uncorsConfig) if err != nil { return err @@ -56,9 +47,6 @@ func (p *Proxy) Start(ctx context.Context, uncorsConfig *config.UncorsConfig) er // config that fails to build leaves the proxy serving the previous generation // untouched. The old generation is released only once the new one is live. func (p *Proxy) Restart(ctx context.Context, uncorsConfig *config.UncorsConfig) error { - output := p.container.CliOutput() - output.Info("Restarting server....") - runtime, err := p.container.BuildRuntime(uncorsConfig) if err != nil { return err @@ -71,8 +59,6 @@ func (p *Proxy) Restart(ctx context.Context, uncorsConfig *config.UncorsConfig) previous := p.swap(runtime) - output.InfoBox("Server restarted", uncorsConfig.Mappings.String()) - return closeRuntime(previous) } diff --git a/internal/render/render.go b/internal/render/render.go new file mode 100644 index 00000000..7927177b --- /dev/null +++ b/internal/render/render.go @@ -0,0 +1,88 @@ +// Package render turns application events into console output. +// +// It is the only place that decides how a service event looks. Both run modes +// use it, so neither can drift from the other in what it reports; the service +// itself renders nothing. +package render + +import ( + "github.com/evg4b/uncors/internal/app" + "github.com/evg4b/uncors/internal/contracts" + "github.com/evg4b/uncors/internal/tui" +) + +// Renderer writes service events to a contracts.Output. +type Renderer struct { + output contracts.Output + version string +} + +func New(output contracts.Output, version string) *Renderer { + return &Renderer{output: output, version: version} +} + +// Consume renders every event on the stream and returns when it closes. +func (r *Renderer) Consume(events <-chan app.Event) { + for event := range events { + r.Render(event) + } +} + +// Render writes a single event. +func (r *Renderer) Render(event app.Event) { + switch typed := event.(type) { + case app.LifecycleEvent: + r.lifecycle(typed) + case app.LogEvent: + r.log(typed) + } +} + +func (r *Renderer) lifecycle(event app.LifecycleEvent) { + switch event.State { + case app.StateStarting: + r.banner(event) + case app.StateStarted: + case app.StateStartFailed: + r.output.Errorf("Failed to start server: %v", event.Err) + case app.StateReloading: + r.output.Info("Restarting server....") + case app.StateReloaded: + r.output.InfoBox("Server restarted", event.Mappings.String()) + case app.StateReloadFailed: + r.output.Errorf("Failed to reload config: %v", event.Err) + case app.StateStopping: + if event.Interrupted { + // Move past the "^C" the terminal echoed. + _, _ = r.output.Write([]byte("\n")) + } + case app.StateStopped: + } +} + +// banner is the startup splash: the logo, the development-only disclaimer and +// the mappings the server came up with. +func (r *Renderer) banner(event app.LifecycleEvent) { + tui.PrintLogo(r.output, r.version) + r.output.Print("") + r.output.WarnBox(tui.DisclaimerMessage) + r.output.Print("") + r.output.InfoBox(event.Mappings.String()) + r.output.Print("") +} + +func (r *Renderer) log(event app.LogEvent) { + output := r.output + if event.Prefix != "" { + output = output.NewPrefixOutput(event.Prefix) + } + + switch event.Level { + case app.LevelInfo: + output.Info(event.Message) + case app.LevelWarn: + output.Warn(event.Message) + case app.LevelError: + output.Error(event.Message) + } +} diff --git a/internal/render/render_test.go b/internal/render/render_test.go new file mode 100644 index 00000000..6d7d0ce3 --- /dev/null +++ b/internal/render/render_test.go @@ -0,0 +1,116 @@ +package render_test + +import ( + "bytes" + "testing" + + "github.com/evg4b/uncors/internal/app" + "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/render" + "github.com/evg4b/uncors/internal/tui" + "github.com/evg4b/uncors/testing/hosts" + "github.com/evg4b/uncors/testing/testutils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var errReload = assert.AnError + +func mappings() config.Mappings { + return config.Mappings{ + {From: hosts.Localhost.HTTPPort(3000), To: hosts.Github.HTTPS()}, + } +} + +func renderOne(t *testing.T, event app.Event) string { + t.Helper() + + var buf bytes.Buffer + + render.New(tui.NewCliOutput(&buf), "v1.2.3").Render(event) + + return buf.String() +} + +// The rendered console output is the user-visible contract of this refactor. +// These snapshots are what prove moving it out of di.Proxy changed nothing. +func TestRenderLifecycle(t *testing.T) { + testCases := []struct { + name string + event app.Event + }{ + { + name: "startup banner", + event: app.LifecycleEvent{State: app.StateStarting, Mappings: mappings()}, + }, + { + name: "started is silent, the banner already said it", + event: app.LifecycleEvent{State: app.StateStarted, Mappings: mappings()}, + }, + { + name: "reloading", + event: app.LifecycleEvent{State: app.StateReloading}, + }, + { + name: "reloaded", + event: app.LifecycleEvent{State: app.StateReloaded, Mappings: mappings()}, + }, + { + name: "reload failed", + event: app.LifecycleEvent{State: app.StateReloadFailed, Err: errReload}, + }, + { + name: "start failed", + event: app.LifecycleEvent{State: app.StateStartFailed, Err: errReload}, + }, + { + name: "interrupted stop moves past the echoed ^C", + event: app.LifecycleEvent{State: app.StateStopping, Interrupted: true}, + }, + { + name: "non-interrupted stop prints nothing", + event: app.LifecycleEvent{State: app.StateStopping}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + testutils.MatchSnapshot(t, renderOne(t, testCase.event)) + }) + } +} + +func TestRenderLog(t *testing.T) { + levels := map[string]app.Level{ + "info": app.LevelInfo, + "warn": app.LevelWarn, + "error": app.LevelError, + } + + for name, level := range levels { + t.Run(name, func(t *testing.T) { + testutils.MatchSnapshot(t, renderOne(t, app.LogEvent{Level: level, Message: "something happened"})) + }) + } +} + +func TestConsumeDrainsUntilTheStreamCloses(t *testing.T) { + var buf bytes.Buffer + + events := make(chan app.Event, 2) + + for _, message := range []string{"first", "second"} { + events <- app.LogEvent{Level: app.LevelInfo, Message: message} + } + + close(events) + + render.New(tui.NewCliOutput(&buf), "v1.2.3").Consume(events) + + assert.Contains(t, buf.String(), "first") + assert.Contains(t, buf.String(), "second") +} + +func TestStoppedIsSilent(t *testing.T) { + require.Empty(t, renderOne(t, app.LifecycleEvent{State: app.StateStopped})) +} diff --git a/internal/uncors_app/app.go b/internal/uncors_app/app.go index ed66fb47..8a023db0 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -14,6 +14,7 @@ import ( "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/helpers" + "github.com/evg4b/uncors/internal/render" "github.com/evg4b/uncors/internal/server" ) @@ -31,8 +32,9 @@ type UncorsApp struct { // and renders what comes back. service *app.Service - output *tuiOutput - tracker server.IRequestTracker + output *tuiOutput + renderer *render.Renderer + tracker server.IRequestTracker outputCh chan string done <-chan struct{} @@ -46,6 +48,8 @@ type UncorsApp struct { memWidget *MemoryWidget } +type serviceEventMsg struct{ event app.Event } + type ( serverStartedMsg struct{} serverErrMsg struct{ err error } @@ -83,6 +87,7 @@ func NewUncorsApp( keys: keys, service: service, output: output, + renderer: render.New(output, container.Version()), tracker: container.RequestTracker(), outputCh: outputCh, done: service.Context().Done(), @@ -100,6 +105,7 @@ func (m *UncorsApp) Init() tea.Cmd { m.startServerCmd(), m.waitOutputCmd(), m.watchEventsCmd(), + m.waitServiceEventCmd(), m.memWidget.Init(), m.trackerWidget.Init(), m.historyWidget.Init(), @@ -129,6 +135,11 @@ func (m *UncorsApp) Update(msg tea.Msg) (tea.Model, tea.Cmd) { cmds = append(cmds, m.watchEventsCmd()) + case serviceEventMsg: + m.renderer.Render(typedMsg.event) + + cmds = append(cmds, m.waitServiceEventCmd()) + case tea.KeyPressMsg: log.Printf("Key pressed: %s", typedMsg.String()) @@ -336,6 +347,23 @@ func (m *UncorsApp) watchEventsCmd() tea.Cmd { } } +// waitServiceEventCmd pulls one service event and renders it into the history. +// Re-armed on every serviceEventMsg, the way the other stream readers are. +func (m *UncorsApp) waitServiceEventCmd() tea.Cmd { + return func() tea.Msg { + select { + case event, ok := <-m.service.Events(): + if !ok { + return nil + } + + return serviceEventMsg{event: event} + case <-m.done: + return nil + } + } +} + func (m *UncorsApp) shutdownCmd() tea.Cmd { return func() tea.Msg { ctx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) diff --git a/internal/uncors_app/app_internal_test.go b/internal/uncors_app/app_internal_test.go index 4ee05c92..2f0be328 100644 --- a/internal/uncors_app/app_internal_test.go +++ b/internal/uncors_app/app_internal_test.go @@ -3,12 +3,12 @@ package uncorsapp import ( "errors" "net/url" - "strings" "testing" "time" "charm.land/bubbles/v2/spinner" tea "charm.land/bubbletea/v2" + "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" @@ -346,18 +346,18 @@ func TestReloadWithFailingConfigLoadKeepsServing(t *testing.T) { defer testutils.Close(t, container) loadCalls := 0 - app := NewUncorsApp(container, "", cfg, func() (*config.UncorsConfig, error) { + model := NewUncorsApp(container, "", cfg, func() (*config.UncorsConfig, error) { loadCalls++ return nil, errBoom }) - defer cleanupTestApp(t, app) + defer cleanupTestApp(t, model) - require.NoError(t, app.service.Start(app.service.Context())) + require.NoError(t, model.service.Start(model.service.Context())) require.NotPanics(t, func() { - msg := app.restartCmd()() + msg := model.restartCmd()() assert.IsType(t, restartMsg{}, msg) }) @@ -365,13 +365,7 @@ func TestReloadWithFailingConfigLoadKeepsServing(t *testing.T) { assert.Equal(t, 1, loadCalls) assert.False(t, testutils.IsPortFree(port), "the previous generation must still be bound") - var reported bool - - for len(app.outputCh) > 0 { - if strings.Contains(<-app.outputCh, "Failed to reload config") { - reported = true - } - } - - assert.True(t, reported, "the load failure must be reported to the user") + status := model.service.Status() + assert.Equal(t, app.StateReloadFailed, status.State) + require.ErrorIs(t, status.Err, errBoom) } diff --git a/testing/snapshots/internal/render/render_test.snap b/testing/snapshots/internal/render/render_test.snap new file mode 100644 index 00000000..5e398745 --- /dev/null +++ b/testing/snapshots/internal/render/render_test.snap @@ -0,0 +1,66 @@ + +[TestRenderLifecycle/startup_banner - 1] +██ ██ ███ ██  ██████ ██████ ██████ ███████ +██ ██ ████ ██ ██ ██ ██ ██ ██ ██  +██ ██ ██ ██ ██ ██ ██ ██ ██████ ███████ +██ ██ ██ ██ ██ ██ ██ ██ ██ ██ ██ + ██████ ██ ████  ██████ ██████ ██ ██ ███████ + version: v1.2.3 + + WARN   DON'T USE IT FOR PRODUCTION! +    This is a reverse proxy for use in testing or debugging web applications locally. +    It hasn't been reviewed for security issues. + + INFO   http://localhost:3000 => https://github.com + + +--- + +[TestRenderLifecycle/started_is_silent,_the_banner_already_said_it - 1] + +--- + +[TestRenderLifecycle/reloading - 1] + INFO   Restarting server.... + +--- + +[TestRenderLifecycle/reloaded - 1] + INFO   Server restarted +    http://localhost:3000 => https://github.com + +--- + +[TestRenderLifecycle/reload_failed - 1] + ERROR   Failed to reload config: assert.AnError general error for testing + +--- + +[TestRenderLifecycle/start_failed - 1] + ERROR   Failed to start server: assert.AnError general error for testing + +--- + +[TestRenderLifecycle/interrupted_stop_moves_past_the_echoed_^C - 1] + + +--- + +[TestRenderLifecycle/non-interrupted_stop_prints_nothing - 1] + +--- + +[TestRenderLog/info - 1] + INFO   something happened + +--- + +[TestRenderLog/warn - 1] + WARN   something happened + +--- + +[TestRenderLog/error - 1] + ERROR   something happened + +--- From ebd72050ec993d22ca2f0cc68547c22843000693 Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 17:56:37 -0400 Subject: [PATCH 4/7] refactor: make the service/TUI boundary explicit and enforce it (Phase 3) Phase 1 already removed the model's di.Proxy field, so the remaining work was to make the boundary something the compiler and CI can hold. - UncorsApp now depends on a narrow service interface declared at the point of use - Start, Reload, Shutdown, Close, Context, Events - rather than on *app.Service. Commands go down, events come back up, and the model reaches for nothing else. - tests/architecture asserts that internal/app, di, server, handler and config do not depend on Bubble Tea or Bubbles, directly or transitively, and do not depend on the TUI package. Verified the guard fails when the import is actually added. - The guard shells out to go list, which Go's test cache cannot see, so it also reads the sources it guards. Without that a violating import could be masked by a cached pass; verified invalidation works from a primed cache. Lip Gloss is deliberately not yet in the forbidden set: internal/di still styles handler prefixes through internal/tui/styles. Phase 5 removes that and adds it. No command queue: the plan's command set is two entries and process separation is out of scope, so a queue would be machinery without a consumer. Co-Authored-By: Claude Opus 5 --- internal/uncors_app/app.go | 14 +++- internal/uncors_app/app_internal_test.go | 7 +- tests/architecture/boundary_test.go | 97 ++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 tests/architecture/boundary_test.go diff --git a/internal/uncors_app/app.go b/internal/uncors_app/app.go index 8a023db0..e2854513 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -30,7 +30,7 @@ type UncorsApp struct { // service owns the application runtime. The model only sends it commands // and renders what comes back. - service *app.Service + service service output *tuiOutput renderer *render.Renderer @@ -48,6 +48,18 @@ type UncorsApp struct { memWidget *MemoryWidget } +// service is the whole of the model's dependency on the application. Commands +// go down (Start, Reload, Shutdown), events come back up; the model reaches +// for nothing else, which is what keeps application behaviour out of the TUI. +type service interface { + Start(ctx context.Context) error + Reload() + Shutdown(ctx context.Context) error + Close() error + Context() context.Context + Events() <-chan app.Event +} + type serviceEventMsg struct{ event app.Event } type ( diff --git a/internal/uncors_app/app_internal_test.go b/internal/uncors_app/app_internal_test.go index 2f0be328..cbf2d780 100644 --- a/internal/uncors_app/app_internal_test.go +++ b/internal/uncors_app/app_internal_test.go @@ -365,7 +365,12 @@ func TestReloadWithFailingConfigLoadKeepsServing(t *testing.T) { assert.Equal(t, 1, loadCalls) assert.False(t, testutils.IsPortFree(port), "the previous generation must still be bound") - status := model.service.Status() + // The model's own interface is deliberately narrow, so reach for the + // concrete service to assert on the state it recorded. + service, ok := model.service.(*app.Service) + require.True(t, ok) + + status := service.Status() assert.Equal(t, app.StateReloadFailed, status.State) require.ErrorIs(t, status.Err, errBoom) } diff --git a/tests/architecture/boundary_test.go b/tests/architecture/boundary_test.go new file mode 100644 index 00000000..35eb602c --- /dev/null +++ b/tests/architecture/boundary_test.go @@ -0,0 +1,97 @@ +// Package architecture_test enforces the service/TUI boundary in CI. +// +// The point of separating the service from Bubble Tea is easy to state and +// easy to erode: one convenient import puts terminal concerns back into the +// application. These tests fail the build when that happens. +package architecture_test + +import ( + "io/fs" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// serviceSide is every package that must run correctly with no TUI attached. +var serviceSide = []string{ + "github.com/evg4b/uncors/internal/app/...", + "github.com/evg4b/uncors/internal/di/...", + "github.com/evg4b/uncors/internal/server/...", + "github.com/evg4b/uncors/internal/handler/...", + "github.com/evg4b/uncors/internal/config/...", +} + +// forbidden are the terminal-interaction libraries. Lip Gloss is deliberately +// absent for now: internal/di still styles handler prefixes through +// internal/tui/styles, which Phase 5 of the migration removes. +var forbidden = []string{ + "charm.land/bubbletea", + "charm.land/bubbles", +} + +func dependenciesOf(t *testing.T, pattern string) []string { + t.Helper() + + out, err := exec.Command("go", "list", "-deps", pattern).Output() //nolint:noctx // build-graph query + require.NoError(t, err, "go list failed for %s", pattern) + + return strings.Split(strings.TrimSpace(string(out)), "\n") +} + +// readGuardedSources exists for its side effect. The build graph is read by +// shelling out to go list, which Go's test cache cannot see, so a violating +// import could otherwise be masked by a cached pass. Opening the files the +// test guards puts them in the cache key. +func readGuardedSources(t *testing.T) { + t.Helper() + + err := filepath.WalkDir("../../internal", func(path string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() || !strings.HasSuffix(path, ".go") { + return err //nolint:wrapcheck // walk error, propagated as-is + } + + _, readErr := os.ReadFile(path) //nolint:gosec // fixed, repo-relative tree + + return readErr + }) + require.NoError(t, err) +} + +// T10: the service must not depend on the TUI toolkit, directly or through +// anything it imports. +func TestServiceDoesNotDependOnBubbleTea(t *testing.T) { + readGuardedSources(t) + + for _, pattern := range serviceSide { + t.Run(pattern, func(t *testing.T) { + deps := dependenciesOf(t, pattern) + + for _, dep := range deps { + for _, banned := range forbidden { + assert.NotContains(t, dep, banned, + "%s must run without a TUI, but depends on %s", pattern, dep) + } + } + }) + } +} + +// The TUI is allowed to know about the service. The reverse is what breaks the +// architecture, and Go's import cycle rules do not catch it on their own. +func TestServiceDoesNotDependOnTheTUIPackage(t *testing.T) { + readGuardedSources(t) + + for _, pattern := range serviceSide { + t.Run(pattern, func(t *testing.T) { + for _, dep := range dependenciesOf(t, pattern) { + assert.NotEqual(t, "github.com/evg4b/uncors/internal/uncors_app", dep, + "%s must not depend on the TUI", pattern) + } + }) + } +} From bccfd2f101568ffe80978bcd2275952251d98e4e Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 18:01:28 -0400 Subject: [PATCH 5/7] refactor: move application state out of the TUI widgets (Phase 4) The set of in-flight requests was a fact about the server that only a Bubble Tea widget knew, rebuilt purely from the events that widget happened to witness. That is what caused the stale-rows bug: TrackerWidget cleared itself on the restart key but not on a reload triggered by saving the config file, so the UI kept showing requests from a generation that no longer existed. - The service is now the single consumer of the request tracker. It maintains the authoritative in-flight set, exposes it through InFlight(), and republishes activity on its own event stream. - A completed reload clears that set, so both reload paths behave identically. The widgets learn about it from StateReloaded rather than from a message the restart key synthesised, which is what removes the asymmetry. - The TUI no longer reads the tracker, and request rendering moved into internal/render, so headless drops the separate RequestPrinter goroutine and renders requests the same way it renders everything else. - History scrollback is capped at 10,000 lines. It grew without limit before, one line per request, for as long as the process lived. The existing test asserting unbounded growth is updated to assert the cap. Verified headless output is byte-identical to the previous commit across start, two requests, reload, a request on the new port, a rejected config and SIGTERM. Co-Authored-By: Claude Opus 5 --- internal/app/events.go | 11 ++ internal/app/service.go | 72 ++++++++++++- internal/app/service_test.go | 70 ++++++++++++ internal/cli/run_non_interactive.go | 8 +- internal/render/render.go | 18 ++++ internal/uncors_app/app.go | 55 ++++------ internal/uncors_app/app_internal_test.go | 108 ++++++++++++------- internal/uncors_app/history.go | 14 ++- internal/uncors_app/history_internal_test.go | 32 +++++- 9 files changed, 305 insertions(+), 83 deletions(-) diff --git a/internal/app/events.go b/internal/app/events.go index 2b55eca1..7e209f6a 100644 --- a/internal/app/events.go +++ b/internal/app/events.go @@ -5,6 +5,7 @@ import ( "sync/atomic" "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/server" ) const eventsBufferSize = 1000 @@ -63,6 +64,16 @@ type LogEvent struct { func (LogEvent) isEvent() {} +// RequestEvent forwards one server request lifecycle event to the presenter. +// The service is the single consumer of the request tracker, because it keeps +// the authoritative set of in-flight requests; clients render what it passes +// on rather than reading the tracker themselves. +type RequestEvent struct { + Event server.RequestEvent +} + +func (RequestEvent) isEvent() {} + // Status is the latest lifecycle state, always readable regardless of whether // the notification for it was delivered. type Status struct { diff --git a/internal/app/service.go b/internal/app/service.go index d3746a15..972e22aa 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -8,17 +8,20 @@ package app import ( + "cmp" "context" "fmt" "log" "os" "os/signal" + "slices" "sync" "syscall" "time" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/di" + "github.com/evg4b/uncors/internal/server" ) const ( @@ -53,7 +56,15 @@ type Service struct { reloading bool pending bool - events *emitter + events *emitter + tracker server.IRequestTracker + + // inFlightMu guards the authoritative set of requests currently being + // served. It lives here rather than in a UI widget because it is a fact + // about the server, and because a reload has to be able to clear it. + inFlightMu sync.RWMutex + inFlight map[uint64]server.RequestEvent + watcher *config.Watcher shutdownOne sync.Once } @@ -64,7 +75,7 @@ type Service struct { func New(container *di.Container, cfg *config.UncorsConfig, configPath string, load Loader) *Service { ctx, cancel := context.WithCancel(context.Background()) - return &Service{ + service := &Service{ container: container, proxy: container.Proxy(), configPath: configPath, @@ -73,7 +84,34 @@ func New(container *di.Container, cfg *config.UncorsConfig, configPath string, l cancel: cancel, cfg: cfg, events: newEmitter(), + tracker: container.RequestTracker(), + inFlight: map[uint64]server.RequestEvent{}, } + + // Start pumping before anything can serve a request, so no activity is + // missed between construction and Start. + go service.pumpRequests() + + return service +} + +// InFlight returns the requests currently being served, oldest first. A client +// that connects late, or one that lost track, can rebuild its view from this +// rather than from the events it happened to witness. +func (s *Service) InFlight() []server.RequestEvent { + s.inFlightMu.RLock() + defer s.inFlightMu.RUnlock() + + requests := make([]server.RequestEvent, 0, len(s.inFlight)) + for _, request := range s.inFlight { + requests = append(requests, request) + } + + slices.SortFunc(requests, func(a, b server.RequestEvent) int { + return cmp.Compare(a.ID, b.ID) + }) + + return requests } // Events returns the service's event stream. It has a single consumer: the TUI @@ -219,6 +257,31 @@ func (s *Service) Close() error { return nil } +// pumpRequests owns the request tracker. It maintains the in-flight set and +// forwards every event to the presenter. +func (s *Service) pumpRequests() { + for event := range s.tracker.Events() { + s.inFlightMu.Lock() + + if event.Done { + delete(s.inFlight, event.ID) + } else if event.URL != nil { + s.inFlight[event.ID] = event + } + + s.inFlightMu.Unlock() + + s.events.send(RequestEvent{Event: event}) + } +} + +func (s *Service) clearInFlight() { + s.inFlightMu.Lock() + defer s.inFlightMu.Unlock() + + clear(s.inFlight) +} + func (s *Service) reloadOnce() { reloaded, err := s.load() if err != nil { @@ -242,6 +305,11 @@ func (s *Service) reloadOnce() { s.cfg = reloaded s.mu.Unlock() + // The generation those requests belonged to is gone; anything still tracked + // against it would linger forever. This is why a reload triggered by a file + // save now clears the view the same way the restart key always did. + s.clearInFlight() + s.events.EmitLifecycle(LifecycleEvent{State: StateReloaded, Mappings: reloaded.Mappings}) } diff --git a/internal/app/service_test.go b/internal/app/service_test.go index 68b08414..db29e679 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -4,6 +4,7 @@ import ( "errors" "net" "net/http" + "net/url" "os" "path/filepath" "strconv" @@ -14,6 +15,7 @@ import ( "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/di" + "github.com/evg4b/uncors/internal/server" "github.com/evg4b/uncors/testing/hosts" "github.com/evg4b/uncors/testing/testutils" "github.com/stretchr/testify/assert" @@ -237,3 +239,71 @@ func TestServiceShutdownIsIdempotent(t *testing.T) { assert.Error(t, service.Context().Err(), "shutdown must cancel the service context") } + +// T6 / P3: the in-flight set is application state, so a reload clears it no +// matter what triggered the reload. Previously the TUI cleared its own copy +// only on the restart key, and a config file save left stale rows on screen. +func TestReloadClearsInFlightRequests(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + container := di.NewContainer() + service := app.New(container, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + t.Cleanup(func() { + require.NoError(t, service.Shutdown(t.Context())) + require.NoError(t, service.Close()) + require.NoError(t, container.Close()) + }) + + require.NoError(t, service.Start(t.Context())) + + requestURL, err := url.Parse("http://localhost/slow") + require.NoError(t, err) + + // A request that started but never finished, exactly what a reload strands. + container.RequestTracker().Emit(server.RequestEvent{ID: 1, Method: "GET", URL: requestURL}) + + require.Eventually(t, func() bool { return len(service.InFlight()) == 1 }, + time.Second, 5*time.Millisecond, "the service must track the started request") + + service.Reload() + + assert.Empty(t, service.InFlight(), "a reload must not leave requests from the old generation in flight") +} + +func TestInFlightIsOrderedAndDrains(t *testing.T) { + port := testutils.GetFreePort(t) + cfg := configFor(port) + + container := di.NewContainer() + service := app.New(container, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + t.Cleanup(func() { + require.NoError(t, service.Close()) + require.NoError(t, container.Close()) + }) + + requestURL, err := url.Parse("http://localhost/x") + require.NoError(t, err) + + tracker := container.RequestTracker() + for id := uint64(3); id >= 1; id-- { + tracker.Emit(server.RequestEvent{ID: id, Method: "GET", URL: requestURL}) + } + + require.Eventually(t, func() bool { return len(service.InFlight()) == 3 }, + time.Second, 5*time.Millisecond) + + ids := make([]uint64, 0, 3) + for _, request := range service.InFlight() { + ids = append(ids, request.ID) + } + + assert.Equal(t, []uint64{1, 2, 3}, ids, "in-flight requests must be ordered oldest first") + + tracker.Emit(server.RequestEvent{ID: 2, Done: true}) + + require.Eventually(t, func() bool { return len(service.InFlight()) == 2 }, + time.Second, 5*time.Millisecond, "a completed request must leave the in-flight set") +} diff --git a/internal/cli/run_non_interactive.go b/internal/cli/run_non_interactive.go index a706dd4b..304e15d8 100644 --- a/internal/cli/run_non_interactive.go +++ b/internal/cli/run_non_interactive.go @@ -8,7 +8,6 @@ import ( "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/render" - "github.com/evg4b/uncors/internal/server" ) // runNonInteractive starts the proxy in headless mode and blocks until the @@ -22,12 +21,11 @@ func runNonInteractive( cfgPath string, ) error { output := container.CliOutput() - - // Headless mode has no TUI, so it must drain the request tracker itself; - // without a consumer the request path can only drop activity events. tracker := container.RequestTracker() - go server.RequestPrinter(tracker, output) + // The service drains the request tracker and republishes activity on its own + // stream, so headless mode renders requests the same way it renders + // everything else. service := app.New(container, cfg, cfgPath, configLoader(container)) defer func() { _ = service.Close() }() diff --git a/internal/render/render.go b/internal/render/render.go index 7927177b..9de19126 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -35,9 +35,27 @@ func (r *Renderer) Render(event app.Event) { r.lifecycle(typed) case app.LogEvent: r.log(typed) + case app.RequestEvent: + r.request(typed) } } +// request renders a completed request. Start events carry no result yet, so +// only the terminal one produces a line - the same rule the console printer +// has always applied. +func (r *Renderer) request(event app.RequestEvent) { + if !event.Event.Done || event.Event.Data == nil { + return + } + + output := r.output + if event.Event.Prefix != "" { + output = output.NewPrefixOutput(event.Event.Prefix) + } + + output.Request(event.Event.Data) +} + func (r *Renderer) lifecycle(event app.LifecycleEvent) { switch event.State { case app.StateStarting: diff --git a/internal/uncors_app/app.go b/internal/uncors_app/app.go index e2854513..94bbf0cd 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -15,7 +15,6 @@ import ( "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/helpers" "github.com/evg4b/uncors/internal/render" - "github.com/evg4b/uncors/internal/server" ) const ( @@ -34,7 +33,6 @@ type UncorsApp struct { output *tuiOutput renderer *render.Renderer - tracker server.IRequestTracker outputCh chan string done <-chan struct{} @@ -100,7 +98,6 @@ func NewUncorsApp( service: service, output: output, renderer: render.New(output, container.Version()), - tracker: container.RequestTracker(), outputCh: outputCh, done: service.Context().Done(), historyWidget: NewHistoryWidget(keys), @@ -116,7 +113,6 @@ func (m *UncorsApp) Init() tea.Cmd { return tea.Batch( m.startServerCmd(), m.waitOutputCmd(), - m.watchEventsCmd(), m.waitServiceEventCmd(), m.memWidget.Init(), m.trackerWidget.Init(), @@ -142,16 +138,18 @@ func (m *UncorsApp) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case outputLineMsg: cmds = append(cmds, m.waitOutputCmd()) - case requestEventMsg: - m.handleRequestEvent(typedMsg) - - cmds = append(cmds, m.watchEventsCmd()) - case serviceEventMsg: m.renderer.Render(typedMsg.event) cmds = append(cmds, m.waitServiceEventCmd()) + // Widgets react to the service's own account of what happened, so a + // reload triggered by a file save reaches them exactly as the restart + // key does. + if translated := widgetMessage(typedMsg.event); translated != nil { + msg = translated + } + case tea.KeyPressMsg: log.Printf("Key pressed: %s", typedMsg.String()) @@ -293,16 +291,20 @@ func (m *UncorsApp) handleServerError(msg serverErrMsg) tea.Cmd { return m.shutdownCmd() } -func (m *UncorsApp) handleRequestEvent(event requestEventMsg) { - if !event.Done || event.Data == nil { - return +// widgetMessage translates a service event into the message the widgets speak, +// or nil when no widget cares about it. +func widgetMessage(event app.Event) tea.Msg { + switch typed := event.(type) { + case app.RequestEvent: + return requestEventMsg(typed.Event) + case app.LifecycleEvent: + if typed.State == app.StateReloaded { + return restartMsg{} + } + case app.LogEvent: } - if event.Prefix != "" { - m.output.NewPrefixOutput(event.Prefix).Request(event.Data) - } else { - m.output.Request(event.Data) - } + return nil } func (m *UncorsApp) handleRestart() { @@ -344,21 +346,6 @@ func (m *UncorsApp) waitOutputCmd() tea.Cmd { } } -func (m *UncorsApp) watchEventsCmd() tea.Cmd { - return func() tea.Msg { - select { - case event, ok := <-m.tracker.Events(): - if !ok { - return nil - } - - return requestEventMsg(event) - case <-m.done: - return nil - } - } -} - // waitServiceEventCmd pulls one service event and renders it into the history. // Re-armed on every serviceEventMsg, the way the other stream readers are. func (m *UncorsApp) waitServiceEventCmd() tea.Cmd { @@ -394,8 +381,10 @@ func (m *UncorsApp) restartCmd() tea.Cmd { m.output.Errorf("Restart error: %v", value) }) + // The reload's effects arrive as service events, which is what the + // widgets act on; nothing to report from here. m.service.Reload() - return restartMsg{} + return nil } } diff --git a/internal/uncors_app/app_internal_test.go b/internal/uncors_app/app_internal_test.go index cbf2d780..bcf3c34e 100644 --- a/internal/uncors_app/app_internal_test.go +++ b/internal/uncors_app/app_internal_test.go @@ -3,6 +3,7 @@ package uncorsapp import ( "errors" "net/url" + "regexp" "testing" "time" @@ -21,9 +22,26 @@ import ( var errBoom = errors.New("boom") +var ansiPattern = regexp.MustCompile(`\x1b\[[0-9;]*m`) + +// stripANSI removes styling so assertions can talk about the text. Lip Gloss +// styles URLs one character at a time, so the plain string is never a +// contiguous substring of the rendered line. +func stripANSI(value string) string { + return ansiPattern.ReplaceAllString(value, "") +} + func newTestApp(t *testing.T) (*UncorsApp, *int) { t.Helper() + model, loadCalls, _ := newTestAppWithContainer(t) + + return model, loadCalls +} + +func newTestAppWithContainer(t *testing.T) (*UncorsApp, *int, *di.Container) { + t.Helper() + uncorsConfig := &config.UncorsConfig{ Mappings: config.Mappings{}, } @@ -46,7 +64,7 @@ func newTestApp(t *testing.T) (*UncorsApp, *int) { }, ) - return app, &loadCalls + return app, &loadCalls, container } func cleanupTestApp(t *testing.T, app *UncorsApp) { @@ -66,7 +84,7 @@ func TestNewUncorsAppAndKeyMap(t *testing.T) { defer cleanupTestApp(t, app) assert.NotNil(t, app.output) - assert.NotNil(t, app.tracker) + assert.NotNil(t, app.renderer) assert.NotNil(t, app.historyWidget.hist) assert.NotNil(t, app.service) assert.NotNil(t, app.done) @@ -110,7 +128,7 @@ func TestUncorsAppUpdateViewAndLayout(t *testing.T) { StartedAt: time.Now().Add(-1500 * time.Millisecond), }) require.Same(t, app, model) - require.NotNil(t, cmd) + require.NotNil(t, cmd) // the spinner starts ticking assert.Len(t, app.trackerWidget.pending, 1) assert.True(t, app.trackerWidget.ticking) @@ -121,9 +139,8 @@ func TestUncorsAppUpdateViewAndLayout(t *testing.T) { assert.Contains(t, view.Content, "GET") assert.Contains(t, view.Content, "example.com/demo") - model, cmd = app.Update(requestEventMsg{ID: 7, Done: true}) + model, _ = app.Update(requestEventMsg{ID: 7, Done: true}) require.Same(t, app, model) - require.NotNil(t, cmd) assert.Empty(t, app.trackerWidget.pending) model, cmd = app.Update(spinner.TickMsg{}) @@ -167,8 +184,7 @@ func TestUncorsAppCommandFactoriesAndChannels(t *testing.T) { // no follow-up command of its own. assert.Nil(t, app.handleServerStarted()) - msg = app.restartCmd()() - assert.Equal(t, restartMsg{}, msg) + assert.Nil(t, app.restartCmd()()) assert.Equal(t, 1, *loadCalls) msg = app.shutdownCmd()() @@ -195,31 +211,36 @@ func TestUncorsAppCommandFactoriesAndChannels(t *testing.T) { assert.Nil(t, app.waitOutputCmd()()) }) - t.Run("watchEventsCmd reads from event channel and handles shutdown", func(t *testing.T) { - app, _ := newTestApp(t) - defer cleanupTestApp(t, app) + t.Run("request activity arrives through the service stream", func(t *testing.T) { + model, _, container := newTestAppWithContainer(t) + defer cleanupTestApp(t, model) requestURL, err := url.Parse("https://example.com/watch") require.NoError(t, err) - app.tracker.Emit(server.RequestEvent{ID: 9, Method: "GET", URL: requestURL}) + emitted := server.RequestEvent{ID: 9, Method: "GET", URL: requestURL} + container.RequestTracker().Emit(emitted) - assert.Equal( - t, - requestEventMsg(server.RequestEvent{ID: 9, Method: "GET", URL: requestURL}), - app.watchEventsCmd()(), - ) + // The service is the single consumer of the tracker; the model sees + // activity only because the service republishes it. + msg := model.waitServiceEventCmd()() - require.NoError(t, app.service.Close()) - assert.Nil(t, app.watchEventsCmd()()) + event, ok := msg.(serviceEventMsg) + require.True(t, ok) + + request, ok := event.event.(app.RequestEvent) + require.True(t, ok) + assert.Equal(t, emitted, request.Event) + + assert.Equal(t, requestEventMsg(emitted), widgetMessage(request)) }) - t.Run("watchEventsCmd returns nil when event channel is closed", func(t *testing.T) { - app, _ := newTestApp(t) - defer cleanupTestApp(t, app) + t.Run("waitServiceEventCmd returns nil once the service is closed", func(t *testing.T) { + model, _ := newTestApp(t) + defer cleanupTestApp(t, model) - app.tracker.Close() - assert.Nil(t, app.watchEventsCmd()()) + require.NoError(t, model.service.Close()) + assert.Nil(t, model.waitServiceEventCmd()()) }) } @@ -256,7 +277,10 @@ func TestUncorsAppKeyHandlingAndMessages(t *testing.T) { _, cmd = app.Update(tea.KeyPressMsg(tea.Key{Text: "r", Code: 'r'})) require.NotNil(t, cmd) - assert.Equal(t, restartMsg{}, cmd()) + // Reload is a command, not a result: the widgets learn about it from the + // service's StateReloaded event, which is what makes a file-triggered + // reload behave identically to this key. + assert.Nil(t, cmd()) _, cmd = app.Update(tea.KeyPressMsg(tea.Key{Text: "q", Code: 'q'})) require.NotNil(t, cmd) @@ -310,25 +334,35 @@ func TestServerStartedMsgUpdate(t *testing.T) { assert.Nil(t, app.handleServerStarted()) } -func TestHandleRequestEventWithData(t *testing.T) { +func TestRequestEventsAreRenderedIntoHistory(t *testing.T) { requestURL, err := url.Parse("https://example.com/api") require.NoError(t, err) data := &contracts.RequestData{Method: "GET", URL: requestURL, Code: 200} - t.Run("outputs request without prefix", func(t *testing.T) { - app, _ := newTestApp(t) - defer cleanupTestApp(t, app) + testCases := []struct { + name string + prefix string + }{ + {name: "without prefix"}, + {name: "with prefix", prefix: "api"}, + } - app.handleRequestEvent(requestEventMsg{Done: true, Data: data}) - }) + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + model, _ := newTestApp(t) + defer cleanupTestApp(t, model) - t.Run("outputs request with prefix", func(t *testing.T) { - app, _ := newTestApp(t) - defer cleanupTestApp(t, app) + // Rendering lives in internal/render now; the model just hands the + // event over and the line lands on the output channel. + model.renderer.Render(app.RequestEvent{ + Event: server.RequestEvent{Done: true, Data: data, Prefix: testCase.prefix}, + }) - app.handleRequestEvent(requestEventMsg{Done: true, Data: data, Prefix: "api"}) - }) + require.NotEmpty(t, model.outputCh) + assert.Contains(t, stripANSI(<-model.outputCh), "example.com/api") + }) + } } // A config that fails to parse or validate must leave the running generation @@ -357,9 +391,7 @@ func TestReloadWithFailingConfigLoadKeepsServing(t *testing.T) { require.NoError(t, model.service.Start(model.service.Context())) require.NotPanics(t, func() { - msg := model.restartCmd()() - - assert.IsType(t, restartMsg{}, msg) + assert.Nil(t, model.restartCmd()()) }) assert.Equal(t, 1, loadCalls) diff --git a/internal/uncors_app/history.go b/internal/uncors_app/history.go index 957d4436..89a03111 100644 --- a/internal/uncors_app/history.go +++ b/internal/uncors_app/history.go @@ -8,9 +8,15 @@ import ( const ( historyInitialCapacity = 1024 + + // historyMaxLines bounds the scrollback. A long-running proxy logs a line + // per request, so an unbounded buffer grows for as long as the process + // lives; the oldest lines are the ones a user is least likely to want. + historyMaxLines = 10_000 ) -// history stores log lines in memory without a fixed limit. +// history stores the most recent log lines in memory, discarding the oldest +// once historyMaxLines is reached. type history struct { mu sync.RWMutex lines []string @@ -34,6 +40,12 @@ func (h *history) AppendLine(line string) { newLines := strings.Split(line, "\n") h.lines = append(h.lines, newLines...) + if overflow := len(h.lines) - historyMaxLines; overflow > 0 { + // Copy down rather than reslicing, so the backing array of the dropped + // lines can actually be collected. + h.lines = append(h.lines[:0], h.lines[overflow:]...) + } + log.Printf("Appended %d lines to history (total lines: %d)", len(newLines), len(h.lines)) } diff --git a/internal/uncors_app/history_internal_test.go b/internal/uncors_app/history_internal_test.go index 3155e37e..ed2ce34b 100644 --- a/internal/uncors_app/history_internal_test.go +++ b/internal/uncors_app/history_internal_test.go @@ -1,6 +1,7 @@ package uncorsapp import ( + "strconv" "strings" "testing" @@ -91,17 +92,16 @@ func TestHistory_AppendLine(t *testing.T) { assert.Equal(t, styled, lines[0]) }) - t.Run("handles large number of lines", func(t *testing.T) { + t.Run("caps a large number of lines at the scrollback limit", func(t *testing.T) { history := newHistory() defer testutils.Close(t, history) - count := 20000 - for i := range count { + for i := range historyMaxLines * 2 { history.AppendLine(strings.Repeat("a", i%100)) } - assert.Equal(t, count, history.LineCount()) + assert.Equal(t, historyMaxLines, history.LineCount()) }) } @@ -157,3 +157,27 @@ func TestHistory_Lines(t *testing.T) { assert.Len(t, history.Lines(), 2) }) } + +func TestHistoryIsBounded(t *testing.T) { + hist := newHistory() + + for i := range historyMaxLines + 500 { + hist.AppendLine(strconv.Itoa(i)) + } + + assert.Equal(t, historyMaxLines, hist.LineCount(), "history must not grow without limit") + + lines := hist.Lines() + assert.Equal(t, strconv.Itoa(historyMaxLines+499), lines[len(lines)-1], "the newest line must survive") + assert.Equal(t, strconv.Itoa(500), lines[0], "the oldest lines must be the ones dropped") +} + +func TestHistoryBoundsMultiLineAppends(t *testing.T) { + hist := newHistory() + + for range historyMaxLines { + hist.AppendLine("a\nb\nc") + } + + assert.Equal(t, historyMaxLines, hist.LineCount()) +} From 0856dd87e0cb777302b9422c6abdddab16a526b3 Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 18:10:55 -0400 Subject: [PATCH 6/7] refactor: remove the terminal libraries from the service layer (Phase 5) The service styled its own output. di rendered handler badges with Lip Gloss, config imported the console package to draw --help, and the version checker imported it for one message constant. Between them they pulled Lip Gloss into every service package, so "the service does not depend on the TUI" was not actually true. - di now emits plain handler names - PROXY, MOCK, CACHE and so on - and internal/tui styles them when it renders a prefix. The badge lookup is the only place that decision is made. - config takes a usage renderer as an option instead of reaching for the console; the CLI supplies it, which is the only place --help is reachable. - The new-version notice moved to internal/version, where it is emitted. - The container defaults to a null output and the composition root installs the real one, so di no longer imports internal/tui at all. Building the generate-certs command moved to the CLI for the same reason. - The TUI's output adapter rendered every message into a throwaway CliOutput and pushed the resulting string. It is now just an io.Writer over a channel: the service hands the model structured events, internal/render decides what they say, and CliOutput decides how they look. - Overriding CliOutput after it has been built now panics. It was a silent no-op that would have sent output to the terminal underneath the TUI. Two tests were asserting that footgun and now assert the contract instead. internal/app, di, server, handler, config and version are now free of charm.land entirely, and tests/architecture enforces it. Verified byte-identical console output against the previous commit with ANSI included, covering the proxy and mock badges, reload and a rejected config. Co-Authored-By: Claude Opus 5 --- internal/cli/generate_certs.go | 6 +- internal/cli/run_uncors.go | 10 +- internal/config/config.go | 14 +- internal/config/flags.go | 29 +++- internal/di/container.go | 19 +-- internal/di/factories.go | 11 +- internal/di/factory.go | 9 ++ internal/di/noop_output.go | 29 ++++ internal/di/override.go | 13 +- internal/di/public_api.go | 18 +-- internal/di/public_api_test.go | 49 ++---- internal/di/runtime.go | 3 +- internal/tui/messages.go | 5 - internal/tui/messages_test.go | 6 - internal/tui/output.go | 4 +- internal/tui/styles/features.go | 25 +++ internal/uncors_app/app.go | 5 +- internal/uncors_app/output.go | 103 ++---------- internal/uncors_app/output_internal_test.go | 169 +++----------------- internal/version/check_new_version.go | 3 +- internal/version/messages.go | 9 ++ internal/version/new_version_check_test.go | 6 + main.go | 6 + tests/architecture/boundary_test.go | 28 ++-- 24 files changed, 239 insertions(+), 340 deletions(-) create mode 100644 internal/di/noop_output.go create mode 100644 internal/version/messages.go diff --git a/internal/cli/generate_certs.go b/internal/cli/generate_certs.go index d5baf26b..c2d5d052 100644 --- a/internal/cli/generate_certs.go +++ b/internal/cli/generate_certs.go @@ -3,6 +3,7 @@ package cli import ( "errors" + "github.com/evg4b/uncors/internal/commands" "github.com/evg4b/uncors/internal/di" "github.com/spf13/pflag" ) @@ -10,7 +11,10 @@ import ( const GenerateCertsCmd = "generate-certs" func GenerateCerts(container *di.Container) error { - cmd := container.GenerateCertsCommand() + cmd := commands.NewGenerateCertsCommand( + commands.WithOutput(container.CliOutput()), + commands.WithFs(container.Fs()), + ) flags := pflag.NewFlagSet(GenerateCertsCmd, pflag.ContinueOnError) cmd.DefineFlags(flags, container.Version()) diff --git a/internal/cli/run_uncors.go b/internal/cli/run_uncors.go index fb22a679..c8232d63 100644 --- a/internal/cli/run_uncors.go +++ b/internal/cli/run_uncors.go @@ -8,6 +8,7 @@ import ( "github.com/evg4b/uncors/internal/app" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/di" + "github.com/evg4b/uncors/internal/tui" "github.com/spf13/pflag" ) @@ -15,7 +16,14 @@ import ( // proxy until ctx is cancelled or a shutdown signal arrives. The --version and // --help flags print their output and return without starting anything. func RunUncors(ctx context.Context, container *di.Container) error { - uncorsConfig, cfgPath, err := config.LoadConfiguration(container.Fs(), container.Version(), container.Args()) + uncorsConfig, cfgPath, err := config.LoadConfiguration( + container.Fs(), + container.Version(), + container.Args(), + // Only the startup parse can reach --help, and drawing it is the CLI's + // job, not the config package's. + config.WithUsage(tui.PrintUsage), + ) if err != nil { switch { case errors.Is(err, config.ErrVersionRequested): diff --git a/internal/config/config.go b/internal/config/config.go index 7496ae68..78556c8a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,8 +20,18 @@ type UncorsConfig struct { Interactive bool `yaml:"-"` } -func LoadConfiguration(fs afero.Fs, version string, args []string) (*UncorsConfig, string, error) { - flags := defineFlags(version) +func LoadConfiguration( + fs afero.Fs, + version string, + args []string, + options ...Option, +) (*UncorsConfig, string, error) { + var opts loadOptions + for _, option := range options { + option(&opts) + } + + flags := defineFlags(version, opts) err := flags.Parse(args) if err != nil { diff --git a/internal/config/flags.go b/internal/config/flags.go index a0c78dd3..81f2710f 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -1,13 +1,36 @@ package config import ( - "github.com/evg4b/uncors/internal/tui" "github.com/spf13/pflag" ) -func defineFlags(version string) *pflag.FlagSet { +// UsageRenderer draws the --help output for a flag set. Rendering it is a +// presentation decision, so the caller supplies one rather than this package +// reaching for the console. +type UsageRenderer func(flags *pflag.FlagSet, version string) + +// Option configures how the configuration is loaded. +type Option func(*loadOptions) + +type loadOptions struct { + usage UsageRenderer +} + +// WithUsage sets the renderer used for --help. Without it, pflag's own default +// usage output is used. +func WithUsage(usage UsageRenderer) Option { + return func(o *loadOptions) { + o.usage = usage + } +} + +func defineFlags(version string, opts loadOptions) *pflag.FlagSet { flags := pflag.NewFlagSet("uncors", pflag.ContinueOnError) - flags.Usage = func() { tui.PrintUsage(flags, version) } + + if opts.usage != nil { + flags.Usage = func() { opts.usage(flags, version) } + } + flags.StringSliceP("to", "t", []string{}, "Target host with protocol for the resource to be proxied") flags.StringSliceP("from", "f", []string{}, "Local host with protocol for the resource from which proxying will take place") //nolint: lll flags.String("proxy", "", "HTTP/HTTPS proxy for requests to the real server (uses system proxy by default)") diff --git a/internal/di/container.go b/internal/di/container.go index efc1ac96..ab3d37da 100644 --- a/internal/di/container.go +++ b/internal/di/container.go @@ -6,7 +6,6 @@ import ( "slices" "sync" - "github.com/evg4b/uncors/internal/commands" "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/helpers" "github.com/evg4b/uncors/internal/server" @@ -19,12 +18,11 @@ type Container struct { args []string version string - cliOutput factory[contracts.Output] - requestTracker factory[*server.RequestTracker] - generateCertsCommand factory[*commands.GenerateCertsCommand] - hostCertManager factory[*server.HostCertManager] - server factory[*server.Server] - proxy factory[*Proxy] + cliOutput factory[contracts.Output] + requestTracker factory[*server.RequestTracker] + hostCertManager factory[*server.HostCertManager] + server factory[*server.Server] + proxy factory[*Proxy] closersMu sync.Mutex closers []io.Closer @@ -64,16 +62,15 @@ func NewContainer(options ...ContainerOption) *Container { closers: []io.Closer{}, } - container = helpers.ApplyOptions(container, options) - + // Factories first, options second: an option may replace a factory, and + // ApplyOptions mutates in place, so the defaults must already be there. container.cliOutput = newFactory(container.newCliOutput) container.requestTracker = newFactory(container.newRequestTracker) - container.generateCertsCommand = newFactory(container.newGenerateCertsCommand) container.hostCertManager = newFactory(container.newHostCertManager) container.server = newFactory(container.newServer) container.proxy = newFactory(container.newProxy) - return container + return helpers.ApplyOptions(container, options) } // Close releases every process-lifetime resource the container built, in diff --git a/internal/di/factories.go b/internal/di/factories.go index b601daa1..1a7a9002 100644 --- a/internal/di/factories.go +++ b/internal/di/factories.go @@ -1,19 +1,10 @@ package di import ( - "github.com/evg4b/uncors/internal/commands" "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/server" - "github.com/evg4b/uncors/internal/tui" ) -func (c *Container) newGenerateCertsCommand() *commands.GenerateCertsCommand { - return commands.NewGenerateCertsCommand( - commands.WithOutput(c.CliOutput()), - commands.WithFs(c.fs), - ) -} - func (c *Container) newHostCertManager() *server.HostCertManager { return server.NewHostCertManager(c.fs) } @@ -23,7 +14,7 @@ func (c *Container) Server() *server.Server { } func (c *Container) newCliOutput() contracts.Output { - return tui.NewCliOutput(c.stdout) + return &noopOutput{} } func (c *Container) Proxy() *Proxy { diff --git a/internal/di/factory.go b/internal/di/factory.go index c6ad1e08..8e66a907 100644 --- a/internal/di/factory.go +++ b/internal/di/factory.go @@ -5,6 +5,7 @@ import "sync" type factory[T any] struct { once sync.Once + built bool cache T factory func() T } @@ -12,11 +13,19 @@ type factory[T any] struct { func (f *factory[T]) GetOrBuild() T { f.once.Do(func() { f.cache = f.factory() + f.built = true }) return f.cache } +// Built reports whether the value has been created. Replacing a factory after +// that point cannot take effect, so callers use this to fail loudly instead of +// silently keeping the old value. +func (f *factory[T]) Built() bool { + return f.built +} + func newFactory[T any](factoryFunc func() T) factory[T] { return factory[T]{factory: factoryFunc} } diff --git a/internal/di/noop_output.go b/internal/di/noop_output.go new file mode 100644 index 00000000..860401cf --- /dev/null +++ b/internal/di/noop_output.go @@ -0,0 +1,29 @@ +package di + +import "github.com/evg4b/uncors/internal/contracts" + +// noopOutput is the container's default console output. +// +// The real one is a presentation decision and is supplied by the composition +// root: main installs the console renderer, and interactive mode installs the +// sink that feeds the TUI. Defaulting to a null object is what lets the +// container - and therefore the whole service - stay free of the terminal +// libraries. +type noopOutput struct{} + +func (*noopOutput) Write(p []byte) (int, error) { return len(p), nil } + +func (*noopOutput) Info(any) {} +func (*noopOutput) Infof(string, ...any) {} +func (*noopOutput) InfoBox(...string) {} +func (*noopOutput) Error(any) {} +func (*noopOutput) Errorf(string, ...any) {} +func (*noopOutput) ErrorBox(...string) {} +func (*noopOutput) Warn(any) {} +func (*noopOutput) Warnf(string, ...any) {} +func (*noopOutput) WarnBox(...string) {} +func (*noopOutput) Print(any) {} +func (*noopOutput) Printf(string, ...any) {} +func (*noopOutput) Request(*contracts.RequestData) {} + +func (n *noopOutput) NewPrefixOutput(string) contracts.Output { return n } diff --git a/internal/di/override.go b/internal/di/override.go index 36d0cd02..9cee19e3 100644 --- a/internal/di/override.go +++ b/internal/di/override.go @@ -1,13 +1,24 @@ package di -import "github.com/evg4b/uncors/internal/contracts" +import ( + "github.com/evg4b/uncors/internal/contracts" +) func (c *Container) Override(action ContainerOption) { action(c) } +// WithCliOutput replaces the console output. It must be applied before +// anything resolves CliOutput: the container caches singletons on first use, so +// a late override would silently keep the old value and send the interactive +// mode's output to the terminal underneath the TUI. That failure is invisible +// at runtime, so it panics instead. func WithCliOutput(factory func() contracts.Output) ContainerOption { return func(c *Container) { + if c.cliOutput.Built() { + panic("di: CliOutput was overridden after it had already been built") + } + c.cliOutput = newFactory(factory) } } diff --git a/internal/di/public_api.go b/internal/di/public_api.go index 35416dfb..96808822 100644 --- a/internal/di/public_api.go +++ b/internal/di/public_api.go @@ -4,7 +4,6 @@ import ( "io" "time" - "github.com/evg4b/uncors/internal/commands" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/handler/mock" @@ -15,7 +14,6 @@ import ( "github.com/evg4b/uncors/internal/handler/static" "github.com/evg4b/uncors/internal/infra" "github.com/evg4b/uncors/internal/server" - "github.com/evg4b/uncors/internal/tui/styles" "github.com/evg4b/uncors/internal/urlreplacer" "github.com/evg4b/uncors/internal/version" "github.com/spf13/afero" @@ -45,10 +43,6 @@ func (c *Container) RequestTracker() *server.RequestTracker { return c.requestTracker.GetOrBuild() } -func (c *Container) GenerateCertsCommand() *commands.GenerateCertsCommand { - return c.generateCertsCommand.GetOrBuild() -} - func (c *Container) HostCertManager() *server.HostCertManager { return c.hostCertManager.GetOrBuild() } @@ -59,7 +53,7 @@ func (c *Container) OptionsMiddleware(cfg config.OptionsHandling) contracts.Midd options.WithHeaders(cfg.Headers), options.WithCode(cfg.Code), ), - styles.OptionsStyle.Render("OPTIONS"), + "OPTIONS", ) } @@ -70,7 +64,7 @@ func (c *Container) StaticMiddleware(path string, dir config.StaticDirectory) co static.WithIndex(dir.Index), static.WithPrefix(path), ), - styles.StaticStyle.Render("STATIC"), + "STATIC", ) } @@ -83,7 +77,7 @@ func (c *Container) VersionChecker(proxy string) *version.Checker { } func (c *Container) MockHandler(response *config.Response) contracts.Handler { - prefix := styles.MockStyle.Render("MOCK") + prefix := "MOCK" return infra.WithPrefix(prefix, mock.NewMockHandler( mock.WithResponse(response), @@ -93,7 +87,7 @@ func (c *Container) MockHandler(response *config.Response) contracts.Handler { } func (c *Container) ScriptHandler(scriptConfig *config.Script) contracts.Handler { - prefix := styles.RewriteStyle.Render("SCRIPT") + prefix := "SCRIPT" output := c.CliOutput() return infra.WithPrefix(prefix, script.NewHandler( @@ -106,12 +100,12 @@ func (c *Container) ScriptHandler(scriptConfig *config.Script) contracts.Handler func (c *Container) RewriteMiddleware(rewriting *config.RewritingOption) contracts.Middleware { return infra.NewPrefixedMiddleware( rewrite.NewMiddleware(rewrite.WithRewritingOptions(rewriting)), - styles.RewriteStyle.Render("REWRITE"), + "REWRITE", ) } func (c *Container) ProxyHandler(mappings config.Mappings, proxyURL string) contracts.Handler { - prefix := styles.ProxyStyle.Render("PROXY") + prefix := "PROXY" output := c.CliOutput() return infra.WithPrefix(prefix, proxy.NewProxyHandler( diff --git a/internal/di/public_api_test.go b/internal/di/public_api_test.go index dd04b2da..b7a79d2b 100644 --- a/internal/di/public_api_test.go +++ b/internal/di/public_api_test.go @@ -4,13 +4,13 @@ import ( "bytes" "testing" - "github.com/evg4b/uncors/internal/commands" "github.com/evg4b/uncors/internal/config" "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/server" "github.com/evg4b/uncors/internal/version" "github.com/evg4b/uncors/testing/hosts" + "github.com/evg4b/uncors/testing/mocks" "github.com/evg4b/uncors/testing/testutils" "github.com/spf13/afero" "github.com/stretchr/testify/assert" @@ -75,13 +75,6 @@ func TestContainer(t *testing.T) { assert.IsType(t, &server.RequestTracker{}, tracker) }) - t.Run("generate certs command", func(t *testing.T) { - cmd := container.GenerateCertsCommand() - - assert.NotNil(t, cmd) - assert.IsType(t, &commands.GenerateCertsCommand{}, cmd) - }) - t.Run("host cert manager", func(t *testing.T) { manager := container.HostCertManager() @@ -176,45 +169,33 @@ func TestContainer(t *testing.T) { } func TestContainerOverride(t *testing.T) { - t.Run("Override replaces cli output factory", func(t *testing.T) { + t.Run("replaces the cli output when applied before first use", func(t *testing.T) { container := di.NewContainer() defer testutils.Close(t, container) - customOutput := container.CliOutput() - - overrideApplied := false + sentinel := mocks.NewOutputMock(t) container.Override(di.WithCliOutput(func() contracts.Output { - overrideApplied = true - - return customOutput - })) - - newContainer := di.NewContainer() - defer testutils.Close(t, newContainer) - - newContainer.Override(di.WithCliOutput(func() contracts.Output { - return customOutput + return sentinel })) - result := newContainer.CliOutput() - assert.Same(t, customOutput, result) - - _ = overrideApplied + assert.Same(t, sentinel, container.CliOutput()) }) - t.Run("OverrideCliOutput sets custom factory", func(t *testing.T) { + // The container caches singletons on first use, so a late override cannot + // take effect. In interactive mode that would silently leave output going to + // the terminal underneath the TUI, so it must fail loudly. + t.Run("panics when applied after the output has been built", func(t *testing.T) { container := di.NewContainer() defer testutils.Close(t, container) - sentinel := container.CliOutput() - - container.Override(di.WithCliOutput(func() contracts.Output { - return sentinel - })) + _ = container.CliOutput() - result := container.CliOutput() - assert.Same(t, sentinel, result) + assert.PanicsWithValue(t, "di: CliOutput was overridden after it had already been built", func() { + container.Override(di.WithCliOutput(func() contracts.Output { + return mocks.NewOutputMock(t) + })) + }) }) } diff --git a/internal/di/runtime.go b/internal/di/runtime.go index 0fb0f578..91cfb7ba 100644 --- a/internal/di/runtime.go +++ b/internal/di/runtime.go @@ -14,7 +14,6 @@ import ( "github.com/evg4b/uncors/internal/handler/router" "github.com/evg4b/uncors/internal/infra" "github.com/evg4b/uncors/internal/server" - "github.com/evg4b/uncors/internal/tui/styles" ) const baseAddress = "127.0.0.1" @@ -95,7 +94,7 @@ func (r *Runtime) CacheMiddleware(globs config.CacheGlobs) contracts.Middleware cache.WithCacheStorage(r.Cache()), cache.WithGlobs(globs), ), - styles.CacheStyle.Render("CACHE"), + "CACHE", ) } diff --git a/internal/tui/messages.go b/internal/tui/messages.go index 73d16496..e56f43c0 100644 --- a/internal/tui/messages.go +++ b/internal/tui/messages.go @@ -3,8 +3,3 @@ package tui const DisclaimerMessage = `DON'T USE IT FOR PRODUCTION! This is a reverse proxy for use in testing or debugging web applications locally. It hasn't been reviewed for security issues.` - -const NewVersionIsAvailable = `NEW VERSION IS AVAILABLE! -%s is not the latest version, you should upgrade to %s. -See more information at https://github.com/evg4b/uncors/releases -` diff --git a/internal/tui/messages_test.go b/internal/tui/messages_test.go index 8314e1f6..e45abeff 100644 --- a/internal/tui/messages_test.go +++ b/internal/tui/messages_test.go @@ -13,9 +13,3 @@ func TestDisclaimerMessage(t *testing.T) { assert.Contains(t, tui.DisclaimerMessage, "reverse proxy") assert.Contains(t, tui.DisclaimerMessage, "security") } - -func TestNewVersionIsAvailable(t *testing.T) { - assert.NotEmpty(t, tui.NewVersionIsAvailable) - assert.Contains(t, tui.NewVersionIsAvailable, "NEW VERSION IS AVAILABLE") - assert.Contains(t, tui.NewVersionIsAvailable, "%s") -} diff --git a/internal/tui/output.go b/internal/tui/output.go index 3e0c34e4..a38a148f 100644 --- a/internal/tui/output.go +++ b/internal/tui/output.go @@ -167,8 +167,10 @@ func (output *CliOutput) renderMessage(msg string) { fmt.Fprint(&output.buffer, msg) } +// renderPrefix styles the prefix here rather than expecting a pre-styled one, +// so that the layers producing prefixes deal only in plain handler names. func (output *CliOutput) renderPrefix() { if len(output.prefix) > 0 { - output.buffer.WriteString(output.prefix) + output.buffer.WriteString(styles.Feature(output.prefix)) } } diff --git a/internal/tui/styles/features.go b/internal/tui/styles/features.go index 26319e34..7bafe532 100644 --- a/internal/tui/styles/features.go +++ b/internal/tui/styles/features.go @@ -1,5 +1,7 @@ package styles +import "charm.land/lipgloss/v2" + var ( ProxyStyle = blockStyle.Background(proxyColor) MockStyle = blockStyle.Background(mockColor) @@ -8,3 +10,26 @@ var ( RewriteStyle = blockStyle.Background(rewriteColor) OptionsStyle = blockStyle.Background(optionsColor) ) + +// featureStyles maps a handler's plain name to its badge style. The names come +// from the service, which must not know how a badge looks; the mapping is the +// only place that decision is made. +var featureStyles = map[string]lipgloss.Style{ + "PROXY": ProxyStyle, + "MOCK": MockStyle, + "STATIC": StaticStyle, + "CACHE": CacheStyle, + "REWRITE": RewriteStyle, + "OPTIONS": OptionsStyle, + "SCRIPT": RewriteStyle, +} + +// Feature renders a handler badge. An unrecognised name is returned unchanged, +// so arbitrary prefixes still work. +func Feature(name string) string { + if style, ok := featureStyles[name]; ok { + return style.Render(name) + } + + return name +} diff --git a/internal/uncors_app/app.go b/internal/uncors_app/app.go index 94bbf0cd..6193e00e 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -15,6 +15,7 @@ import ( "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/helpers" "github.com/evg4b/uncors/internal/render" + "github.com/evg4b/uncors/internal/tui" ) const ( @@ -31,7 +32,7 @@ type UncorsApp struct { // and renders what comes back. service service - output *tuiOutput + output contracts.Output renderer *render.Renderer outputCh chan string @@ -81,7 +82,7 @@ func NewUncorsApp( loadConfig app.Loader, ) *UncorsApp { outputCh := make(chan string, outputChannelSize) - output := newTuiOutput(outputCh) + output := tui.NewCliOutput(newChannelWriter(outputCh)) // The sink has to be installed before anything resolves CliOutput, because // the container caches it on first use. diff --git a/internal/uncors_app/output.go b/internal/uncors_app/output.go index d61ca4dc..8fcf3881 100644 --- a/internal/uncors_app/output.go +++ b/internal/uncors_app/output.go @@ -1,103 +1,34 @@ package uncorsapp import ( - "bytes" "strings" - - "github.com/evg4b/uncors/internal/contracts" - "github.com/evg4b/uncors/internal/tui" ) -type tuiOutput struct { - ch chan<- string - prefix string -} - -func newTuiOutput(ch chan<- string) *tuiOutput { - return &tuiOutput{ch: ch} -} - -func (o *tuiOutput) Write(p []byte) (int, error) { - o.send(string(p)) - - return len(p), nil -} - -func (o *tuiOutput) Info(msg any) { - o.capture(func(out *tui.CliOutput) { out.Info(msg) }) -} - -func (o *tuiOutput) Infof(msg string, args ...any) { - o.capture(func(out *tui.CliOutput) { out.Infof(msg, args...) }) -} - -func (o *tuiOutput) InfoBox(messages ...string) { - o.captureBox(func(out *tui.CliOutput) { out.InfoBox(messages...) }) -} - -func (o *tuiOutput) Error(msg any) { - o.capture(func(out *tui.CliOutput) { out.Error(msg) }) -} - -func (o *tuiOutput) Errorf(msg string, args ...any) { - o.capture(func(out *tui.CliOutput) { out.Errorf(msg, args...) }) -} - -func (o *tuiOutput) ErrorBox(messages ...string) { - o.captureBox(func(out *tui.CliOutput) { out.ErrorBox(messages...) }) +// channelWriter turns rendered console output into history lines. +// +// CliOutput writes one complete line per call, so this is simply where the +// TUI's console output goes instead of a terminal. Nothing renders here: the +// service hands the model structured events, internal/render decides what they +// say, and CliOutput decides how they look. +type channelWriter struct { + ch chan<- string } -func (o *tuiOutput) Warn(msg any) { - o.capture(func(out *tui.CliOutput) { out.Warn(msg) }) +func newChannelWriter(ch chan<- string) *channelWriter { + return &channelWriter{ch: ch} } -func (o *tuiOutput) Warnf(msg string, args ...any) { - o.capture(func(out *tui.CliOutput) { out.Warnf(msg, args...) }) -} - -func (o *tuiOutput) WarnBox(messages ...string) { - o.captureBox(func(out *tui.CliOutput) { out.WarnBox(messages...) }) -} - -func (o *tuiOutput) Print(msg any) { - o.capture(func(out *tui.CliOutput) { out.Print(msg) }) -} - -func (o *tuiOutput) Printf(msg string, args ...any) { - o.capture(func(out *tui.CliOutput) { out.Printf(msg, args...) }) -} - -func (o *tuiOutput) Request(data *contracts.RequestData) { - o.capture(func(out *tui.CliOutput) { out.Request(data) }) -} - -func (o *tuiOutput) NewPrefixOutput(prefix string) contracts.Output { - return &tuiOutput{ - ch: o.ch, - prefix: prefix, - } -} - -func (o *tuiOutput) send(msg string) { - msg = strings.TrimRight(msg, "\n") +// Write never blocks. A model that cannot keep up drops lines rather than +// stalling whatever produced them, which is the same policy the request +// tracker applies to activity. +func (w *channelWriter) Write(line []byte) (int, error) { + msg := strings.TrimRight(string(line), "\n") if len(msg) > 0 { select { - case o.ch <- msg: + case w.ch <- msg: default: } } -} - -func (o *tuiOutput) capture(fn func(out *tui.CliOutput)) { - var buf bytes.Buffer - - tmp := tui.NewCliOutput(&buf, tui.WithPrefix(o.prefix)) - fn(tmp) - o.send(buf.String()) -} -func (o *tuiOutput) captureBox(fn func(out *tui.CliOutput)) { - var buf bytes.Buffer - fn(tui.NewCliOutput(&buf)) - o.send(buf.String()) + return len(line), nil } diff --git a/internal/uncors_app/output_internal_test.go b/internal/uncors_app/output_internal_test.go index bfe04236..dbd20429 100644 --- a/internal/uncors_app/output_internal_test.go +++ b/internal/uncors_app/output_internal_test.go @@ -1,173 +1,42 @@ package uncorsapp import ( - "net/url" "testing" - "time" - "github.com/evg4b/uncors/internal/contracts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func recv(t *testing.T, ch <-chan string) string { - t.Helper() +func TestChannelWriter(t *testing.T) { + t.Run("forwards a rendered line without its trailing newline", func(t *testing.T) { + lines := make(chan string, 1) - select { - case msg := <-ch: - return msg - case <-time.After(time.Second): - t.Fatal("timed out waiting for channel message") + count, err := newChannelWriter(lines).Write([]byte("a line\n")) - return "" - } -} - -func newTestOutput() (*tuiOutput, <-chan string) { - outputCh := make(chan string, 10) - - return newTuiOutput(outputCh), outputCh -} - -func TestTuiOutput_Info(t *testing.T) { - t.Run("Info sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.Info("hello info") - assert.Contains(t, recv(t, ch), "hello info") - }) - - t.Run("Infof formats and sends message", func(t *testing.T) { - out, ch := newTestOutput() - out.Infof("value is %d", 42) - assert.Contains(t, recv(t, ch), "42") - }) - - t.Run("InfoBox sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.InfoBox("box line one", "box line two") - - msg := recv(t, ch) - assert.Contains(t, msg, "box line one") - assert.Contains(t, msg, "box line two") - }) -} - -func TestTuiOutput_Error(t *testing.T) { - t.Run("Error sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.Error("something failed") - assert.Contains(t, recv(t, ch), "something failed") - }) - - t.Run("Errorf formats and sends message", func(t *testing.T) { - out, ch := newTestOutput() - out.Errorf("error code %d", 500) - assert.Contains(t, recv(t, ch), "500") - }) - - t.Run("ErrorBox sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.ErrorBox("err a", "err b") - - msg := recv(t, ch) - assert.Contains(t, msg, "err a") - assert.Contains(t, msg, "err b") - }) -} - -func TestTuiOutput_Warn(t *testing.T) { - t.Run("Warn sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.Warn("watch out") - assert.Contains(t, recv(t, ch), "watch out") - }) - - t.Run("Warnf formats and sends message", func(t *testing.T) { - out, ch := newTestOutput() - out.Warnf("threshold %d%%", 90) - assert.Contains(t, recv(t, ch), "90") - }) - - t.Run("WarnBox sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.WarnBox("warn x", "warn y") - - msg := recv(t, ch) - assert.Contains(t, msg, "warn x") - assert.Contains(t, msg, "warn y") - }) -} - -func TestTuiOutput_Print(t *testing.T) { - t.Run("Print sends message to channel", func(t *testing.T) { - out, ch := newTestOutput() - out.Print("plain text") - assert.Contains(t, recv(t, ch), "plain text") + require.NoError(t, err) + assert.Equal(t, len("a line\n"), count, "the writer must report every byte consumed") + assert.Equal(t, "a line", <-lines) }) - t.Run("Printf formats and sends message", func(t *testing.T) { - out, ch := newTestOutput() - out.Printf("count=%d", 7) - assert.Contains(t, recv(t, ch), "7") - }) -} + t.Run("drops empty writes", func(t *testing.T) { + lines := make(chan string, 1) -func TestTuiOutput_Write(t *testing.T) { - t.Run("Write sends bytes as string to channel", func(t *testing.T) { - out, ch := newTestOutput() - n, err := out.Write([]byte("raw bytes")) - require.NoError(t, err) - assert.Equal(t, 9, n) - assert.Contains(t, recv(t, ch), "raw bytes") - }) + _, err := newChannelWriter(lines).Write([]byte("\n")) - t.Run("Write returns len(p) on success", func(t *testing.T) { - out, _ := newTestOutput() - data := []byte("test data 123") - n, err := out.Write(data) require.NoError(t, err) - assert.Equal(t, len(data), n) + assert.Empty(t, lines, "a bare newline carries nothing to show") }) - t.Run("Write with only whitespace does not send", func(t *testing.T) { - out, ch := newTestOutput() - _, err := out.Write([]byte("\n\n")) - require.NoError(t, err) + // Presentation must never stall whatever produced the line. + t.Run("never blocks when the model cannot keep up", func(t *testing.T) { + lines := make(chan string, 1) + writer := newChannelWriter(lines) - select { - case msg := <-ch: - t.Fatalf("expected no message, got %q", msg) - case <-time.After(50 * time.Millisecond): + for range 100 { + _, err := writer.Write([]byte("overflow\n")) + require.NoError(t, err) } - }) -} - -func TestTuiOutput_Request(t *testing.T) { - t.Run("Request sends formatted request data", func(t *testing.T) { - out, outputCh := newTestOutput() - u, _ := url.Parse("http://example.com/api/resource") - out.Request(&contracts.RequestData{ - Method: "GET", - URL: u, - Code: 200, - }) - - msg := recv(t, outputCh) - assert.NotEmpty(t, msg) - }) -} - -func TestTuiOutput_NewPrefixOutput(t *testing.T) { - t.Run("returns output that shares the same channel", func(t *testing.T) { - out, outputCh := newTestOutput() - prefixed := out.NewPrefixOutput("[SVC]") - prefixed.Info("service message") - assert.NotEmpty(t, recv(t, outputCh)) - }) - - t.Run("NewPrefixOutput implements contracts.Output", func(_ *testing.T) { - out, _ := newTestOutput() - _ = out.NewPrefixOutput("prefix") + assert.Len(t, lines, 1) }) } diff --git a/internal/version/check_new_version.go b/internal/version/check_new_version.go index 7c6221f7..fbbc10a9 100644 --- a/internal/version/check_new_version.go +++ b/internal/version/check_new_version.go @@ -7,7 +7,6 @@ import ( "net/http" "github.com/evg4b/uncors/internal/helpers" - "github.com/evg4b/uncors/internal/tui" "github.com/hashicorp/go-version" ) @@ -62,7 +61,7 @@ func (checker *Checker) CheckNewVersion(ctx context.Context) { } if lastVersion.GreaterThan(checker.currentVersion) { - checker.output.Infof(tui.NewVersionIsAvailable, checker.currentVersion.String(), lastVersion.String()) + checker.output.Infof(NewVersionIsAvailable, checker.currentVersion.String(), lastVersion.String()) checker.output.Info("") } else { log.Print("Version is up to date") diff --git a/internal/version/messages.go b/internal/version/messages.go new file mode 100644 index 00000000..ad56fa15 --- /dev/null +++ b/internal/version/messages.go @@ -0,0 +1,9 @@ +package version + +// NewVersionIsAvailable is the notice shown when a newer release exists. It +// lives here rather than with the terminal helpers so that checking for a new +// version does not drag the rendering libraries into the service. +const NewVersionIsAvailable = `NEW VERSION IS AVAILABLE! +%s is not the latest version, you should upgrade to %s. +See more information at https://github.com/evg4b/uncors/releases +` diff --git a/internal/version/new_version_check_test.go b/internal/version/new_version_check_test.go index ce4b4c38..2358d756 100644 --- a/internal/version/new_version_check_test.go +++ b/internal/version/new_version_check_test.go @@ -136,3 +136,9 @@ func TestCheckNewVersion(t *testing.T) { testutils.MatchSnapshot(t, string(outputData)) }) } + +func TestNewVersionIsAvailableMessage(t *testing.T) { + assert.NotEmpty(t, version.NewVersionIsAvailable) + assert.Contains(t, version.NewVersionIsAvailable, "NEW VERSION IS AVAILABLE") + assert.Contains(t, version.NewVersionIsAvailable, "%s") +} diff --git a/main.go b/main.go index 3e363c00..42bebed6 100644 --- a/main.go +++ b/main.go @@ -5,6 +5,7 @@ import ( "os" "github.com/evg4b/uncors/internal/cli" + "github.com/evg4b/uncors/internal/contracts" "github.com/evg4b/uncors/internal/di" "github.com/evg4b/uncors/internal/helpers" "github.com/evg4b/uncors/internal/infra" @@ -22,6 +23,11 @@ func main() { di.WithStdout(os.Stdout), di.WithVersion(Version), di.WithArgs(os.Args[1:]), + // The composition root decides how the application talks to the user. + // The container itself knows nothing about terminals. + di.WithCliOutput(func() contracts.Output { + return tui.NewCliOutput(os.Stdout) + }), ) // A panic anywhere below is a bug, but the user still deserves a readable diff --git a/tests/architecture/boundary_test.go b/tests/architecture/boundary_test.go index 35eb602c..8f289a94 100644 --- a/tests/architecture/boundary_test.go +++ b/tests/architecture/boundary_test.go @@ -24,14 +24,14 @@ var serviceSide = []string{ "github.com/evg4b/uncors/internal/server/...", "github.com/evg4b/uncors/internal/handler/...", "github.com/evg4b/uncors/internal/config/...", + "github.com/evg4b/uncors/internal/version/...", } -// forbidden are the terminal-interaction libraries. Lip Gloss is deliberately -// absent for now: internal/di still styles handler prefixes through -// internal/tui/styles, which Phase 5 of the migration removes. +// forbidden is every terminal library. The service must not reach for any of +// them: not the event loop, not the widgets, and not the styling. Rendering +// belongs to internal/render and internal/tui, which the service never imports. var forbidden = []string{ - "charm.land/bubbletea", - "charm.land/bubbles", + "charm.land/", } func dependenciesOf(t *testing.T, pattern string) []string { @@ -62,9 +62,9 @@ func readGuardedSources(t *testing.T) { require.NoError(t, err) } -// T10: the service must not depend on the TUI toolkit, directly or through -// anything it imports. -func TestServiceDoesNotDependOnBubbleTea(t *testing.T) { +// T10: the service must not depend on any terminal library, directly or +// through anything it imports. +func TestServiceDoesNotDependOnTerminalLibraries(t *testing.T) { readGuardedSources(t) for _, pattern := range serviceSide { @@ -74,7 +74,7 @@ func TestServiceDoesNotDependOnBubbleTea(t *testing.T) { for _, dep := range deps { for _, banned := range forbidden { assert.NotContains(t, dep, banned, - "%s must run without a TUI, but depends on %s", pattern, dep) + "%s must run without a terminal, but depends on %s", pattern, dep) } } }) @@ -89,8 +89,14 @@ func TestServiceDoesNotDependOnTheTUIPackage(t *testing.T) { for _, pattern := range serviceSide { t.Run(pattern, func(t *testing.T) { for _, dep := range dependenciesOf(t, pattern) { - assert.NotEqual(t, "github.com/evg4b/uncors/internal/uncors_app", dep, - "%s must not depend on the TUI", pattern) + for _, presentation := range []string{ + "github.com/evg4b/uncors/internal/uncors_app", + "github.com/evg4b/uncors/internal/tui", + "github.com/evg4b/uncors/internal/render", + } { + assert.NotEqual(t, presentation, dep, + "%s must not depend on the presentation layer", pattern) + } } }) } From db20914ba2890251e29f5afacd94292697337fc2 Mon Sep 17 00:00:00 2001 From: Evgeny Abramovich Date: Thu, 3 Sep 2026 18:14:46 -0400 Subject: [PATCH 7/7] fix: close the remaining resource-ownership gaps (Phase 6) Three resources outlived what created them. - Server.Restart shut the old listeners down before it knew the new ones could bind. Proxy.Restart carefully builds the new generation first, but that discipline was defeated one layer down: a port that could not be rebound left the server bound to nothing, and in headless mode that also ended the process, because every listener goroutine had exited and Wait returned. Restart now restores the previous targets when the new ones fail, so a rejected configuration costs a short interruption rather than the whole proxy. ErrRollbackFailed reports the case where the old ones cannot be restored either. Verified the test fails without the rollback. - The upstream HTTP client is created per generation but was owned by nobody, so the idle connection pool of every superseded configuration survived until the process exited. It is now built by the Runtime and released with it. - The per-host certificate cache is driven by traffic rather than by configuration - a {placeholder} mapping serves whatever host is asked for, and each entry is an RSA-2048 key pair. It is now bounded at 128 entries with oldest-first eviction. Tests: a full service start/reload/shutdown cycle repeated 12 times asserts no goroutine growth, which covers the config watcher, the request pump and the listeners rather than the runtime alone. Co-Authored-By: Claude Opus 5 --- internal/app/service_test.go | 51 ++++++++++++++++ internal/di/public_api.go | 8 ++- internal/di/public_api_test.go | 2 +- internal/di/runtime.go | 27 ++++++++- internal/server/errors.go | 5 ++ internal/server/host_cert_manager.go | 24 +++++++- .../server/host_cert_manager_internal_test.go | 18 ++++++ internal/server/server.go | 34 ++++++++++- internal/server/server_test.go | 59 +++++++++++++++++++ 9 files changed, 222 insertions(+), 6 deletions(-) diff --git a/internal/app/service_test.go b/internal/app/service_test.go index db29e679..14feb7c0 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -7,6 +7,7 @@ import ( "net/url" "os" "path/filepath" + "runtime" "strconv" "sync" "testing" @@ -307,3 +308,53 @@ func TestInFlightIsOrderedAndDrains(t *testing.T) { require.Eventually(t, func() bool { return len(service.InFlight()) == 2 }, time.Second, 5*time.Millisecond, "a completed request must leave the in-flight set") } + +// T4: the generation model is only worth having if releasing a generation +// actually releases it. This exercises the whole service lifecycle rather than +// the runtime alone, so it also covers the config watcher, the request pump +// and the listener goroutines. +func TestRepeatedServiceCyclesDoNotLeakGoroutines(t *testing.T) { + const cycles = 12 + + settle := func() { + for range 3 { + runtime.GC() + time.Sleep(20 * time.Millisecond) + } + } + + cycle := func(t *testing.T) { + t.Helper() + + port := testutils.GetFreePort(t) + cfg := configFor(port) + + container := di.NewContainer() + service := app.New(container, cfg, "", func() (*config.UncorsConfig, error) { return cfg, nil }) + + require.NoError(t, service.Start(t.Context())) + + service.Reload() + + require.NoError(t, service.Shutdown(t.Context())) + service.Wait() + require.NoError(t, service.Close()) + require.NoError(t, container.Close()) + } + + // One warm-up cycle first: the first run allocates pools and lazily built + // singletons that legitimately persist. + cycle(t) + settle() + + baseline := runtime.NumGoroutine() + + for range cycles { + cycle(t) + } + + settle() + + assert.LessOrEqual(t, runtime.NumGoroutine(), baseline+cycles/2, + "a full start/reload/shutdown cycle must not leak goroutines") +} diff --git a/internal/di/public_api.go b/internal/di/public_api.go index 96808822..40f22b3d 100644 --- a/internal/di/public_api.go +++ b/internal/di/public_api.go @@ -104,13 +104,17 @@ func (c *Container) RewriteMiddleware(rewriting *config.RewritingOption) contrac ) } -func (c *Container) ProxyHandler(mappings config.Mappings, proxyURL string) contracts.Handler { +// ProxyHandler builds the fallthrough handler for a set of mappings. The HTTP +// client is passed in rather than created here, because its connection pool has +// a configuration lifetime and must be released with the generation that owns +// it. +func (c *Container) ProxyHandler(mappings config.Mappings, client contracts.HTTPClient) contracts.Handler { prefix := "PROXY" output := c.CliOutput() return infra.WithPrefix(prefix, proxy.NewProxyHandler( proxy.WithURLReplacerFactory(urlreplacer.NewURLReplacerFactory(mappings)), - proxy.WithHTTPClient(infra.MakeHTTPClient(proxyURL)), + proxy.WithHTTPClient(client), proxy.WithOutput(output.NewPrefixOutput(prefix)), )) } diff --git a/internal/di/public_api_test.go b/internal/di/public_api_test.go index b7a79d2b..58d929be 100644 --- a/internal/di/public_api_test.go +++ b/internal/di/public_api_test.go @@ -149,7 +149,7 @@ func TestContainer(t *testing.T) { mappings := config.Mappings{ {From: hosts.Localhost.HTTP(), To: hosts.Localhost.HTTPS()}, } - handler := container.ProxyHandler(mappings, "") + handler := container.ProxyHandler(mappings, mocks.NewHTTPClientMock(t)) assert.NotNil(t, handler) assert.Implements(t, (*contracts.Handler)(nil), handler) diff --git a/internal/di/runtime.go b/internal/di/runtime.go index 91cfb7ba..91d49f6c 100644 --- a/internal/di/runtime.go +++ b/internal/di/runtime.go @@ -4,6 +4,7 @@ import ( "errors" "io" "net" + "net/http" "slices" "strconv" @@ -154,13 +155,37 @@ func (r *Runtime) router(mappings config.Mappings, proxyURL string) (contracts.H muxRouter, err := router.NewRouter( mappings, router.WithDiContainer(r), - router.ForRouterWithDefaultHandler(r.container.ProxyHandler(mappings, proxyURL)), + router.ForRouterWithDefaultHandler(r.container.ProxyHandler(mappings, r.httpClient(proxyURL))), router.ForRouterWithCacheMiddlewareFactory(r.CacheMiddleware), ) return infra.CastToContractsHandler(muxRouter), err } +// httpClient builds the upstream client for this generation and binds its +// connection pool to the generation's lifetime. Without this the idle +// connections of every superseded configuration survive until the process +// exits. +func (r *Runtime) httpClient(proxyURL string) *http.Client { + client := infra.MakeHTTPClient(proxyURL) + + register(r, transportCloser{client: client}) + + return client +} + +// transportCloser releases a client's idle connections. http.Client has no +// Close, so this adapts the one thing that actually needs releasing. +type transportCloser struct { + client *http.Client +} + +func (t transportCloser) Close() error { + t.client.CloseIdleConnections() + + return nil +} + // register binds a resource to the generation's lifetime. func register[T io.Closer](runtime *Runtime, resource T) T { runtime.closers = append(runtime.closers, resource) diff --git a/internal/server/errors.go b/internal/server/errors.go index 0e7d515e..bc0eaa6c 100644 --- a/internal/server/errors.go +++ b/internal/server/errors.go @@ -20,4 +20,9 @@ var ( // ErrCACertExpiringSoon is returned when the CA certificate is close to expiring. ErrCACertExpiringSoon = errors.New("consider regenerating with: uncors generate-certs --force") + + // ErrRollbackFailed is returned when a restart could not bind the new + // targets and could not put the previous ones back either. The server is + // serving nothing at that point, which the caller has to act on. + ErrRollbackFailed = errors.New("failed to restore the previous listeners after a failed restart") ) diff --git a/internal/server/host_cert_manager.go b/internal/server/host_cert_manager.go index 5791dc38..1dd34843 100644 --- a/internal/server/host_cert_manager.go +++ b/internal/server/host_cert_manager.go @@ -10,12 +10,20 @@ import ( "github.com/spf13/afero" ) +// maxCachedCertificates bounds the per-host certificate cache. A mapping with a +// {placeholder} host serves any name that matches it, so the set of hosts is +// driven by whatever is requested rather than by the configuration, and each +// entry is an RSA-2048 key pair. The limit is far above what a development +// session needs while keeping the memory bounded. +const maxCachedCertificates = 128 + // HostCertManager manages TLS certificates for HTTPS mappings, generating a // certificate per host on the fly signed by the local development CA. type HostCertManager struct { fs afero.Fs generator *CertGenerator cache map[string]*tls.Certificate + order []string mutex sync.RWMutex } @@ -104,12 +112,26 @@ func (m *HostCertManager) certificateForHost(host string) (*tls.Certificate, err return nil, fmt.Errorf("failed to generate certificate for %s: %w", host, err) } - m.cache[host] = cert + m.store(host, cert) log.Printf("Generated TLS certificate for host: %s", host) return cert, nil } +// store caches the certificate, evicting the oldest entry once the cache is +// full. Callers hold the write lock. +func (m *HostCertManager) store(host string, cert *tls.Certificate) { + if len(m.cache) >= maxCachedCertificates { + oldest := m.order[0] + m.order = m.order[1:] + + delete(m.cache, oldest) + } + + m.cache[host] = cert + m.order = append(m.order, host) +} + func extractServerHost(clientHello *tls.ClientHelloInfo) (string, bool) { if clientHello == nil { return "", false diff --git a/internal/server/host_cert_manager_internal_test.go b/internal/server/host_cert_manager_internal_test.go index 20c2f70e..5899cef0 100644 --- a/internal/server/host_cert_manager_internal_test.go +++ b/internal/server/host_cert_manager_internal_test.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "net" "path/filepath" + "strconv" "testing" "time" @@ -286,3 +287,20 @@ func TestHostCertManager_Concurrent(t *testing.T) { } }) } + +// A {placeholder} mapping serves whatever host is requested, so the cache is +// driven by traffic rather than by configuration and has to be bounded. +func TestHostCertManagerCacheIsBounded(t *testing.T) { + manager := NewHostCertManager(afero.NewMemMapFs()) + + for i := range maxCachedCertificates + 20 { + manager.store("host-"+strconv.Itoa(i)+".local", &tls.Certificate{}) + } + + assert.Len(t, manager.cache, maxCachedCertificates, "the cache must not grow without limit") + assert.Len(t, manager.order, maxCachedCertificates, "eviction order must stay in step with the cache") + + assert.NotContains(t, manager.cache, "host-0.local", "the oldest entry must be evicted first") + assert.Contains(t, manager.cache, "host-"+strconv.Itoa(maxCachedCertificates+19)+".local", + "the newest entry must be kept") +} diff --git a/internal/server/server.go b/internal/server/server.go index b9aa3072..cfacc63e 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -150,16 +150,37 @@ func (s *Server) Shutdown(ctx context.Context) error { return errors.Join(errs...) } +// Restart replaces the running listeners with a new set. +// +// The new generation usually wants the same ports, so the old listeners have to +// be released before the new ones can bind, and a failure at that point would +// otherwise leave the server bound to nothing. When the new targets cannot be +// started the previous ones are put back, so a rejected configuration costs a +// short interruption rather than the whole proxy. func (s *Server) Restart(ctx context.Context, targets []Target) error { + // Holding the wait group up across the whole restart stops Wait from + // returning during the gap when nothing is listening. s.Add(1) defer s.Done() + previous := s.currentTargets() + err := s.Shutdown(ctx) if err != nil { return err } - return s.Start(ctx, targets) + err = s.Start(ctx, targets) + if err == nil { + return nil + } + + rollbackErr := errors.Join(s.Shutdown(ctx), s.Start(ctx, previous)) + if rollbackErr != nil { + return errors.Join(err, ErrRollbackFailed, rollbackErr) + } + + return err } func (s *Server) Wait() { @@ -183,6 +204,17 @@ func (s *Server) Close() error { return errors.Join(errs...) } +// currentTargets returns the targets currently installed, so a failed restart +// can restore them. +func (s *Server) currentTargets() []Target { + s.mu.RLock() + defer s.mu.RUnlock() + + return lo.Map(s.listeners, func(listener *PortListener, _ int) Target { + return *listener.target + }) +} + func (s *Server) handleRequest(handler contracts.Handler, writer http.ResponseWriter, request *http.Request) { helpers.NormaliseRequest(request) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 1076edff..ab277a7b 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "path/filepath" + "strconv" "sync" "testing" "time" @@ -23,6 +24,30 @@ import ( "github.com/stretchr/testify/require" ) +func addr(port int) string { + return net.JoinHostPort("127.0.0.1", strconv.Itoa(port)) +} + +func okHandler() contracts.Handler { + return infra.HandlerFunc(func(w contracts.ResponseWriter, _ *contracts.Request) error { + w.WriteHeader(http.StatusOK) + + return nil + }) +} + +// blockPort holds port for the duration of the test so the server cannot bind it. +func blockPort(t *testing.T, port int) { + t.Helper() + + listenConfig := &net.ListenConfig{} + + listener, err := listenConfig.Listen(t.Context(), "tcp4", addr(port)) + require.NoError(t, err) + + t.Cleanup(func() { _ = listener.Close() }) +} + func TestServer(t *testing.T) { const porstCount = 5 @@ -354,3 +379,37 @@ func TestServer(t *testing.T) { require.Error(t, err) }) } + +// P4: a restart that cannot bind the new targets must put the previous ones +// back. Before this, Shutdown ran first and a failed Start left the server +// bound to nothing - which in headless mode also ended the process, because +// every listener goroutine had exited and Wait returned. +func TestServerRestartRollsBackWhenTheNewPortIsTaken(t *testing.T) { + ports := testutils.GetFreePorts(t, 2) + original, contested := ports[0], ports[1] + + instance := server.New(nil, nil) + + require.NoError(t, instance.Start(t.Context(), []server.Target{ + {Address: addr(original), Handler: okHandler()}, + })) + + defer testutils.Close(t, instance) + + blockPort(t, contested) + + err := instance.Restart(t.Context(), []server.Target{ + {Address: addr(contested), Handler: okHandler()}, + }) + + require.Error(t, err, "restart must report that the new target could not be bound") + require.NotErrorIs(t, err, server.ErrRollbackFailed, "the previous listeners should have been restored") + + assert.Eventually(t, func() bool { return !testutils.IsPortFree(original) }, 2*time.Second, 10*time.Millisecond, + "the previous generation must still be serving after a failed restart") + + response, getErr := http.Get("http://" + addr(original)) //nolint:noctx // liveness probe + require.NoError(t, getErr) + require.NoError(t, response.Body.Close()) + assert.Equal(t, http.StatusOK, response.StatusCode) +}