diff --git a/internal/app/events.go b/internal/app/events.go new file mode 100644 index 00000000..7e209f6a --- /dev/null +++ b/internal/app/events.go @@ -0,0 +1,166 @@ +package app + +import ( + "sync" + "sync/atomic" + + "github.com/evg4b/uncors/internal/config" + "github.com/evg4b/uncors/internal/server" +) + +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() {} + +// 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 { + 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 new file mode 100644 index 00000000..972e22aa --- /dev/null +++ b/internal/app/service.go @@ -0,0 +1,376 @@ +// 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 ( + "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 ( + 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 + + 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 +} + +// 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()) + + service := &Service{ + container: container, + proxy: container.Proxy(), + configPath: configPath, + load: load, + ctx: ctx, + 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 +// 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 { + 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 { + 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() + + 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) + + s.events.EmitLifecycle(LifecycleEvent{State: StateStopped, Err: err}) + }) + + 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() + s.events.Close() + + if s.watcher != nil { + return s.watcher.Close() + } + + 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 { + 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 { + s.events.EmitLifecycle(LifecycleEvent{State: StateReloadFailed, Err: err}) + + return + } + + s.mu.Lock() + 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}) +} + +// 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.events.EmitLog(LevelError, fmt.Sprintf("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) + + interrupted := false + + select { + case sig := <-stop: + 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() + + _ = s.Shutdown(shutdownCtx) +} diff --git a/internal/app/service_test.go b/internal/app/service_test.go new file mode 100644 index 00000000..14feb7c0 --- /dev/null +++ b/internal/app/service_test.go @@ -0,0 +1,360 @@ +package app_test + +import ( + "errors" + "net" + "net/http" + "net/url" + "os" + "path/filepath" + "runtime" + "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/internal/server" + "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") +} + +// 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") +} + +// 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/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_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/cli/run_non_interactive.go b/internal/cli/run_non_interactive.go index e2fd722c..304e15d8 100644 --- a/internal/cli/run_non_interactive.go +++ b/internal/cli/run_non_interactive.go @@ -2,24 +2,18 @@ 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/contracts" "github.com/evg4b/uncors/internal/di" - "github.com/evg4b/uncors/internal/server" -) - -const ( - shutdownTimeout = 15 * time.Second - versionCheckDelay = 50 * time.Millisecond + "github.com/evg4b/uncors/internal/render" ) // 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 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,94 +21,34 @@ 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) + // 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() }() + + // 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 } - 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) - } + reportDropped(output, tracker.Dropped(), "activity lines") + reportDropped(output, service.DroppedEvents(), "service events") output.Info("Server was stopped") 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 +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) } - - <-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..c8232d63 100644 --- a/internal/cli/run_uncors.go +++ b/internal/cli/run_uncors.go @@ -5,8 +5,10 @@ import ( "errors" "fmt" + "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" ) @@ -14,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): @@ -34,3 +43,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/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/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..ab3d37da 100644 --- a/internal/di/container.go +++ b/internal/di/container.go @@ -3,8 +3,9 @@ package di import ( "errors" "io" + "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" @@ -17,14 +18,14 @@ 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] - closers []io.Closer + closersMu sync.Mutex + closers []io.Closer } type ContainerOption = func(c *Container) @@ -61,22 +62,29 @@ 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(server.NewRequestTracker) - container.generateCertsCommand = newFactory(container.newGenerateCertsCommand) + container.requestTracker = newFactory(container.newRequestTracker) 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 +// 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() + + errs := make([]error, 0, len(closers)) - for _, closer := range c.closers { + for _, closer := range slices.Backward(closers) { err := closer.Close() if err != nil { errs = append(errs, err) @@ -85,3 +93,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..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 { @@ -31,5 +22,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/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/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/di/public_api.go b/internal/di/public_api.go index 35416dfb..40f22b3d 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,17 +100,21 @@ 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") +// 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 dd04b2da..58d929be 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() @@ -156,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) @@ -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..91d49f6c 100644 --- a/internal/di/runtime.go +++ b/internal/di/runtime.go @@ -4,6 +4,7 @@ import ( "errors" "io" "net" + "net/http" "slices" "strconv" @@ -14,7 +15,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 +95,7 @@ func (r *Runtime) CacheMiddleware(globs config.CacheGlobs) contracts.Middleware cache.WithCacheStorage(r.Cache()), cache.WithGlobs(globs), ), - styles.CacheStyle.Render("CACHE"), + "CACHE", ) } @@ -155,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/render/render.go b/internal/render/render.go new file mode 100644 index 00000000..9de19126 --- /dev/null +++ b/internal/render/render.go @@ -0,0 +1,106 @@ +// 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) + 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: + 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/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) +} 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 907eb65d..6193e00e 100644 --- a/internal/uncors_app/app.go +++ b/internal/uncors_app/app.go @@ -9,17 +9,18 @@ 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" "github.com/evg4b/uncors/internal/helpers" - "github.com/evg4b/uncors/internal/server" + "github.com/evg4b/uncors/internal/render" + "github.com/evg4b/uncors/internal/tui" ) const ( outputChannelSize = 1000 - shutdownTimeout = 5 * time.Second - versionCheckDelay = 50 * time.Millisecond + shutdownTimeout = 15 * time.Second memTickInterval = 2 * time.Second bytesPerMegabyte = 1024 * 1024 ) @@ -27,21 +28,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 service - outputCh chan string - appContext func() context.Context - appDone <-chan struct{} - cancel context.CancelFunc + output contracts.Output + renderer *render.Renderer - cfg *config.UncorsConfig - loadConfig func() *config.UncorsConfig - configPath string - - watcher *config.Watcher + outputCh chan string + done <-chan struct{} termHeight int termWidth int @@ -52,6 +47,20 @@ 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 ( serverStartedMsg struct{} serverErrMsg struct{ err error } @@ -63,42 +72,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, + loadConfig app.Loader, ) *UncorsApp { outputCh := make(chan string, outputChannelSize) - output := newTuiOutput(outputCh) - - appCtx, cancel := context.WithCancel(context.Background()) + output := tui.NewCliOutput(newChannelWriter(outputCh)) + // 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, + renderer: render.New(output, container.Version()), 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(), @@ -111,7 +114,7 @@ func (m *UncorsApp) Init() tea.Cmd { return tea.Batch( m.startServerCmd(), m.waitOutputCmd(), - m.watchEventsCmd(), + m.waitServiceEventCmd(), m.memWidget.Init(), m.trackerWidget.Init(), m.historyWidget.Init(), @@ -136,10 +139,17 @@ func (m *UncorsApp) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case outputLineMsg: cmds = append(cmds, m.waitOutputCmd()) - case requestEventMsg: - m.handleRequestEvent(typedMsg) + case serviceEventMsg: + m.renderer.Render(typedMsg.event) - cmds = append(cmds, m.watchEventsCmd()) + 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()) @@ -266,48 +276,36 @@ 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) + log.Println("Server started") - err := watcher.Watch(m.appContext(), func() { - defer helpers.PanicInterceptor(func(value any) { - m.output.Errorf("Config reloading error: %v", value) - }) - - newCfg := m.loadConfig() - - 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 { - 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) { - 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() { @@ -318,10 +316,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 @@ -329,7 +323,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} } @@ -347,22 +341,24 @@ func (m *UncorsApp) waitOutputCmd() tea.Cmd { } return outputLineMsg(line) - case <-m.appDone: + case <-m.done: return nil } } } -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.tracker.Events(): + case event, ok := <-m.service.Events(): if !ok { return nil } - return requestEventMsg(event) - case <-m.appDone: + return serviceEventMsg{event: event} + case <-m.done: return nil } } @@ -370,12 +366,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{} } @@ -387,23 +382,9 @@ func (m *UncorsApp) restartCmd() tea.Cmd { m.output.Errorf("Restart error: %v", value) }) - newCfg := m.loadConfig() - - err := m.proxy.Restart(m.appContext(), newCfg) - if err != nil { - m.output.Errorf("Failed to restart: %v", err) - } - - return restartMsg{} - } -} - -func (m *UncorsApp) versionCheckCmd() tea.Cmd { - return func() tea.Msg { - time.Sleep(versionCheckDelay) - - m.container.VersionChecker(m.cfg.Proxy). - CheckNewVersion(m.appContext()) + // The reload's effects arrive as service events, which is what the + // widgets act on; nothing to report from here. + m.service.Reload() return nil } diff --git a/internal/uncors_app/app_internal_test.go b/internal/uncors_app/app_internal_test.go index 013c0807..bcf3c34e 100644 --- a/internal/uncors_app/app_internal_test.go +++ b/internal/uncors_app/app_internal_test.go @@ -3,16 +3,18 @@ package uncorsapp import ( "errors" "net/url" - "os" + "regexp" "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" "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" @@ -20,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{}, } @@ -38,22 +57,21 @@ 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 }, ) - return app, &loadCalls + return app, &loadCalls, container } 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() @@ -66,10 +84,10 @@ 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.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) @@ -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{}) @@ -163,11 +180,11 @@ 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) + assert.Nil(t, app.restartCmd()()) assert.Equal(t, 1, *loadCalls) msg = app.shutdownCmd()() @@ -182,7 +199,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()()) }) @@ -194,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) + + // The service is the single consumer of the tracker; the model sees + // activity only because the service republishes it. + msg := model.waitServiceEventCmd()() + + event, ok := msg.(serviceEventMsg) + require.True(t, ok) - assert.Equal( - t, - requestEventMsg(server.RequestEvent{ID: 9, Method: "GET", URL: requestURL}), - app.watchEventsCmd()(), - ) + request, ok := event.event.(app.RequestEvent) + require.True(t, ok) + assert.Equal(t, emitted, request.Event) - app.cancel() - assert.Nil(t, app.watchEventsCmd()()) + 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()()) }) } @@ -255,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) @@ -291,9 +316,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())) }) } @@ -301,175 +325,84 @@ 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 { return cfg }) - - 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) - - app := NewUncorsApp(container, "/nonexistent/path/config.yaml", cfg, func() *config.UncorsConfig { return cfg }) - - 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) { +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") + }) + } } -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) +// 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) - app := NewUncorsApp(container, tmpFile.Name(), cfg, func() *config.UncorsConfig { - select { - case called <- struct{}{}: - default: - } + loadCalls := 0 + model := NewUncorsApp(container, "", cfg, func() (*config.UncorsConfig, error) { + loadCalls++ - return cfg + return nil, errBoom }) - 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) - } + defer cleanupTestApp(t, model) - if app.historyWidget != nil && app.historyWidget.hist != nil { - err := app.historyWidget.hist.Close() - require.NoError(t, err) - } - }() + require.NoError(t, model.service.Start(model.service.Context())) - 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 + require.NotPanics(t, func() { + assert.Nil(t, model.restartCmd()()) + }) - cmd := app.handleShutdown() - require.NotNil(t, cmd) - assert.Equal(t, tea.Quit(), cmd()) + assert.Equal(t, 1, loadCalls) + assert.False(t, testutils.IsPortFree(port), "the previous generation must still be bound") - app.cancel() - err = app.proxy.Close() - require.NoError(t, err) + // 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) - if app.historyWidget != nil && app.historyWidget.hist != nil { - err := app.historyWidget.hist.Close() - require.NoError(t, err) - } + status := service.Status() + assert.Equal(t, app.StateReloadFailed, status.State) + require.ErrorIs(t, status.Err, errBoom) } 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()) +} 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/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 + +--- diff --git a/tests/architecture/boundary_test.go b/tests/architecture/boundary_test.go new file mode 100644 index 00000000..8f289a94 --- /dev/null +++ b/tests/architecture/boundary_test.go @@ -0,0 +1,103 @@ +// 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/...", + "github.com/evg4b/uncors/internal/version/...", +} + +// 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/", +} + +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 any terminal library, directly or +// through anything it imports. +func TestServiceDoesNotDependOnTerminalLibraries(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 terminal, 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) { + 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) + } + } + }) + } +}