refactor: separate the application service from the Bubble Tea TUI - #131
Open
evg4b wants to merge 7 commits into
Open
refactor: separate the application service from the Bubble Tea TUI#131evg4b wants to merge 7 commits into
evg4b wants to merge 7 commits into
Conversation
…des (Phase 0) Pre-migration correctness pass. These are the bugs the service/TUI boundary would otherwise cement in, most of them caused by interactive and headless mode implementing the same behaviour twice. - run_interactive.go discarded the error from LoadConfiguration, so a config that failed to parse or validate handed a nil *UncorsConfig to proxy.Restart, which BuildRuntime dereferenced at runtime.go:44. PanicInterceptor re-panics in non-release builds, so a YAML typo killed the TUI. Headless already handled this correctly. The loader now returns an error and both reload paths report it and keep the running generation. - handleServerError quit without shutting the proxy down, stranding the generation a failed start left behind; it now goes through shutdownCmd. - The TUI's shutdown grace period was 5s against 15s in cli and server. - Container.closers was declared and iterated but never appended to, so container.Close() was a no-op. Server and RequestTracker now register themselves; Close releases in reverse creation order and is idempotent, so the server stops before the sink it emits into. - Watcher.Watch used a check-then-set guard that Close never released, making a Watcher permanently "watching". It now claims atomically and Close releases the claim. Making the watcher reusable exposed a data race on the fsnotify handle between a previous run goroutine and a new Watch, so run now owns the watcher it was handed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ner (Phase 1) Interactive and non-interactive mode each implemented start, config watching, reload, the version check and shutdown separately. They had already drifted (see the previous commit), so this gives that orchestration one owner that both modes drive. internal/app.Service owns the active configuration, the reload lifecycle, the config.Watcher, the version check, signal handling and the service lifetime context. It has no dependency on a terminal. - runNonInteractive shrinks to draining the request tracker and calling service.Run; awaitShutdown, watchConfig and versionCheck moved into the service verbatim in behaviour. - UncorsApp drops proxy, container, cfg, loadConfig, configPath, watcher and its private root context. handleServerStarted is now empty: the service starts watching and version-checking itself. The model sends Start, Reload and Shutdown, and renders what comes back. - Both modes now share one config loader, so a reload cannot behave differently depending on the mode. - Reloads are serialised and coalesced. The decision to stop looping and the clearing of the running flag happen under the lock that sets the pending flag, so a request arriving mid-reload is never dropped. Tests: internal/app/service_test.go covers the service running with no client attached, reload keeping the generation when the config fails, reload moving to a new port, config-file watching, idempotent shutdown, and concurrent reloads never overlapping. The watcher tests move out of the TUI package, which no longer owns a watcher. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Service code no longer decides what the console looks like. di.Proxy printed the logo, the disclaimer, the mappings box and the restart messages from inside the generation-transition code, which meant the dependency-injection layer knew what a terminal was. - internal/app now emits LifecycleEvent and LogEvent. The set is small and matches what the application already communicated: starting, started, start-failed, reloading, reloaded, reload-failed, stopping, stopped. - Lifecycle is recorded as Status as well as notified. Notifications are dropped under pressure like any other event, which is only safe because the latest state stays readable - a client that misses a notification can still read the truth. - Log events are dropped and counted, mirroring RequestTracker. Presentation must never be able to stall the service. - internal/render is the single place that turns an event into console output. Both modes use it, so neither can drift from the other in what it reports. - di.Proxy prints nothing at all now. Verified the headless output is byte-identical to the previous commit by running both binaries through start, reload, a rejected config and SIGTERM, stripping ANSI and normalising ports. That comparison caught one real regression: Reloading was announced before the config had loaded, so a rejected config claimed the server was restarting. It is now emitted only once the config is known to be good. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e 3) Phase 1 already removed the model's di.Proxy field, so the remaining work was to make the boundary something the compiler and CI can hold. - UncorsApp now depends on a narrow service interface declared at the point of use - Start, Reload, Shutdown, Close, Context, Events - rather than on *app.Service. Commands go down, events come back up, and the model reaches for nothing else. - tests/architecture asserts that internal/app, di, server, handler and config do not depend on Bubble Tea or Bubbles, directly or transitively, and do not depend on the TUI package. Verified the guard fails when the import is actually added. - The guard shells out to go list, which Go's test cache cannot see, so it also reads the sources it guards. Without that a violating import could be masked by a cached pass; verified invalidation works from a primed cache. Lip Gloss is deliberately not yet in the forbidden set: internal/di still styles handler prefixes through internal/tui/styles. Phase 5 removes that and adds it. No command queue: the plan's command set is two entries and process separation is out of scope, so a queue would be machinery without a consumer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The set of in-flight requests was a fact about the server that only a Bubble Tea widget knew, rebuilt purely from the events that widget happened to witness. That is what caused the stale-rows bug: TrackerWidget cleared itself on the restart key but not on a reload triggered by saving the config file, so the UI kept showing requests from a generation that no longer existed. - The service is now the single consumer of the request tracker. It maintains the authoritative in-flight set, exposes it through InFlight(), and republishes activity on its own event stream. - A completed reload clears that set, so both reload paths behave identically. The widgets learn about it from StateReloaded rather than from a message the restart key synthesised, which is what removes the asymmetry. - The TUI no longer reads the tracker, and request rendering moved into internal/render, so headless drops the separate RequestPrinter goroutine and renders requests the same way it renders everything else. - History scrollback is capped at 10,000 lines. It grew without limit before, one line per request, for as long as the process lived. The existing test asserting unbounded growth is updated to assert the cap. Verified headless output is byte-identical to the previous commit across start, two requests, reload, a request on the new port, a rejected config and SIGTERM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The service styled its own output. di rendered handler badges with Lip Gloss, config imported the console package to draw --help, and the version checker imported it for one message constant. Between them they pulled Lip Gloss into every service package, so "the service does not depend on the TUI" was not actually true. - di now emits plain handler names - PROXY, MOCK, CACHE and so on - and internal/tui styles them when it renders a prefix. The badge lookup is the only place that decision is made. - config takes a usage renderer as an option instead of reaching for the console; the CLI supplies it, which is the only place --help is reachable. - The new-version notice moved to internal/version, where it is emitted. - The container defaults to a null output and the composition root installs the real one, so di no longer imports internal/tui at all. Building the generate-certs command moved to the CLI for the same reason. - The TUI's output adapter rendered every message into a throwaway CliOutput and pushed the resulting string. It is now just an io.Writer over a channel: the service hands the model structured events, internal/render decides what they say, and CliOutput decides how they look. - Overriding CliOutput after it has been built now panics. It was a silent no-op that would have sent output to the terminal underneath the TUI. Two tests were asserting that footgun and now assert the contract instead. internal/app, di, server, handler, config and version are now free of charm.land entirely, and tests/architecture enforces it. Verified byte-identical console output against the previous commit with ANSI included, covering the proxy and mock badges, reload and a rejected config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three resources outlived what created them.
- Server.Restart shut the old listeners down before it knew the new ones could
bind. Proxy.Restart carefully builds the new generation first, but that
discipline was defeated one layer down: a port that could not be rebound left
the server bound to nothing, and in headless mode that also ended the
process, because every listener goroutine had exited and Wait returned.
Restart now restores the previous targets when the new ones fail, so a
rejected configuration costs a short interruption rather than the whole
proxy. ErrRollbackFailed reports the case where the old ones cannot be
restored either. Verified the test fails without the rollback.
- The upstream HTTP client is created per generation but was owned by nobody,
so the idle connection pool of every superseded configuration survived until
the process exited. It is now built by the Runtime and released with it.
- The per-host certificate cache is driven by traffic rather than by
configuration - a {placeholder} mapping serves whatever host is asked for,
and each entry is an RSA-2048 key pair. It is now bounded at 128 entries with
oldest-first eviction.
Tests: a full service start/reload/shutdown cycle repeated 12 times asserts no
goroutine growth, which covers the config watcher, the request pump and the
listeners rather than the runtime alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Separates the application service from the Bubble Tea layer, in seven independently reviewable phases. Each commit builds, passes
make checkand the integration suite, and leaves the application working.Why
Interactive and non-interactive mode implemented the same orchestration twice — headless with four bare goroutines in
run_non_interactive.go, interactive astea.Cmds plus aconfig.Watcherthe Bubble Tea model owned itself.They had already drifted, and the drift was a user-visible bug: headless handled a bad config reload correctly, interactive discarded the error and dereferenced
nil. A YAML typo killed the TUI. That is the concrete cost of having no single owner of application behaviour.Much of the groundwork already existed —
di.Runtimescopes resources to a config generation,di.Proxyowns generation transitions,RequestTrackeris a non-blocking event stream. The missing piece was an owner for orchestration.What changed
internal/app.Servicenow owns the active configuration, the reload lifecycle, the config watcher, the version check, signal handling and the service lifetime context. Both modes drive it; they differ only in the client attached.internal/renderis the single place that turns a service event into console output, so the two modes cannot drift again in what they report.The headline result:
internal/app,di,server,handler,configandversionnow depend on zerocharm.landpackages, transitively.tests/architecturefails the build if that regresses.8d1a04ee4c48feinternal/app.Service— one orchestration owner9a50b28Lifecycle/Logevents +internal/renderebd7205bccfd2f0856dd8db20914Bugs fixed
Each has a test that was verified to fail without its fix.
run_interactive.godiscarded the error fromLoadConfiguration, so a rejected config handedniltoproxy.Restart, whichBuildRuntimedereferenced atruntime.go:44.PanicInterceptorre-panics in non-release builds. Headless already handled this correctly.TrackerWidgetcleared itself on the restart key but not on a config save. The in-flight set is now service state, cleared on reload, so both paths behave identically.Server.Restartshut the old listeners down before knowing the new ones could bind — and in headless mode that also ended the process, because every listener goroutine had exited andWaitreturned. It now restores the previous targets.Container.closerswas declared and iterated but never appended to, socontainer.Close()was a no-op andRequestTrackerwas never closed.{placeholder}mapping serves any name, each entry an RSA-2048 key pair. Now bounded at 128 with oldest-first eviction.Watcher.Watchused a check-then-set guardClosenever released. Making it reusable exposed a data race on the fsnotify handle between a previousrungoroutine and a newWatch;runnow owns the watcher it is handed.For the reviewer
Output was verified byte-identical at every phase that touched it. For each, I ran the real binary and the previous commit's binary through start → request → reload → rejected config → SIGTERM and diffed. Phase 5 was compared with ANSI included — the
PROXY/MOCKbadge bytes are unchanged.That comparison caught a regression I had introduced:
Reloadingwas announced before the config loaded, so a rejected config falsely claimed the server was restarting. It is now emitted only once the config is known good.Interactive mode was driven end-to-end in a pty: request served,
rreload reaching both widgets,qclean exit, port released.Three deliberate deviations from the plan:
Reload,Shutdown) and process separation is out of scope, so a queue would be machinery with no consumer. The TUI depends on a narrow consumer-side interface instead.signal.Notifywould double-fire. The service provides it, interactive does not opt in.WithCliOutputwas hardened, not deleted. Deleting it requires replacingcontracts.Outputacross the handler layer. It now panics if applied after the output has been built, instead of silently sending output to the terminal underneath the TUI. Two tests were asserting that footgun and now assert the contract.Behaviour changes worth a look:
callCount <= 3for 5 writes.config.LoadConfigurationtakes an optionalWithUsagerenderer. Existing call sites are unaffected (variadic); only the CLI passes it, so--helpoutput is unchanged.Container.GenerateCertsCommandremoved — the CLI constructs it, which is what letsdidrop itsinternal/tuiimport.Testing
make checkandmake test-integrationpass. New coverage: service running with no client attached, reload keeping the generation when config fails, reload port migration, concurrent reloads never overlapping, in-flight cleared on reload, restart rollback, full start/reload/shutdown cycles asserting no goroutine growth, and the architecture guard itself (verified it fails on a real violation, and is cache-correct).🤖 Generated with Claude Code