refactor: deepen module seams across server, agent, and web#160
Merged
Conversation
Both the background scheduler and the manual HTTP trigger previously assembled the check transition themselves, with three behavioral gaps: record and state were written in two separate statements, slow checks for the same monitor could overlap, and the manual path skipped the maintenance gate and notifications entirely (a manual success after failures cleared last_status, permanently losing the recovery notification). Move the complete transition behind MonitorCheckRunner in service::monitor_check: per-monitor in-flight guard, checker dispatch, transactional record+state commit, maintenance gating, and failure/recovery notifications. The scheduler and the HTTP handler now only decide when a check happens; a concurrent manual trigger returns 409 Conflict instead of interleaving writes.
…et auth
The browser, terminal, and docker-logs WebSocket handlers each carried a
private copy of the credential ladder (cookie -> API key -> bearer) plus
header extractors, duplicating precedence, forced-password, session-source,
and mobile-expiry rules that middleware::auth already claimed to own. Any
future change to one rule could silently drift between the four copies.
Deepen middleware::auth with resolve_connection, returning identity plus
lifetime facts (AuthenticatedConnection { user, mobile_expires }), and a
resolve_ws_connection view that rejects must_change_password users because
onboarding cannot complete over a WebSocket. All three WS adapters and the
HTTP middleware now resolve through the same seam while keeping their
protocol-specific responses; api/auth.rs change_password reuses the shared
raw-token extractors.
Two edge cases now follow one policy instead of per-copy accidents: a
flagged user's valid cookie no longer falls through to a weaker credential
on WS upgrades, and a non-web session enforces its fixed expiry whichever
header carried it.
…thority Effective capabilities were a per-connection Arc<AtomicU32> handed to every subsystem, with a per-connection grant supervisor rewriting it. Consumers knew the cell, the bit timing, and who updates when — and two lifecycle gaps followed from that shape: a temporary grant's expiry never reconciled running work (live terminal sessions survived revocation), and the SecurityManager was started once at boot from the base bitmask, so a temporary security-events grant could never start it (nor could a persisted grant enable it across a restart) and expiry never stopped it. Introduce a process-wide CapabilityAuthority (capability_grants::authority) that owns the base bitmask, folds in grants, and drives every transition. It exposes has()/effective()/active_grants(), a state watch for long-running subsystems, and a transition broadcast that connections forward to the server as CapabilitiesChanged. All consumers (terminal, pinger, network prober, docker, file manager, IP-quality checker, reporter dispatch) now gate through the authority instead of sharing the atomic. Lifecycle fixes riding on the seam: - the reporter's transition arm reconciles in-flight work before announcing: losing CAP_TERMINAL tears down live PTY sessions with a TerminalError - SecurityManager::spawn_supervised starts/stops the pipeline as CAP_SECURITY_EVENTS becomes effective or expires, at any point in the agent's lifetime The old per-connection grant supervisor is deleted; its pure evaluate() logic and tests move into the authority. An e2e test covers the full expiry path: grant folded into connect-time SystemInfo, live session torn down on expiry, CapabilitiesChanged announced without the bit.
Add Unreleased changelog entries for the shared WebSocket credential policy, the unified service-monitor check transition, and temporary grants bounding running work. Document the new expiry semantics (terminal teardown, live security-pipeline start/stop) on the capabilities page in both languages.
Scheduled-task validation, persistence, in-memory jobs, execution, and startup restoration were split across service, task, and router modules. The service and task halves called each other, routers compensated database and scheduler mutations by hand, and cleanup paths could bypass the scheduler entirely. Deepen service::task_scheduler into the single lifecycle owner. Its interface now covers typed create/update inputs, lifecycle mutations, manual execution, startup restoration, and the two-phase server-cleanup plan; the router keeps only HTTP mapping and definition-level audit logs. Remove the old task module and the redundant cron dependency. Lifecycle fixes riding on the seam: - validate and calculate next runs with the scheduler parser, including correct weekday semantics - serialize lifecycle changes and fence queued callbacks by their registered job UUID - retain active-run ownership through cancellation, wait for completion before deletion, and reject stale cleanup - keep database rows, job registrations, orphan cleanup, and late task results synchronized across failures and commits
Own the live, REST-list, and detail cache topology behind a single catalog interface. Route WebSocket frames, REST reads, and server mutations through typed projection events so transport adapters no longer coordinate query keys themselves.\n\nPreserve runtime metrics across REST snapshots, treat full sync and incremental frames according to their protocol semantics, guard refresh ordering and cache lifetime, and keep list/detail membership consistent after removals. Add catalog-level regression coverage and migrate all readers to the private namespace.
Connect and mutation paths each carried their own fetch/map/send logic for ping tasks, network probes, IP-quality services, and the firewall blocklist, so capability filtering and protocol gating were duplicated and could drift. AgentDesiredStateReconciler now owns the full fetch -> map -> send lifecycle per domain, with per-provider locks so a newer snapshot can never be overwritten by an older concurrent one. - SystemInfo handshake reconciles every domain from live capabilities instead of hand-patching only the firewall - capabilities_changed drives a targeted per-domain reconcile; revoking ping/firewall caps now clears the corresponding agent state - ping/network/IP-quality/firewall CRUD routes signal the reconciler instead of hand-rolling their own sync sends - firewall reset ack only clears target state on ok=true, keeping the desired target when the agent reports a failed reset - PingService keeps DB/CRUD semantics only; AgentManager sync moves out
handle_server_message threaded 13 borrows through every call and the test harness had to mirror that exact parameter list, so adding any manager meant touching the dispatcher signature, the select! loop, and the harness in lockstep. ConnectionRuntime now owns every manager, outbound sender, and piece of Docker lifecycle state a server command can touch; the dispatcher is a method on the state it drives. - reporter/runtime.rs owns the managers, capability authority, upgrade config, firewall manager, and Docker probe/retry/stats/demote cycle - the two Docker select! arms merge into one docker_tick() future, so the loop no longer juggles three loose Option/bool/Interval locals - the triple-duplicated connection-exit cleanup collapses into runtime.shutdown() - the dispatch test Harness builds a real runtime and calls the two-argument method instead of mirroring a 13-parameter signature wire and file_ops keep their existing seams; the dispatcher still delegates file replies to file_ops unchanged.
Tab catalogs, search validation, range catalogs and resolution, the legacy hours-to-key conversion, and the show_network/show_server_detail toggle combos were each restated at their call sites, so the admin detail route, the public detail route, the shared detail content, the network overview, and both public network routes all had to agree by copy. lib/server-detail-nav.ts now owns the whole policy: - SERVER_DETAIL_TABS / PUBLIC_SERVER_DETAIL_TABS plus the two validateSearch parsers move out of the route files - ADMIN/PUBLIC_TIME_RANGES, rangesForVariant, and resolveRange move out of server-detail-content - the legacy /network/$serverId redirect uses the shared legacyNetworkRangeToRangeKey conversion - publicNetworkHome(config) names the ADR-0001 decision (tab / standalone fallback / hidden) once; both public network routes and the overview deep-links consume the verdict instead of re-deriving it from raw toggles Behavior-preserving: the standalone public network page still renders only for show_server_detail=false + show_network=true deployments.
Review follow-up to the desired-state reconciler: ping/network/IP-quality routes each carried an identical warn-and-swallow wrapper, server.rs inlined the same shape, and the capability-change handler re-implemented reconcile_connection's four-domain loop by hand — a drift risk when a fifth domain lands. The reconciler now owns the demoted-to-warning variants (reconcile_connected_or_warn / reconcile_agent_or_warn) and the capability-change path calls reconcile_connection like the connect path does.
Add Unreleased entries for the three user-visible behavior changes that rode in with the desired-state reconciler: capability changes resync everything the agent executes, network probe target edits reach connected agents, and a failed firewall blocklist reset keeps its pending state instead of being recorded as done.
The desired-state reconciler now pushes IpQualitySync on every fresh SystemInfo, but four mock-agent suites still panicked on any frame outside their hand-listed first-connect noise set, failing whenever the sync raced ahead of the docker/file command under test. Add ip_quality_sync to the noise filters, matching the suites that already tolerated it.
Extend the 'What the server sees' list on the capabilities page (both languages): a capability change now re-syncs everything the agent executes for the server — ping tasks re-filtered, probe targets and IP-quality services re-pushed, firewall blocklist reset and only re-sent while firewall_block stays effective.
Map each domain to its provider lock once via ProviderLocks::for_domain instead of repeating the lock acquisition in both dispatch matches, share the IpQualitySync payload load between the single-agent and fan-out paths, and collapse the hand-rolled first-error folds to get_or_insert/map_or. Document that the capability-change handler's full-domain reconcile is deliberate, and drop leftover doubled blank lines in the mutation routers.
Only terminal_manager, unlock_checker and cmd_result_tx are touched by the reporter select! loop; the remaining fields are runtime-internal.
Ten integration suites carried their own copy of the ignorable first-connect push list, and each new sync frame (latest: ip_quality_sync) meant patching every copy — one had already drifted. Hoist the predicate into tests/common and replace integration.rs's drain_through_ack frame counts with a drain-until-quiet loop so connect-flow changes stop breaking unrelated suites.
Derive RangeKey from the range catalog, validate the range search param on parse instead of casting it, and narrow onRangeChange callbacks so invalid keys can no longer flow into detail navigation. The raw catalogs lose their exports — rangesForVariant is the only consumer.
BrowserMessage::Update carried a full ServerStatus with static fields zero-filled, an invariant enforced only by comments. Every client re-encoded it defensively: the web merge hand-picked live fields and iOS guarded totals with >0 checks but missed swapTotal, which updates stomped to 0 on every frame. Update now carries a dedicated LiveMetrics type holding only the fields an agent report can populate. The wire is compatible by subtraction (same tag, same keys, fewer of them), so older iOS builds decode the absent statics as nil and keep cached values. The web merge becomes a plain spread, update frames can no longer seed the catalog cache with placeholder rows, and the unused name field is dropped from updates. See docs/adr/0002-update-frames-carry-live-metrics.md.
Shipped iOS builds decode id and name with non-optional decode() and drop the whole frame when either key is missing, so removing name from LiveMetrics silently killed every live update on existing installs. Keep name as a documented decoder-compat field: it is the agent connection name, not live data. The web merge pins the REST-managed name so the wire value can never stomp an edited one, the server test asserts name is present while static keys stay absent, and a new iOS decoding regression pins the id+name-only requirement against the real LiveMetrics payload. ADR-0002 records the compat floor.
…keys
The upload-ack-{id} / upload-complete-{id} key format was hand-formed in
seven places across the HTTP producer (api/file.rs) and the WS consumer
(ws/agent/file_transfer.rs); a rename on either side would silently
orphan every in-flight upload. The format now lives in one place on
AgentManager, next to the pending-request mechanism it keys into.
on_system_info and on_ip_changed each hand-rolled the same audit + alert + ServerIpChanged broadcast sequence with slightly different inputs, so a fix to one path could silently miss the other. Both now funnel through apply_ip_change with an explicit IpChange struct; persistence stays with each caller. Behavior deltas: agent-IP (v4/v6) transitions detected at SystemInfo now write an audit row too (previously only remote-addr changes did), and first population (None -> Some) never audits — it happens on every fresh registration and only fed noise. Alert and broadcast semantics are unchanged, including the tolerated first-connect frames.
…sage The WS layer had two pub/sub seams for the same job: traceroute frames used the module-owned subscribeBrowserMessage registry while network_probe_update detoured through a global window CustomEvent that no test could reach. useNetworkRealtime now subscribes like every other consumer, and the branch gains dispatch + malformed-frame regression tests.
terminal.rs, docker_logs.rs and browser.rs each re-implemented the same session scaffold: credential resolve, admin-role denial audit, online check, audited capability gate, and a byte-identical mobile-token-expiry select! arm. A fix to any of those had to be applied three times. ws/session.rs now owns both pieces: admin_capability_gate runs the whole pre-upgrade sequence and returns what the pump needs (user, ip, mobile deadline), and mobile_token_expired is the shared expiry future. Routes keep only their own pumps; the open/close audit bookends stay per-route because their contexts genuinely differ.
…art-model server-detail-content.tsx buried every chart transform — the metric row mappers, the pct guard, realtime tick dedup, long-range tick and tooltip formats, the x-axis stride and GPU row shaping — as unexported inner functions, so none of it was reachable by a test and the row mapper existed three times (admin, public, realtime). lib/metric-chart-model.ts now owns the transforms behind small pure functions: one toMetricChartRow covers both admin records and public points, one pct guard is shared with toRealtimeDataPoint, and the formatters take plain rangeHours. The component keeps rendering plus thin useMemo glue. 22 new test cases cover the model; chart behavior verified in the browser across realtime/24h/7d ranges.
Four hooks hand-rolled the same ISO {from, to} window construction
(server records, network records, network anomalies, GPU records /
public metrics). Extract isoWindow(hours) into lib/utils so the query
window shape has one owner.
Hoist tick/tooltip format regexes to module scope, use T[] shorthand, and document why MetricChartRow stays a type alias (interfaces lack an implicit index signature for Record<string, unknown> chart props).
t('chart_gpu') has no entry in either locale, so the GPU usage chart
rendered the raw key as its title.
…criptor Each scalar column of records/records_hourly appeared three times in a hand-written SQL string (INSERT list, SELECT aggregate, ON CONFLICT update), and the AVG-vs-MAX aggregation decision lived only inside that string. Declare every column once in service::rollup::METRIC_COLUMNS with its RollupAgg and generate the upsert from the table. A golden test pins the generated statement byte-for-byte to the original SQL.
query_history hardcoded the 24h auto-switch inline, and the unit test covering it re-implemented the branch instead of calling it. Own the switch point in service::rollup (select_history_table + the RAW_WINDOW_MAX_HOURS constant) so bucket choice and aggregation rules live in the same module, and test the real function.
Alert evaluation kept a second private read path: check_threshold and get_last_record_time queried the records table directly, and extract_metric re-encoded the rule_type-to-column mapping as a match that nothing cross-checked against storage. The descriptor now carries alert_rule_type plus a typed accessor per column (rollup::alert_metric), and alert reads go through RecordService::query_recent / latest_record_time, so a storage change cannot silently detach alerting from what the recorder writes. Transfer counters stay deliberately non-alertable (alert_rule_type: None), matching the old fallback.
get_server_metrics hand-copied the same 16-field PublicMetricsPoint mapping for the raw and hourly query branches. Make the identical- column-set fact code: record_hourly::Model converts into record::Model, QueryHistoryResult::into_rows() erases the resolution for consumers that don't care which table served them, and the public mapping shrinks to a single field list.
Nine hand-written MetricsChart JSX blocks each re-stated a metric's dataKey, color, unit, domain, byte formatting and availability gate. Declare them once in METRIC_CHART_SPECS and render through one loop, so adding a chart is a descriptor row plus its i18n keys. RealtimeDataPoint becomes an alias of MetricChartRow — the WS feed and the records API already converged on one chart row shape; now the types say so.
ADR-0003 captures why scalar metric columns are declared once in service::rollup (generated SQL pinned by a golden test, alert reads via the shared descriptor, resolution-erasing row conversion) and why full struct codegen and OpenAPI-derived chart specs were rejected. CONTEXT.md gains the Rollup policy and Metric column descriptor terms.
The retry/demote state machine (4 fields, 5 methods) lived inside the shared ConnectionRuntime, and its transitions wrote FeaturesUpdate / DockerUnavailable straight to the WebSocket sink, so exercising them required a live daemon plus a recording sink. DockerSubsystem owns detection, promote/demote, stats-poll gating and the unavailable replies; methods return the message to send instead of writing it, and the availability transitions now have direct unit tests (announce-once demotion, msg_id correlation, stats-interval lifecycle). The dispatcher match collapses to one delegating arm; dispatch-level harness coverage in runtime::tests is unchanged. Deliberately not a full Subsystem trait across all message families: ping/terminal/file/firewall already sit behind their own managers, and a uniform trait over heterogeneous shapes would be a shallow interface serving a single hypothetical seam.
Collector hardwired sysinfo handles and eleven free-function reads, so the assembly logic (network rate differencing, counter-reset saturation, the elapsed clamp, temperature/GPU gating) could only run against the real host and tests degraded to loose >=0 invariants. The MetricsSource trait is the seam: SysinfoSource is the production adapter wrapping the existing submodules (which are unchanged), and the test FakeSource drives every assembly path with arbitrary readings — counter resets now provably read as zero-speed samples, sub-second windows provably clamp, disabled sensors provably gate to None. Real-host smoke coverage in collector/tests.rs is preserved.
…ty_gate The security-sensitive priority rule (live agent report first, persisted mirror before the first SystemInfo) was encoded three times: in agent_manager::capability_denied_reason, in security.rs's private effective_caps_or_db, and as a cache-only ad-hoc check in the ip-quality run-now guard that hard-denied during the pre-report window. Name the rule once (agent_manager::effective_capabilities_or), expose the raw mask through capability_gate::effective_capabilities for consumers with bespoke error shapes, and route both bypasses through it. The ip-quality guard now follows the same mirror-fallback semantics as every other gate instead of its own stricter variant.
The web capability constants were a hand-typed mirror of the Rust table, synced by comments, with CAP_DEFAULT = 1852 duplicated as a magic number. capability-bits.generated.ts is now emitted from ALL_CAPABILITIES by a cargo example (same pipeline shape as dump_openapi -> api-types), the generator asserts CAP_DEFAULT equals the OR of default_enabled bits, and capabilities.ts re-exports the generated definitions alongside its hand-written helpers so import sites are unchanged. Adding a capability bit is now one Rust table row plus regeneration (iOS remains a deliberate cross-repo hand mirror).
The local closure-style helper in IpChange::is_transition was named `t`, which reads as opaque next to the semantic split between changed() and is_transition(). Rename it to `transitioned`.
The integration test only asserted that some ip_changed audit row exists, leaving the first-population rule unpinned. Add unit tests asserting that None -> Some drives changed() (alert + broadcast) but not is_transition() (audit), that Some -> different and Some -> None drive both, and that equal addresses drive neither.
rollup::alert_metric returned 0.0 for unknown rule types and for the deliberately non-alertable transfer counters, so a degenerate threshold (min <= 0) could still fire on them. Return Option<f64> instead and skip valueless samples in check_threshold, turning the documented "cannot be alerted on" claim into a hard guarantee. Behavior only changes for those degenerate configurations. Also correct the RollupAgg::MaxInt doc: MAX() keeps the window maximum, which equals the window-end value only while the counter grows monotonically.
ADR-0003 and CONTEXT.md described the transfer-counter rollup as "window-end MAX"; MAX() is the window maximum (coinciding with the window-end value only for monotonic counters). Also record that alert_metric now resolves non-alertable columns to None.
Collector::collect stamps prev_time before assemble_report reads the counters, so the window is measured refresh-to-refresh. State that explicitly and note the us-level skew is absorbed by the 1s clamp.
CI runs a newer clippy (1.97) that sees through sysinfo's Networks Deref to the underlying map and flags the (_name, data) iteration. Use networks.values() in total_bytes and its test.
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.
Summary
Architecture-deepening series: turn shallow, duplicated logic into deep modules with small interfaces, plus fixes from an external review pass. 43 commits, one concern each.
Server
service::rollup): every scalar metric column declared once inMETRIC_COLUMNS(SQL name, aggregation, alert rule type, typed accessor). Hourly rollup SQL is generated from the descriptor and pinned byte-for-byte by a golden test; raw/hourly table selection and alert metric reads route through the same module (ADR-0003).capability_gate::effective_capabilities/agent_manager::effective_capabilities_or.IpChange+apply_ip_changepath shared bySystemInfoandIpChanged, with unit tests pinning first-population vs transition audit semantics.namekept in update frames for shipped iOS decoders.Agent
Collector<S: MetricsSource>with a fake source proving counter-reset saturation, sub-second clamp, and sensor gating.Web
METRIC_CHART_SPECS(color, unit, domain, availability gate) through one loop.ALL_CAPABILITIES(bun run generate:capabilities), so web constants cannot drift; generator assertsCAP_DEFAULTequals the OR of default-enabled bits.isoWindow, andRangeKeytyping centralized.Review fixes (last 5 commits)
rollup::alert_metricreturnsOption<f64>; non-alertable transfer counters and unknown rule types now contribute no sample, so degeneratemin <= 0thresholds can no longer fire on them.Testing
cargo test -p serverbee-server(1490 lib + all integration suites) — one known-flaky docker_integration test passed on standalone reruncargo test -p serverbee-agent— 886 passedcargo clippy --workspace -- -D warnings— cleanbun run test,bun run typecheck, ultracite — clean on touched web files