Complete agent reconciliation and fleet revision status - #35
Merged
Conversation
Fleet convergence rejected truncated service data but never checked that the services it did receive covered the node, and assessConditions treats an empty condition list as clean. A current PhaseReady heartbeat with matching revisions, no services, and no conditions was therefore classified as converged: the endpoint reported a healthy fleet on the strength of evidence it never received. This also disagreed with /v1/services, which reports service_status_missing for the same snapshot. The node check now reuses findAgentService, so the two views cannot diverge. An agent that completes a tick always sends every condition, because it finalizes unevaluated ones as unknown. An incomplete set therefore means the telemetry does not describe a completed reconciliation, and requiring the blocking set costs a healthy agent nothing. PeerRoutesReady is not required: being non-blocking, its absence can only mask a degraded classification, never a failed one. The shared test helper now builds the telemetry a healthy agent actually sends. The degraded and future-condition cases append to that set rather than replacing it — replacing it would have made them incomplete, so they would have passed as unknown and quietly lost their coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two separate ways the reported conditions disagreed with reality. NetworkReady was set true immediately after routing metadata preflight, but host networking is attempted later, inside Reconcile. A tick that failed at capacity or image sync therefore reported NetworkReady=true for a revision whose networking was never attempted. It is now left unevaluated so finishTickStatus finalizes it as unknown, which is what the per-tick contract says an unreached stage should be. The success path still sets it after Reconcile returns, and the unchanged-revision fast path still covers it, so a converged node is unaffected. Metrics were snapshotted inside failAgentStatus, before finishTickStatus rewrote unreached conditions to unknown. After an earlier successful tick Prometheus kept reporting later conditions as true while /status and the registry reported them unknown, and on a first failing tick it omitted them. Refreshing the snapshot inside finishTickStatus covers every early failure path through one choke point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A SetupPortForward failure during create was logged and discarded, so Reconcile returned success and the agent advanced the applied revision. Because an existing VM produces no further plan actions, nothing retried it: the fleet stayed permanently converged while cross-node routing was broken. Neither obvious fix works. Returning the error without retrying reports one failed tick and then converges again. Rolling the service back retries but crash-loops a running VM on a durable failure such as the host-port collision this deployment has already hit. So port forwards now converge on every tick, like peer routes already do. Re-asserting is cheap because the helper checks before it adds, a transient failure self-heals, and a persistent one keeps failing the tick instead of being reported once and lost. The create path still attempts setup for immediacy, and its warning now says what retries it. Teardown failures are recorded for the tick rather than returned, because the update path skips the recreate when deleteService errors and a transient teardown failure must not leave a service deleted. They surface in the tick result instead. Obsolete rules left by a failed teardown are still not pruned; that needs rule enumeration rather than asserting desired state, and is not attempted here. Host networking moves behind an interface so this is testable without touching real iptables rules. New keeps a nil *network.Manager out of the interface: stored directly it would be a non-nil interface, and every guard would pass before dereferencing nil. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Input validation required non-empty, unique service names but no length limit, while the registry discards an entire agent_status carrying a name over 128 bytes. An accepted configuration could therefore run while every heartbeat describing it was dropped, leaving the node permanently unknown in fleet visibility with nothing pointing at the cause. Validation runs after tenant expansion, so this checks the composed name that is actually reported, not the input name. The limit is read from statusmodel rather than restated, so the two cannot drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correcting 89d8a2a, whose message claimed port forwards "converge on every tick, like peer routes already do". They did not. The convergence lived inside Reconcile, and the unchanged-revision fast path returns before Reconcile. On a single-label node with a store revision — the common case — steady-state ticks skipped it entirely, so the rules were re-asserted only when the revision changed. Peer routes really are refreshed on that path, which is what made the comparison wrong rather than merely imprecise. The failure this was written for is exactly the one it missed. Host DNAT is flushed by things that have nothing to do with the config revision: a firewalld reload, a container runtime restart. Meanwhile the same path calls markUnchangedRevisionReady, which reports NetworkReady, and the control plane now treats that as evidence of convergence. A node could report converged with its port forwards gone. The fast path now converges port forwards through an exported SyncPortForwards and only reports ready when that succeeded. The route refresh still runs either way, so one failing subsystem does not stop the other from converging. The earlier tests could not have caught this: they drive Reconcile directly. The new ones drive the agent tick, which is where the gap was. markUnchangedRevisionReady's comment now also distinguishes the stages it genuinely re-verifies from the ones it infers from the revision being unchanged, since the control plane weighs them all equally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 4 MiB cap sat below the largest agent_status the validator accepts, measured at 11.81 MiB. The bound is driven by per-volume messages: MaxServices x MaxServiceVolumes is 6400 volumes, each with a LastError that BoundedMessage limits to 256 runes rather than bytes, so up to 1 KiB apiece. An oversized body is truncated by MaxBytesReader and fails JSON decoding, so the handler returns 400 before it can reach validatedHeartbeatAgentStatus. That rejects the node's liveness heartbeat, not just its telemetry: LastSeenAt stops advancing, the node goes stale and then down, and the fleet reports unknown. Treating status as optional telemetry was meant to prevent exactly that, so a valid but large status failed harder than an invalid one. The new test measures the largest valid heartbeat and fails if the model bounds ever grow past the cap, so this cannot drift back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setAgentStatusSnapshot wrote lastAppliedAt and lastAppliedRevision, which recordConfigApply owns. recordConfigApply reports the store revision; the snapshot carries AppliedRevision, which markAgentStatusApplied has already rewritten to the rendered revision. The later write won, so firework_agent_config_last_applied_revision_info silently changed which revision it names, and any dashboard correlating that label with store revisions stopped matching with no other signal. The snapshot now writes only the phase and condition gauges it is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reconciliation conditions were written out in three places: the agent's per-tick list, the control plane's blocking set, and the literal case list inside assessConditions. They agreed, but nothing held them together. That matters because the control plane refuses to call a node converged unless every blocking condition is present. An agent-side rename without a matching control-plane change would leave no node ever classifiable as converged — silently, and permanently, since a missing condition looks the same as incomplete telemetry. All three now derive from statusmodel, and assessConditions classifies by lookup rather than a literal list. A test pins the invariant the control plane depends on: every condition it requires is one an agent reports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…at size - Empty desired state no longer converges until every known node confirms it via a fresh heartbeat, since the controller deletes legacy node configs instead of publishing explicit empty ones and an agent treats the missing file as a fetch failure. - A failed DNAT/network teardown after a VM is removed is now retried every tick (and, via WithStateDir, persisted across an agent restart) instead of being forgotten once Plan has no further delete action for it. Retries distinguish a reclaimed service name from a genuinely deleted one so they never tear down a live VM's own tap device, while still catching and retrying a stale DNAT rule left behind by a changed port forward. - The 16 MiB heartbeat cap is now actually bounded end-to-end: every previously-unbounded AgentStatus/ServiceStatus/VolumeStatus field is either length-validated or (for mount_path and the composed LogicalID) truncated on the agent's send path and rejected at config validation time, with a test that builds the worst case through the real agent status path rather than by hand. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… name pendingNetworkTeardowns had the same overwrite/erase bug as the port-forward map fixed in 45bae2a: it was keyed by service name on the false premise that a tap device's path is derived purely from the name. svc.Network.Interface, when set explicitly, overrides the default tap-<name> path, so a service moving through two consecutive generations (A -> B -> C) before an earlier device's teardown ever succeeds could lose track of A's still-pending tap the same way. Key pendingNetworkTeardowns by (tapName, bridgeName) instead, mirroring the pendingPortForwards design, with TestReconcile_ConsecutiveUpdatesTrackEachGenerationsStaleNetworkDeviceIndependently as the regression proof. Also: atomicWriteFile now fsyncs the parent directory after the rename, so the journal's atomicity claim covers the rename's own durability, not just the write. Documented the new node_id/backend_id byte limits in docs/configs/README.md. The persisted journal's network field changed shape (map -> slice) to carry a struct key; this is safe since the branch has never been deployed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ately Two blocking review findings at 1fff31a. Write-ahead journal. deleteService removed the VM first and only then recorded what still needed cleaning up. A crash in between left the VM gone, no delete action for Plan to ever emit again (the service is absent from both desired and actual state), and no durable record of the host resources it left behind: the node restarted and reported converged with stale DNAT rules and devices on the host. The intended cleanup is now journalled before Remove, and a failed journal write aborts the removal instead of proceeding unrecorded. persistPendingTeardowns returns an error rather than only logging one, and refuses to write on a corrupt journal -- Reconcile appends the corrupt-journal error but continues, so the write-ahead gate has to fail closed itself. Entries are cleared as each resource is actually released; a failure on that path is logged only, since every operation the journal drives is idempotent. Independent network device tracking. TAP and bridge were tracked as one composite (tapName, bridgeName) identity and retried through NetworkManager.Teardown, which always deletes both. A config that drops host_dev_name while keeping the same explicit interface makes only the bridge obsolete, but the composite key differed, so the retry fired and took the live TAP down with it while reporting success. Each device is now tracked by its own (kind, name) identity and released through new component-specific DeleteTAP/DeleteBridge operations, and no device claimed by any running service is ever deleted. Teardown stays on the interface for createService's rollback, where deleting both components is correct because it undoes a Setup from the same call; that is now commented rather than left to be re-flagged. The persisted journal's network field changed shape again to carry the per-device key; safe since the branch has never been deployed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sion the journal Addresses four of seven review findings on 94e8e78. [P1] Port-forward retry could delete a rule claimed by another service. retryPendingPortForwards guarded only against entry.ServiceName being live, but a DNAT rule is identified by its tuple alone: after a rename or replacement, another service can legitimately claim the exact (hostPort, guestIP, vmPort) a previous one left pending, and the retry would delete that live rule while reporting success. This was an incomplete application of the previous commit -- the identical global claimant guard already existed in retryPendingNetworkDevices. [P1] Failed-create networking cleanup was untracked. createService discarded the rollback error after a failed VM start and journalled nothing beforehand. A service whose Start never succeeded is absent from vmManager.List(), so once it is dropped from desired state Plan emits no delete action and its devices are orphaned; a crash between Setup and Start left them with no record at all. Devices are now journalled before Setup, the rollback goes through the component-level teardown so failures are recorded and retried, and the entries are cleared once the running VM claims them. [P1, partial] TeardownPortForward reported success when it could not resolve the host ingress context: it fell through to legacy-only cleanup, and removeIPTablesRule treats "no such rule" as success, so the scoped DNAT rule survived while teardown returned nil. Resolution failure is now a teardown failure, which keeps the rule on the retry list. Persisting the resolved rule identity at setup time is the remaining half and is not in this change. [P2] The journal is now versioned, decoded with DisallowUnknownFields, and validated on load. The previous release's shape is valid JSON that decodes into zero-valued entries -- a device with an empty kind and name deletes nothing and then clears itself -- which silently discarded exactly the cleanup the journal exists to guarantee. Any other shape now fails closed. The composite Teardown is removed from the reconciler's NetworkManager interface entirely; every removal now goes through DeleteTAP/DeleteBridge one resource at a time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed VM converged Addresses all four remaining review findings on 88ee1ac. [P1] Partial node retirement reported false convergence. scheduler.BuildNodeConfigs omits a node left with no services, and publishRendered then deletes its nodes/<node>.yaml. An agent treats a missing config as a fetch failure, not as "run nothing", so it keeps every VM it had -- while the fleet view derives its relevant node set from the same placement and stops counting that node at all. During an A -> B migration, B alone converged the revision while A still ran the old workload. The controller now appends an explicit empty config for every active node the placement gave no work to, before the revision metadata is stamped, so retirement becomes an ordinary convergence step: the node fetches it, reconciles to zero services, reports the new revision, and stays in the relevant set until it does. Nodes absent from activeNodes entirely (down, stale, decommissioned) are deliberately untouched. [P1] The unchanged-revision fast path claimed a crashed VM as converged. markUnchangedRevisionReady asserted VMsReconciled and Reconciled true unconditionally. Those describe work whose inputs have not changed, which is not the same as its outputs still holding: a VM can fail after the revision was applied, and this path skips reconciliation entirely. It now checks for failed VMs first and reports vm_reconcile_failed -- the same vocabulary the reconciling path uses -- and returns false so the caller does not publish PhaseReady over the top of a failed condition. [P2] The journal decoder accepted trailing content. json.Decoder.Decode reads one value and stops, so {"version":1}{"network":[...]} was accepted as an empty journal. A second decode must now return io.EOF. [P2] A corrupt journal blocked non-networked creates. journalPendingCleanup persisted even when the mutation added nothing, so a service with no network config -- and therefore no host resources to clean up -- could not start. It now skips the write when neither map changed. 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.
Closes #24.
Summary
Current
mainalready carries the initial revision fields, agent heartbeat snapshot, registry persistence, and node/service visibility from PR #29. This PR completes the remaining issue #24 acceptance gaps:GET /v1/status,fireworkctl status, and an overview UI section deriving published/progressing/converged/degraded/failed/unknown fleet stateIssue #22 is implemented separately in PR #34 and should be merged first as the runtime-recovery prerequisite described by the design. This branch is independently based on current
main; after #34 merges, this PR may need a small conflict-only rebase in the shared agent/reconciler files.Validation
make fmtmake testmake lintmake test-racemake smoke-localmake build-allnode --check internal/controlplane/web/app.js