Harden persistent local volumes (issue #39)

[artemnikitin]

Closes the eleven code-review findings in #39 against the merged local-volume slice, following the persistent-volume hardening plan. Item 1 is deferred to #49 with the required documented caveat; the other ten are fixed here.

The plan sequences this as six PRs; per the issue they are combined into one. Within the diff the ordering that mattered was kept: the three-phase Start lands before the shrink clamp that sits between its phases, and the record schema lands before the state that uses it.

What each item does

Item 3 — a config edit could evict a whole node. fitStorage checked the local pool unconditionally, so once retained reservations exceeded a node's configured capacity that node rejected every service — including services with no volumes, whose delta is zero. BuildNodeConfigs then dropped them and the agent turned each absent service into ActionDelete. The check is now guarded on a positive local delta, because a volume already counted in the reservations cannot recover capacity by being rejected — only evict. The collapsed pending reason is split into a placement fact (volume_capacity_unavailable) and a capacity fact (node_storage_exhausted), which have opposite remedies during an incident.

Item 3b — an infeasible size is refused instead of persisted. Node discovery is hoisted above the record work so capacity is in scope. Declining the record write alone would change nothing the agent sees — the scheduling copy still carries the requested SizeBytes and renders it regardless — so a refusal clamps the rendered configuration to the last accepted size and records the request durably. The service keeps running. Marking it pending is not an option: pending drops the service and the agent turns that into a delete.

The idempotence guard keys on RequestedSizeBytes, not DesiredSizeBytes: after a rejection DesiredSizeBytes holds the effective size, so comparing against it makes the unchanged request look new on every tick and mints a generation forever. RejectedAt is preserved rather than restamped for the same reason. Raises are evaluated in name order against an evolving reservation map, so a batch cannot admit two raises that jointly exceed one pool, and every leader admits the same subset.

Item 5 — one bad record no longer stops cluster scheduling. loadVolumeRecords partitions instead of returning on the first bad key. Partitioning alone would be unsafe — dropping a record silently releases its reservation and turns a hard failure into over-commit — so each quarantine charges what it can prove and flags the scope it cannot: exact at tier 1, a lower bound plus an unknown-capacity block at tier 2, and a class-wide block at tier 3, covering both classes when even the type is unreadable (the key encodes only service and volume names). An unrecognized resize_state is carried through rather than quarantined, so a record written by a newer control plane cannot brick an older one during a rollback.

Blocking an owner does not evict it. A held service that is already placed is re-rendered from the previous placement revision at its last effective volume configuration, with its compute still reserved; one that was never placed is left pending. Quarantined outcomes enter volumeRecordsDigest — hashing the normalized outcome, excluding the display reason — so a repair actually invalidates the signature cache.

Item 2 — destructive commands are no longer SIGKILL-able by a node drain. CommandRunner splits into Run and RunDestructive; the latter detaches the context, takes its own absolute timeout, and cancels with SIGTERM plus a WaitDelay. The distinction lives in the interface, not a name match, so a new destructive command cannot inherit the cancellable path by omission.

Item 6 — an interrupted first creation is recoverable. A creation marker is written before the backing image and removed after the manifest; a matching marker authorizes re-creating the image, and an absent, unreadable, or mismatched one still quarantines. Every successful manifest read clears a surviving marker, so its delete authority cannot outlive the condition it records — otherwise a later manifest loss would let it authorize destroying populated data.

Item 9 — the command-line length check reaches the create path. writeVMConfig and Preflight now share one boot-args builder.

Item 7 — a writable path above a volume mount is chowned again. overlapsVolumePath also matched when the volume was below the path, so declaring a volume at /var/lib/app/data disabled the chown of /var/lib/app. It now matches only at or below a volume root, and the walk prunes volume subtrees rather than skipping the parent.

Item 4 — a long volume operation no longer makes the node look dead. Start held the manager lock across volume preparation, and the heartbeat goroutine reaches the same lock through List, so the node went stale precisely while it was busy resizing its own services' volumes. Start is split into three phases with an explicit start barrier: a startID-stamped StateStarting placeholder, unlocked preparation, and a phase 3 that confirms ownership before any side effect. Stop and Remove abort an in-flight start and return immediately rather than waiting out an mkfs. An aborted start is reported as incomplete rather than failed, so the tick neither advances lastRevision nor claims the revision applied — otherwise the next tick takes the unchanged-revision shortcut and the service stays down until the revision changes.

Item 8 — a refused resize is terminal instead of a stop/restart loop. A rejection is now a decision rather than a fault: prepareOne records it in the manifest and returns the volume at its applied size with no error, so one pass collects every refusal (a retry budget cannot survive a second rejected volume). Terminality does not depend on the control plane — the agent normalizes the desired node config through the durable manifests before anything else in the tick reads it. The advisory preflight rejection is terminal by the same mechanism, and its whole read/measure/write now runs under the same per-volume lifecycle lock prepareOne takes. On rejection the checking transaction is removed before the manifest is written, so a crash between them loses only the record rather than leaving the stale transaction that quarantines the corrected retry.

Visibility does depend on the control plane: a rejection is reported as a third volume state carrying the requested generation, and acknowledgeVolumeRecords converges the record on the effective size in one write.

Items 10 and 11. Pool gauges are published once per tick from retained state rather than from the admission path, so they no longer vanish on a node holding retained-but-unplaced volumes. mergeVolumeStatuses gains the missing <= 0 fallback for AppliedSizeBytes, plus fallbacks for the three control-plane-sourced rejection fields — the agent is handed the clamped config and does not know a rejection happened, so whole-struct replacement would clear them every cycle.

Convergence signal. A standing rejection and a held service both leave a healthy workload running, so neither fails the node — but neither is convergence. A new non-blocking VolumeSizesApplied condition degrades the node, and held services degrade the revision status. Without this the node reports ready while running a size nobody asked for.

Deliberate divergences from the plan

• The capacity rejection carries RejectedReason rather than ResizeState. §4.2.2 says ResizeState must not carry it (it describes the last resize the agent attempted), while §4.2.4's guard text keys on VolumeResizeRejected; the two cannot both hold. VolumeResizeRejected is retained for the agent-reported shrink rejection, which did enter that state machine.
• The clamp substitutes generation as well as size. §7.3.0 says size only, but needsUpdate compares whole VolumeConfig structs, so a size-only clamp leaves the generation differing forever and re-plans the update on every tick. The refused generation travels separately, in the rejection record, which is what the acknowledgement matches on.

Rollback

Rolling the agent back below this change re-opens the resize loop. An older agent ignores the manifest's rejection fields and re-attempts the refused configuration until the controller's clamp reaches it — and if the rollback also predates the controller change, indefinitely. Roll the agent back only after confirming the controller is rendering effective sizes; otherwise stop the affected services first.

cmd/fc-init ships a new guest binary, so a service that declares a volume under a writable path stays broken until its guest image is rebuilt on both architectures.

Coordination

PR #47 (node-exclusive host-port claims) merged as 30e6295 and is now merged into this branch. The two changes interacted in two places:

• Textual: both add a per-candidate rejection tracker to the same loop in ScheduleWithStorage. Both are kept; the port check stays ahead of fitStorage as Make host ports node-exclusive placement claims #47 placed it, so a port-rejected node commits no storage usage, and a host-port conflict outranks a storage reason in the pending vocabulary — the port check runs first, so a node rejected on ports is never evaluated for storage and any storage reason recorded elsewhere describes a different node than the one the operator has to fix. That precedence is now stated where the constant is defined rather than left implicit in statement order.
• Semantic: a held service — one re-rendered outside the scheduler because its own volume record could not be read — still holds its host ports, and the scheduler could not see them. Neither branch had this hole; only the combination does. ScheduleWithStorage now takes the pinned claims explicitly and the controller supplies them from the held set, the same way reserveHeldCapacity already reserves their compute. Without it the scheduler would hand a held service's port to a newcomer on the same node, the agent would reject the whole rendered node config, and an unreadable record for one service would take down every service on that node.

Review rounds

Four review rounds against this branch found seventeen defects, all reproduced with focused probes and all fixed, each with a test that fails without its fix — sixteen here, and one that cannot be fixed here at all (see below). Every one of the seventeen passed go test ./..., targeted race tests, and all eight CI checks — none was reachable from the paths this branch's own tests exercised, which is worth knowing when weighing the validation below.

Round one (five): an acknowledged shrink rejection restarted the VM on every reconcile; a placement read failure could evict a held service; the rejection snapshot was never reconciled as a whole; capacity admission was applied to shrinks, refusing the one operation that recovers an over-capacity pool; and admission rewrote an unknown resize_state.

Round two (four): a missing placement object bypassed the eviction guard, which keyed on the read error alone; the clamp kept reporting a refusal after the request was withdrawn — not direct-Git-only, since reverting size: under a controller clears the record while the controller renders the identical shape; configcheck --node-config parsed without validating; and an absent resize_state was treated as a future protocol's rather than as malformed.

Round three (three): a refusal on a deleted service degraded every later revision, because retained records outlive their service and the signal was not filtered by the desired revision; withdrawing a refusal cleared its fields but left ResizeState: rejected, publishing state: rejected beside rejected: false with no path left to repair it; and configcheck --node-config validated generic service fields but not the volume contract, accepting a local volume with no bound_node that the agent then refuses to start.

Round four (five): retained manifests with a non-positive applied size bypassed pool capacity, because a negative size subtracts from the reserved total; a stale heartbeat reopened a withdrawn refusal, producing two durable writes per tick; an explicit zero-volume prior render was mistaken for a missing snapshot and replaced with the unvalidated desired config; ValidateNodeVolumes checked volume names but not the service name the agent also turns into a path component; and — the one that is not a code defect here — item 2's design was incomplete.

Item 2 needs a change in another repository

RunDestructive detaches a filesystem utility from Go context cancellation, and §5.1 of the plan treated that as closing the window where "a systemd restart, deploy, or node drain during a shrink SIGKILLs resize2fs mid-operation". It does not. Detaching from a Go context does not detach a child from the agent's cgroup: under systemd's default KillMode=control-group, stopping the unit signals every process in it — the detached resize2fs included — and force-kills the group at TimeoutStopSec. The named scenario was still unprotected.

The unit must set KillMode=mixed and a TimeoutStopSec above destructiveCommandTimeout. Those two numbers are a cross-repository contract; raising either alone reopens the gap. This PR documents it in docs/persistent-volumes.md and on the constant; the unit change is firework-deployment-example#24, and item 2 is not complete until both land.

It also promotes one validation gap: the live SIGTERM-during-shrink test is now a release gate for item 2 rather than missing confidence, because the protection spans two repositories and cannot be verified in either alone.

The second round's central fix is a design correction worth calling out: clamping and reporting a refusal are different questions and must be keyed differently. The clamp applies for as long as the refused generation stands, or the generation diverges from the running instance and the service restarts. A report of a standing refusal requires the refused size to actually be requested — and once the control plane acknowledges, the two shapes are identical bytes, so only the record can tell a standing request from a withdrawn one. A standing record refusal therefore degrades the revision status. That is the division §7.3.2 of the plan already draws: terminality does not depend on the control plane, visibility does.

Round three added two rules the plan never stated at all, now written into §12.2.1: a retained record is not evidence that anything still wants what it describes, so any convergence signal derived from retained state must be filtered by the desired revision; and a state machine's exit must be as complete as its entry, because clearing a terminal state's detail fields while leaving the state itself publishes a contradiction that no later path revisits.

Validation

make fmt, make test, make test-race, and make lint are green. cmd/fc-init is //go:build linux, so its tests were run in a linux container rather than on the development host. The privileged end-to-end paths — an agent SIGTERM during a live shrink, a multi-minute Prepare against a real heartbeat, and the guest chown on both architectures — need the #42 validation lab and were not executed.

Refs #39, #19, #49