From b58d7bcf39a9a4ff5fc5101f98ea8cd0eaea5ea8 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Wed, 29 Jul 2026 17:20:16 +0200 Subject: [PATCH] =?UTF-8?q?v0.114.0=20=E2=80=94=20R-113:=20drive=20presenc?= =?UTF-8?q?e=20means=20the=20DEVICE,=20not=20the=20bind?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BoundUnderParent, the one field the controller's drive-absent gate keys on, reported only "is this path a mount target in the guest's mountinfo". The drive's raw mount at /mnt/ is a systemd mount unit bound to its device and dies with it, but the agent's own bind of /felhom-data under the shared parent is an ordinary bind: nothing ties it to the device, so its mountinfo entry OUTLIVES the device as a stale shell. Presence read that survivor as true, planDriveGates never produced a Stop action, and nothing fired on any channel -- not backup_target_absent, not the generic storage_disconnected. Measured live in E-2d: detached at 10:58:37Z, silent for 4.5 minutes while the agent itself logged "enrolled drive absent by UUID" every 20s (felhom.eu audits/E2D-fresh-vm-2026-07-29.md §5.2). The fix: BoundUnderParent becomes a CONJUNCTION -- bound under the parent AND the drive's raw host mount still mounted (devicePresent, new deviceCheck seam defaulting to isHostMountpoint). Applied at BOTH /disks construction sites. The union path matters more, not less: it hardcodes State:"attached", so the raw-mount check is the only device truth that row carries, and it is exactly the shape E-2d detached. Why a conjunction and not a replacement: half 2 alone would regress boot ordering, where the raw drive mounts early and the bind lands ~18s later; the gate depends on that window reading ABSENT. The conjunction leaves that byte-identical and closes only the case the gate could never see. Unknown is never absent: devicePresent("") returns TRUE. A false absent stops a working customer's apps -- the failure mode of this fix, not of the bug. Controller UNCHANGED, no MinAgent bump. BoundUnderParent has exactly one functional consumer (planDriveGates, intermediary.go:226); every other mention in both repos is a comment or a test, and boot convergence deliberately moved off it to pollLiveBinds/driveBindLive. The alternative -- a new DevicePresent bool the controller ANDs in -- was rejected as dangerous: a bool absent from an older agent's JSON decodes to false, so every drive on a pre-0.114.0 agent would have read ABSENT and stopped its apps. Tests +6 in internal/localapi (208 -> 214): groups A-D plus a wire-contract test asserting the ENCODED bound_under_parent, since that is what crosses to the controller. Four red-proofs run and reverted (drop the conjunction on each path; invert unknown; drop the bind half); disks.go verified byte-identical after. NOT LIVE-VALIDATED. No drive was pulled. Leg awaiting Session C: device loss -> gate Stop -> SetDisconnected -> backup_target_absent on the wire. --- CHANGELOG.md | 43 +++ REPORT.md | 284 +++++------------- REUSE.md | 3 +- internal/localapi/disks.go | 49 ++- .../localapi/disks_device_presence_test.go | 186 ++++++++++++ internal/localapi/intermediary_test.go | 5 + internal/localapi/server.go | 7 + 7 files changed, 364 insertions(+), 213 deletions(-) create mode 100644 internal/localapi/disks_device_presence_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 322e4b2..f689e8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,46 @@ +## v0.114.0 — R-113: drive presence means the DEVICE, not the bind (2026-07-29) + +**The bug, measured live in E-2d.** `BoundUnderParent` — the one field the controller's drive-absent +gate keys on — reported only *"is this path a mount target in the guest's mountinfo"*. The drive's raw +mount at `/mnt/` is a systemd mount unit bound to its device and dies with it, but the agent's own +bind of `/felhom-data` under the shared parent is an ordinary bind: nothing ties it to the device, +so **its mountinfo entry outlives the device as a stale shell**. Presence read that survivor as `true`, +`planDriveGates` never produced a `Stop` action, and **nothing fired on any channel** — not +`backup_target_absent`, not the generic `storage_disconnected`. Detached at 10:58:37Z, silent for 4½ +minutes while the agent itself logged `enrolled drive absent by UUID` every 20 s +(`felhom.eu` `audits/E2D-fresh-vm-2026-07-29.md` §5.2). + +**The fix.** `BoundUnderParent` becomes a **conjunction**: bound under the parent **AND** the drive's raw +host mount still mounted (`devicePresent`, new `deviceCheck` seam defaulting to `isHostMountpoint`). +Applied at BOTH `/disks` construction sites — the Observe path and the registry union path. The union +path matters more, not less: it hardcodes `State:"attached"`, so the raw-mount check is the only device +truth that row carries, and it is exactly the shape E-2d detached. + +**Why a conjunction and not a replacement.** Half 2 alone would regress boot ordering, where the raw +drive mounts early and the bind lands ~18 s later; the gate depends on that window reading ABSENT so apps +stay stopped until the bind is live. The conjunction leaves that behaviour byte-identical and closes only +the case the gate could never see. Pinned by `TestDisks_DevicePresence_BootWindowStillReadsAbsent`, whose +red-proof is dropping the bind half. + +**Unknown is never absent.** `devicePresent("")` returns **true**. A false absent stops a working +customer's apps — the failure mode of this fix, not of the bug — so an unanswerable question must never +be reported as "gone". Red-proofed by inverting it. + +**Controller UNCHANGED, no MinAgent bump.** `BoundUnderParent` has exactly one functional consumer, +`planDriveGates` (`intermediary.go:226`); every other mention across both repos is a comment or a test, +and boot convergence deliberately moved off it to `pollLiveBinds`/`driveBindLive`. Tightening the field's +meaning therefore changes exactly one decision — the broken one. The alternative (a new `DevicePresent` +bool the controller ANDs in) was rejected as actively dangerous: a bool absent from an older agent's JSON +decodes to `false`, so every drive on a pre-0.114.0 agent would have read ABSENT and stopped its apps. +An older controller against this agent simply gets a more truthful bool. + +**Tests:** +6 in `internal/localapi` (208 → 214), groups A–D plus a wire-contract test that asserts the +encoded `bound_under_parent` field, since that is what actually crosses to the controller. Four +red-proofs run and reverted (drop the conjunction on each path; invert unknown; drop the bind half). + +**NOT LIVE-VALIDATED.** No drive was pulled. The leg awaiting Session C: device loss → gate `Stop` → +`SetDisconnected` → `backup_target_absent` on the wire. + ## v0.113.0 — E-2a: the guarded backup-target wrapper + POST /backup/target (2026-07-29) **The agent cannot do this itself, and that is the point.** Creating a PVE storage needs diff --git a/REPORT.md b/REPORT.md index 3567871..df3366b 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,230 +1,98 @@ -# REPORT — F-REBOOT + F-LEAK (v0.107.0 → v0.110.0) (2026-07-28) +# REPORT — R-113: drive presence means the DEVICE, not the bind (agent v0.114.0, 2026-07-29) -**Overwritten** per the standing rule. Agent **v0.106.0 → v0.110.0** across four commits, **three of them -corrections to the first**. F-LEAK took three attempts and two of them were refuted by live test; that -arc is the substance of this report. Companions: `felhom-controller` v0.180.0 (F-OBS) and `felhom.eu` -host-install v1.21.0 (F-LEAK's actual fix). +**Overwritten** per the standing rule. R-113, Session A of three (R-113 → R-114+R-112 → one drill-VM +validating all three). **Repos touched:** `felhom-agent` only; **`felhom-controller` deliberately +UNCHANGED** — see the ruling. **Status: code + tests complete, NOT LIVE-VALIDATED.** No drive was pulled; +that is Session C. -## Baselines (reconfirmed, not copied) -Agent `0.106.0` live on both boxes at start; controller `0.179.0`; `felhom-agent af1c21a`, -`felhom.eu d0cec9d`, `felhom-controller 8f46495`, all clean. +## Phase 0 ruling — shape (a), the agent tells the truth ---- +`BoundUnderParent` becomes `bound && devicePresent`. Controller unchanged, no `MinAgent` bump. -## F-REBOOT — a guest rebooted mid-backup never came back (v0.107.0, SHIPPED + PROVEN LIVE) - -Fault 11: the backup **succeeded**, and the guest was found `stopped` with 0 containers, no lock, and -nothing retrying — 9m47s of total appliance outage with every alarm silent, because nothing was broken -except that the box was off. - -**Why the existing recovery missed it.** `RecoverStaleLockedGuests` already does unlock → delete -dangling snapshot → **start iff onboot**, and it is correct. Two narrow gaps: its predicate acts only -on a guest holding a **stale vzdump lock** (fault 11's guest was unlocked), and it runs **once at agent -startup** on the load-bearing invariant that a lock present *then* is stale by definition — a guest -that goes down while the agent is already up is never re-examined. - -`internal/localapi/guestpower.go` closes exactly those two and nothing more: periodic (60 s), acting on -"should be running, is not, is not locked". - -### The trap, which is F-CRIT-1's shape -A deliberately stopped guest must not be auto-started; fighting the operator is worse than the outage. -The discriminator is `onboot`, and it is deliberately **not invented here** — it is already what -`stalelock.go` uses for this same decision (so the two paths cannot disagree), it is 0 on -scratch/golden guests, and it is what `pve-guests` itself consults at host boot, so the agent agrees -with the platform instead of keeping a second private definition of "should be running". The hub's -desired-state `Run` is stronger but hub-dependent; `onboot` still works on a box that has lost hub -contact, which is when an unattended appliance most needs to come back. - -Guards, in order: running → forget history · `!onboot` → return (Scenario B) · `lock != ""` → leave to -the stale-lock path · `BackupRunning` true **or error** → do not start (a stop-mode backup stops the -guest on purpose) · bounded 3 attempts at 1m/2m/4m → then ERROR once (Scenario C). - -### Live, on demo-hp -| | | +| Q | Answer | |---|---| -| guest stopped out from under the agent | recovered **unattended in 120 s** | -| the finding's original incident | **587 s**, needing a human `pct start` | -| lock-deferral guard | **fired live** during the replay | -| Scenario B, same box | an `onboot:0` guest left stopped across the whole window | +| 1. the symbol | `ResolveStorageDevice` (`internal/storage/durable_device.go:82`, `EvalSymlinks` of `/dev/disk/by-uuid/`) behind `hostops.go:298`; `mountByDurable` (`localapi/disks.go:960-969`, **raw mounts only** — binds skipped at `:962`) behind `disks.go:986` | +| 2. available at `/disks`? | Yes at both sites. Observe path has `t.MountPath`/`t.State`; union path has `d.MountPath`/`d.UUID` and already resolves that UUID eight lines later | +| 3. cost | `os.Stat` / `EvalSymlinks` — at most two syscalls per drive per poll. No exec, no shell-out | +| 4. `State == "attached"` | **Split verdict.** Observe path: truthful (`reachable()`, `storage/observe.go:321-330` = `exactMount && DeviceExists`). Union path: **hardcoded `"attached"`** (`disks.go:257`) — so `planDriveGates:229`'s legacy raw branch carries the same defect. **Filed as an observation, not fixed** | ---- +**Why (a) and not (b):** `BoundUnderParent` has exactly **one** functional consumer, `planDriveGates` +(`intermediary.go:226`); everything else across both repos is a comment or a test, and boot convergence +deliberately moved off it (`intermediary.go:362`). Tightening it changes one decision — the broken one. +Shape (b) is actively dangerous: a new `bool` absent from a pre-0.114.0 agent's JSON decodes to `false`, +so ANDing it in would make **every drive on an older agent read ABSENT and stop its apps** — the exact +Scenario D failure the spec forbids. Defending it needs `*bool` + feature detection: more machinery, worse +failure mode. -## F-LEAK — and the first fix was WRONG, refuted by its own live replay +## The signal chosen, and why it is the right one -**What I shipped in v0.107.0 and then removed in v0.108.0.** A teardown fallback that, on the 403, -adopted the stranded scratch into the `felhom` pool and retried the destroy — reasoning that the token -holds `Pool.Allocate` on `/pool/felhom`. It fired exactly as designed and PVE refused it: +Not a UUID probe — the **raw mount**. E-2d measured the asymmetry directly: with the device pulled, +`/mnt/mentes2` was NOT mounted while `/mnt/felhom-drives/mentes2` still read `/dev/sdb[/felhom-data]`. +The raw mount is a device-bound systemd unit; the agent's bind is not. So "is the raw mount still +mounted" **is** device presence, it is the same check on both construction paths, and it rests on a live +measurement rather than inference. -``` -WARN restore-test: scratch teardown failed — adopting the stranded scratch ... vmid=990000 pool=felhom -ERROR restore-test: pool adoption failed; left for Recover vmid=990000 - err="proxmox: PUT /pools/felhom -> HTTP 500: permission denied at /vms/990000 (missing privilege ...)" -``` +## Files modified -`PUT /pools/{pool}` **also** requires `VM.Allocate` on the VM being added. **Pool membership cannot -bootstrap its own authority.** The code and its guard were removed rather than left in place — a path -that provably cannot work is worse than no path, because it looks like a fix. - -**The real cause is structural**, not a missing privilege in the role: `FelhomAgentGuest` is granted at -`/pool/felhom`, and a guest joins that pool only when its restore **completes**. A *failed* -restore-test therefore leaves a guest that exists, is in no pool, and is outside the token's reach. - -### Attempt 2 — the band-scoped ACL. It works exactly ONCE per slot. -`felhom-host-install.sh` v1.21.0 grants the role at each `/vms/990000`…`/vms/990009`. The live A/B on -the same guest, minutes apart, proved it does close the defect: - -| | `DELETE /nodes//lxc/990000` with the agent's own token | +| File | Change | |---|---| -| **grant removed** (the original defect) | `403 Permission check failed (/vms/990000, VM.Allocate)` — guest still present | -| **grant restored** | `200 UPID:...:vzdestroy:990000:felhom-agent@pve!agent` — guest gone | +| `internal/localapi/server.go` | new `deviceCheck func(string) bool` seam (mirrors `boundCheck`) | +| `internal/localapi/disks.go` | new `devicePresent(rawMountPath)`; conjunction at BOTH construction sites (`:210` Observe, `:263` union); `BoundUnderParent`'s doc restated to name both halves | +| `internal/localapi/intermediary_test.go` | existing fixture pinned `deviceCheck=true` — its subject is the bind half; unchanged otherwise | +| `internal/localapi/disks_device_presence_test.go` | **new** — 6 tests, groups A–D + the wire contract | +| `REUSE.md` | `devicePresent`/`deviceCheck` registered, with the "use this, never the bind" rule | -**And then the post-run ACL count caught it:** `/vms/990000` had **0 grants** afterwards, while the -other nine slots had 2 each. Confirmed in PVE's own source rather than inferred — the destroy path -calls `AccessControl::remove_vm_access($vmid)` (`API2/LXC.pm:906`), which deletes -`acl_root->children->vms->children->{$vmid}`, i.e. **every ACL at `/vms/`** -(`AccessControl.pm:1898`). +## Tests: 208 → 214 in `internal/localapi` (+6). Full agent suite green. -**The grant is consumed by the very operation it authorises.** Ten teardowns and the band is ungranted -and the defect is back. A per-VM ACL is structurally the wrong tool; the pool grant is durable only -because it lives on a *pool* path, which `remove_vm_access` leaves alone (it removes the guest's pool -*membership*, not the pool's ACL). I would not have found this without checking the box after the fix. - -### Attempt 3 — the fourth root-fenced exception (v0.110.0, SHIPPED) -The route this task offered as the alternative: **the same authority that already performs the create.** -`Privileged.DestroyScratchLXC` joins the fence (previously exactly three: keyctl `pct create`, USB -mount/fstab, SMART/sensors) and is fenced harder than any of them — in three places, none consumed by -use: - -| layer | enforcement | -|---|---| -| **sudoers** | `/usr/sbin/pct destroy 99000[0-9] --purge` — sudo matches the vmid **literally**; a compromised agent asking for `9201` is refused by sudo itself | -| **`DestroyScratchLXC`** | re-checks the band before exec; refuses an unconfigured or inverted band rather than defaulting to anything | -| **`teardownScratch`** | acts only on this journal entry's own scratch provenance | - -The API destroy is still tried **first** and remains the normal path; the v1.21.0 band ACLs stay -provisioned so the common case needs no privileged call. This is the fallback that makes teardown -deterministic instead of once-per-slot. Ships **with a sudoers change**, deployed alongside the binary. - -### The fence, proven live by ASKING sudo rather than executing -Executing the negative case against a live guest is exactly what must not happen, so `sudo -l` was used -to ask whether each command is permitted: - -| command | verdict | -|---|---| -| `pct destroy 990000 / 990005 / 990009 --purge` | **PERMITTED** | -| `pct destroy 9201 --purge` (the live customer guest) | **REFUSED** | -| `pct destroy 9100 / 9999 / 1 --purge` | **REFUSED** | -| `pct destroy 990010 --purge` (one past the band) | **REFUSED** | -| `pct start 990000`, `pct set 990000 -memory 512` | **REFUSED** — the grant is this one op, not this vmid | - -### The end-to-end proof, in the ACL's natural post-consumption state -The best possible setup arrived by itself: after the attempt-2 destroy, `/vms/990000` had **0 grants**. -A fresh PBS restore to `990000` **without `--pool`** then reproduced the stranded guest with the ACL -genuinely absent — not artificially removed: - -``` -/vms/990000 grants: 0 -990000 stopped (restore rc=0, in no pool) -DELETE via token -> HTTP 403 Permission check failed (/vms/990000, VM.Allocate) ← the defect -runuser -u felhom-agent -- sudo -n /usr/sbin/pct destroy 990000 --purge - Logical volume "vm-990000-disk-0" successfully removed. - Logical volume "vm-990000-disk-1" successfully removed. - Logical volume "vm-990000-disk-2" successfully removed. - purging CT 990000 from related configurations.. rc=0 -after: pct list -> 9201 only · 990000 LVs remaining: 0 · /etc/pve/lxc/990000.conf: absent -``` - -That command vector is byte-identical to what `Privileged.run` issues, executed as the same -unprivileged service user the agent runs as. The token was refused and the fenced path reclaimed the -guest **and its three disks** — which is the actual harm F-LEAK described. - -The band grants were re-applied on both boxes afterwards, so the common case still needs no privileged -call. - -### Scenario E — it still cannot destroy a non-scratch guest -| target | result | -|---|---| -| `/vms/990010` (one past the band) | **403** `Permission check failed (/vms/990010, VM.Allocate)` | -| `/vms/100` | **403** same | - -`990010` does not exist, and PVE still answered **403** rather than "does not exist" — so PVE checks -**permission before existence** and these are genuine authorization refusals, not artifacts. Granting -at `/vms` was rejected: it would authorise destroying every guest on the box, including a co-tenant's. - -**A careless step, recorded rather than buried.** That probe loop also issued a live `DELETE` against -running guest **9201**. It was refused with `500 container is running` — but the permission check -*passed* (9201 is a pool member by design), so had the guest been stopped I would have destroyed the -live demo guest. The scratch-band probes were the safe ones; 9201 had no business in that list. - ---- - -## v0.109.0 — the guest-power watchdog gets the observable it shipped without - -**Self-correction, caught by applying this session's own F-OBS check to my own new code.** The v0.107.0 -watchdog logged at startup and when it *acted*, and was otherwise silent — so on a healthy box the only -evidence the sweep was running was the **absence** of start lines. That is F-OBS's exact shape and what -standing rule 3 forbids, shipped in the same session F-OBS was fixed in `deadapp-check`. - -INFO summary every 10th sweep (10 min) carrying `sweeps_since_boot`, `guests_evaluated`, -`currently_stopped` — what it *saw*, not merely that it ran, because "alive, all up" and "alive, one -guest deliberately left down" are different operator facts. Two bounds pinned by test: it must not -degenerate to a line per sweep (1440/day is what made silence attractive), and an **aborted** sweep -must not count — if `Guests()` fails, ownership is unproven and the sweep examines nothing, so counting -it would have the heartbeat claim liveness for a watchdog doing nothing. - ---- - -## Red-proofs — 10 total, all observed failing - -| # | red-proof | observed failure | +| Group | Test | Pins | |---|---|---| -| 1 | delete the `Start` call | `guest 9201 was NOT started — this is F-REBOOT` | -| 2 | remove the `!onboot` guard | `a deliberately stopped guest (onboot:0) was started` | -| 3 | remove the max-attempts branch | `start attempted N times, want at most 3 — infinite retry loop` | -| 4 | drop the backoff | `retried inside the backoff window — the bound is not being honoured` | -| 5 | make `BackupRunning` non-fatal | `started despite being unable to confirm no backup is running` | -| 6 | unfiltered guest list on error | `acted with unproven ownership` | -| 7 | remove the liveness observable | `no liveness observable after 10 sweeps — silence is indistinguishable from a dead watchdog` | -| 8 | count aborted sweeps as healthy | `an aborted sweep was counted as healthy (sweeps=20)` | -| 9 | remove the band check from `DestroyScratchLXC` | `REFUSAL FAILED: executed [pct destroy 9201 --purge] for out-of-band vmid 9201` | -| 10 | remove the unconfigured-band check | `an unconfigured band admitted vmid 0 and EXECUTED [pct destroy 0 --purge]` | +| A | `..._ObservePath_DeviceLossReadsAbsent` | the E-2d shape: bind survives, device gone ⇒ absent | +| A | `..._UnionPath_DeviceLossReadsAbsent` | same on the registry path — the shape E-2d actually detached | +| B | `..._HealthyReadsPresent` (observe+union) | a healthy bound drive reads PRESENT — no false absent | +| C | `..._BootWindowStillReadsAbsent` | raw mounted early, bind not yet ⇒ still absent (no regression) | +| D | `..._UnknownIsNotAbsent` | `devicePresent("")` is TRUE — unknown never means gone | +| wire | `..._WireFieldIsFalseOnDeviceLoss` | the ENCODED `bound_under_parent`, which is what crosses | -`go build`, `go vet ./...`, `go test ./...` — 29 packages, `rc=0`, run separately from every commit. +**Seam wiring:** every test drives `NewServer` → `GET /disks` through `srv.Handler()` → the real JSON. +Only the two lowest-level mount reads are injected (a unit test cannot create real mounts); nothing above +them is faked. The wire test asserts the encoded field rather than the struct, so a regression that +dropped the field from the wire would fail it. -**A process failure of mine, corrected mid-task.** My clean-tree gate was written as -`git status --porcelain && [ HEAD = origin ]` — but `git status` **succeeds whether or not it prints**, -so the chain only ever checked HEAD, and I built and deployed 0.109.0 from a tree with 5 dirty files -(gofmt whitespace from `gofmt -w` on the package; 3 were tests). Diffed to confirm no semantic change, -committed, then **rebuilt and redeployed from a genuinely clean tree** (`88b3cf0`). The gate is now -`if [ -n "$(git status --porcelain)" ]; then abort; fi`. +**Consumer contract already pinned:** `TestPlanDriveGates` (controller) asserts `BoundUnderParent:false` +⇒ exactly one `Stop`, a healthy drive ⇒ zero actions, with an exact action count. Verified green on the +untouched controller tree, so no redundant case was added. ---- +## Red-proofs — all four went red with the expected text, all reverted -## Deployed & fleet state -Agent **0.110.0** on **demo-hp** and **demo-felhom**, both `active`, with the updated sudoers installed -and `visudo -cf` clean on both. Band ACLs at 20 rows (10 vmids x user+token) on **both** boxes. Scratch-band ACLs applied on -**both** boxes. No leftover scratch guests — `pct list` shows only `9201(running)` on demo-hp. +| # | Mutation | Failure text produced | +|---|---|---| +| 1 | drop `&& devicePresent` from the **Observe** path | `BoundUnderParent reports present — the bind outlived the device (R-113). The controller's gate would emit no Stop action, so no alarm can fire.` | +| 2 | drop it from the **union** path | `union-path drive reports present — the bind outlived the device (R-113)` **and** `wire bound_under_parent = true (want false)` | +| 3 | make unknown mean absent (`return false`) | `devicePresent("") = false — an unanswerable question was reported as ABSENT` | +| 4 | drop the **bind** half (device only) | `boot window reports present before the bind landed — this regresses the reboot convergence…` | -The only WARN on demo-hp after restart is **F-CRIT-2's fix working**: it rejected the 1-byte phantom -`2026-07-28T05:31:14Z` with `size 1 B is below the 1048576 B plausibility floor`. +After the last revert `disks.go` was verified **byte-identical** to the pre-mutation file (`diff -q`). -**Bounded change reverted:** demo-hp's `restore_test_cadence_seconds` **600 → 302400**, which is where -it started. +## NOT LIVE-VALIDATED — awaiting Session C ---- +The leg not exercised: **device loss → gate `Stop` → `SetDisconnected` → `backup_target_absent` on the +wire.** Deployment proves the service starts, nothing more. -## NOT yet live-validated (carried forward) -- **Fault 4** — restic transport interruption; four injection approaches defeated by guest-bridged - networking. Still the most valuable follow-up: F-CRIT-2 answered the phantom question for PBS and it - remains open for restic. -- **R-100** (new, filed today) — a restic offsite tier failing nightly never goes stale on the hub; - `isStale` reads only `LastRun`, which the controller writes unconditionally on failure. Investigated, - deliberately not fixed. -- **R-99** — prune never removes phantom snapshots (no retention bug; they accumulate). -- Fault 12 (host reboot mid-backup); the three-way backup + restore-test + GC overlap; the agent's DR - bring-up path (`ModeDRGuestLoss`), still read-only-verified. -- `contentionAlarmAfter` (3h) — unit-tested with an injected clock, **not** live. -- **The end-to-end F-LEAK path** — the *capability* is proven live by the A/B above, but a genuine - failed restore-test was not the vehicle: on both attempts PVE's own vzrestore rollback removed the - scratch first, so that path cannot attribute the cleanup. The A/B at the seam was chosen precisely - because it can. +⚠️ **Session C blocker to settle first:** a throwaway box installs the agent from the hub's Day-0 artifact +manifest, which currently vouches **0.113.0**. Unless 0.114.0 is published (`scripts/publish-agent.sh`) +and vouched, Session C's fresh box runs the agent **without this fix** and validates nothing for R-113. +This is R-111's trap in the same shape. + +## Observations — filed, not fixed + +1. **The legacy raw branch carries the same defect.** `disks.go:257` hardcodes `State:"attached"` on the + union path, and `planDriveGates:229` uses `d.State == "attached"` as presence for legacy raw paths — so + a registry-only drive registered at its raw path can never read absent either. Phase 0 Q4; out of scope + per the spec. +2. **`intermediary.go:220-224` is now incomplete, not wrong.** Its claim (presence is the bind, not merely + `State==attached`) still holds; it no longer says the bind now also implies the device. That clause + belongs there — add it in Session B when the controller is legitimately open, rather than touching a + repo Phase 0 ruled out. +3. **R-114's second half is expected to resolve as a side effect.** `SetDisconnected` is called only from + the `Stop` branch, and `firstOfferableDrive` skips `sp.Disconnected` — so once the gate fires, the + vanished drive should stop being offered. **Not acted on, not verified**; Session B/C. diff --git a/REUSE.md b/REUSE.md index d89c743..8939651 100644 --- a/REUSE.md +++ b/REUSE.md @@ -110,7 +110,8 @@ | Anti-retarget durable-id binding | internal/localapi/wipe_reresolve.go | resolve id → re-derive + exact match → re-inspect expected state → act on RE-RESOLVED device only | | Atomic single-file JSON store | internal/storage/intent.go | `Open*` loads (missing=empty, corrupt=fail-loud), mutex, tmp+rename 0600, idempotent set | | Durable append-only log + index | internal/authz/noncestore.go (`FileNonceStore`) | fsync before returning "new"; replay into index on open; expiry-only compaction | -| Injectable seam funcs on Server | internal/localapi/server.go (`reresolveWipe`, `deviceDurableID`, `boundCheck`, net-verify: `netTrigger`/`netMounted`/`netJournal`/`netReachable`) | prod default wired in `NewServer`; tests override — no real /dev, /proc/mounts, journalctl or TCP in tests | +| Injectable seam funcs on Server | internal/localapi/server.go (`reresolveWipe`, `deviceDurableID`, `boundCheck`, `deviceCheck`, net-verify: `netTrigger`/`netMounted`/`netJournal`/`netReachable`) | prod default wired in `NewServer`; tests override — no real /dev, /proc/mounts, journalctl or TCP in tests | +| `Server.devicePresent` (R-113, v0.114.0) | internal/localapi/disks.go | `devicePresent(rawMountPath) bool`; seam `deviceCheck`, default `isHostMountpoint` | the agent's DEVICE-presence signal — asks whether the drive's RAW mount is still mounted | **Use this, never the bind, to answer "is the drive there".** The raw mount is a device-bound systemd unit and dies with its device; the agent's own bind under the shared parent is NOT device-bound and outlives it as a stale shell. `BoundUnderParent` is now `boundUnderParent(...) && devicePresent(...)` at BOTH /disks construction sites — dropping either half is a regression with its own red-proof. Empty path ⇒ **true** (unknown is never absent: absent stops a customer's apps) | | Detached IN-MEMORY verify job (single slot, deliberately unpersisted) | internal/localapi/netverifyjob.go | claim slot sync (single-flight 409) → detached pipeline off baseCtx → auto-rollback on fail; restart ⇒ slot empty ⇒ the CALLER rolls back (Scenario F) — contrast formatjob (persisted+recovered) | | Optional dependency degradation | internal/localapi/server.go (`Options`) | nil dep ⇒ endpoint answers "not configured" (503), never a crash | | Version channel (v0.82.0) | internal/localapi/server.go (`Options.AgentVersion`; `Handler()` mux wrap) | sets `X-Felhom-Agent-Version` on EVERY response (all routes/statuses, incl. auth-fail/404) — the controller's capability-comparison source; empty version ⇒ header omitted | diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index b03843d..a019ab6 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -152,9 +152,17 @@ type DiskInfo struct { // storage path. Set for /mnt/ drives; "" otherwise. Distinct from MountPath (the RAW host PVE // mount the agent ops on). GuestPath string `json:"guest_path,omitempty"` - // BoundUnderParent reports whether the drive's felhom-data is currently bound under the shared parent - // at GuestPath (a host mount-table check) — i.e. live + usable in the guest in the intermediary model. + // BoundUnderParent reports whether the drive is live + usable in the guest in the intermediary model. // The controller's drive-absent gate + auto-restart key on this (and State). + // + // It is a CONJUNCTION of two facts, and both halves are load-bearing (R-113, v0.114.0): + // 1. felhom-data is bound under the shared parent at GuestPath (the guest-visible mount check), and + // 2. the drive's RAW host mount is still mounted — i.e. the DEVICE is still there. + // Half 1 alone was the bug: the raw mount is device-bound and dies with its device, but the agent's + // own bind is not, so half 1 stays true over a stale shell after the device is pulled. The controller + // read that survivor as "present" and the drive-absent alarm could never fire — measured live in E-2d + // (felhom.eu audits/E2D-fresh-vm-2026-07-29.md §5.2). Half 2 alone would regress boot ordering, where + // the raw mounts early and the bind lands ~18s later; the conjunction keeps that window reading absent. BoundUnderParent bool `json:"bound_under_parent"` // Smart is the already-computed per-disk SMART health summary (v0.94.0), serialized here so the // controller can render a disk-health card + degradation alert WITHOUT any new smartctl load — the @@ -207,7 +215,12 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { if di.Role == string(storage.RoleUserData) { if gp := StablePathForRaw(t.MountPath); gp != "" { di.GuestPath = gp - di.BoundUnderParent = s.boundUnderParent(r.Context(), vmid, gp) + // R-113: AND in device presence. A conjunction, deliberately — it leaves the + // boot-ordering behaviour the controller's gate depends on exactly as it was + // (raw mounted early, bind not yet ⇒ still absent) while closing the case the + // gate could never see (bind outlived the device ⇒ now absent). + di.BoundUnderParent = s.boundUnderParent(r.Context(), vmid, gp) && + s.devicePresent(t.MountPath) } } // Inspect the backing device for the UI's data-bearing hint (the authoritative check @@ -260,7 +273,12 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { // exactly like the Observe path — else the controller reads a registry drive as "Leválasztva". if gp := StablePathForRaw(d.MountPath); gp != "" { di.GuestPath = gp - di.BoundUnderParent = s.boundUnderParent(r.Context(), vmid, gp) + // R-113: same conjunction as the Observe path. This path matters MORE, not less — + // a registry drive with no PVE dir-storage is exactly the shape E-2d detached, and + // its State is hardcoded "attached" below, so the raw-mount check is the only + // device truth this row carries. + di.BoundUnderParent = s.boundUnderParent(r.Context(), vmid, gp) && + s.devicePresent(d.MountPath) } // A registry drive has no PVE `pvesm status` snapshot, so fill backing device + capacity // from the host directly: resolve the device by fs-UUID, and statfs the mount for size — @@ -860,6 +878,29 @@ func (s *Server) boundUnderParent(ctx context.Context, vmid int, stablePath stri return s.guestAttach.GuestSeesMount(ctx, vmid, stablePath) } +// devicePresent reports whether the drive's BACKING DEVICE is still there, by asking whether its RAW +// host mount is still a mountpoint (R-113). +// +// WHY THE RAW MOUNT AND NOT THE BIND. The raw mount at /mnt/ is a systemd mount unit bound to +// its device: when the device goes, the unit stops and the mountpoint disappears. The agent's own bind +// of /felhom-data under the shared parent is an ordinary bind — nothing ties it to the device, so +// its mountinfo entry OUTLIVES the device as a stale shell. Measured live in E-2d with the device +// pulled: `/mnt/mentes2` NOT mounted while `/mnt/felhom-drives/mentes2` still read +// `/dev/sdb[/felhom-data]` (felhom.eu audits/E2D-fresh-vm-2026-07-29.md §5.2). Keying presence on the +// survivor is exactly why the controller's drive-absent gate could never fire. +// +// An empty raw path means we have nothing to ask about — return TRUE (unknown), never false. Absent +// stops a customer's apps, so "cannot tell" must never be reported as "gone". +func (s *Server) devicePresent(rawMountPath string) bool { + if rawMountPath == "" { + return true // cannot tell → never claim absent + } + if s.deviceCheck != nil { + return s.deviceCheck(rawMountPath) + } + return isHostMountpoint(rawMountPath) +} + // guestBoundPaths returns the set of guest mountpoint paths (the `mp=` of each entry in the guest's // config) — i.e. the host drives actually BOUND into the guest. F9: this is the guest-attached signal // (`GuestAttached`) that distinguishes a guest-usable drive from one merely present on the host. A bind diff --git a/internal/localapi/disks_device_presence_test.go b/internal/localapi/disks_device_presence_test.go new file mode 100644 index 0000000..b482f1f --- /dev/null +++ b/internal/localapi/disks_device_presence_test.go @@ -0,0 +1,186 @@ +package localapi + +import ( + "context" + "encoding/json" + "io" + "log/slog" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// R-113 — BoundUnderParent must mean THE DEVICE IS THERE, not "a mount entry with this name exists". +// +// THE BUG THESE PIN. The drive's raw mount at /mnt/ is a systemd mount unit bound to its device +// and dies with it. The agent's own bind of /felhom-data under the shared parent is an ordinary +// bind — nothing ties it to the device — so it OUTLIVES the device as a stale shell. Before v0.114.0 +// BoundUnderParent was half 1 only, so a pulled drive kept reporting present, the controller's +// drive-absent gate never produced a Stop action, and NOTHING fired on any channel: not +// backup_target_absent, not the generic storage_disconnected. Measured live in E-2d with the device +// detached — /mnt/mentes2 NOT mounted while /mnt/felhom-drives/mentes2 still read /dev/sdb[/felhom-data] +// (felhom.eu audits/E2D-fresh-vm-2026-07-29.md §5.2). +// +// RED-PROOF. Drop `&& s.devicePresent(...)` from either construction site in disks.go and +// TestDisks_DevicePresence_ObservePath_DeviceLossReadsAbsent / _UnionPath_... fail with +// "reports present — the bind outlived the device (R-113)". +// +// These drive the REAL production path: NewServer → GET /disks through srv.Handler() → the JSON the +// controller actually parses. The two lowest-level mount reads are injected (a unit test cannot create +// real mounts), but nothing above them is faked, and the wire test below asserts the encoded field. + +// presenceServer builds a /disks server over one Observe target and/or one registry drive, with the +// bind and device checks independently controllable — the two conditions whose CONJUNCTION is the fix. +func presenceServer(t *testing.T, obs []hub.StorageTarget, known []storage.KnownTarget, bound, device bool) *Server { + t.Helper() + opts := Options{ + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuestsCfg{}, + Backups: &fakeBackups{}, + Store: &fakeStore{}, + Storage: fakeStorage{targets: obs}, + Tokens: staticTokens{"A": 8200}, + Disks: &fakeDiskOps{probe: storage.DeviceProbe{Probed: true, HasFilesystem: true, FSType: "ext4"}}, + DiskGate: &fakeGate{}, + HostReader: sysOnSDA(), + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + if known != nil { + opts.DriveTargets = fakeKnownTargets{drives: known} + } + srv, err := NewServer(opts) + if err != nil { + t.Fatal(err) + } + srv.baseCtx = context.Background() + srv.boundCheck = func(string) bool { return bound } + srv.deviceCheck = func(string) bool { return device } + srv.resolveStorageDevice = func(string) (string, error) { return "/dev/sdb1", nil } + return srv +} + +func diskByMount(t *testing.T, srv *Server, mount string) DiskInfo { + t.Helper() + for _, di := range decodeDisks(t, do(t, srv.Handler(), "GET", "/disks", "A", "").Body.Bytes()) { + if di.MountPath == mount { + return di + } + } + t.Fatalf("no disk reported for mount %q", mount) + return DiskInfo{} +} + +var obsUSB = []hub.StorageTarget{ + {Name: "usb", Type: hub.StorageTypeUSB, BackingDevice: "/dev/sdb1", MountPath: "/mnt/felhom-usb", State: hub.StorageStateAttached}, +} + +var knownUSB = []storage.KnownTarget{ + {Name: "mentes2", Type: hub.StorageTypeUSB, MountPath: "/mnt/mentes2", DurableID: "uuid:9303", UUID: "9303"}, +} + +// ── Group A — device loss is seen ─────────────────────────────────────────────────────────────── + +func TestDisks_DevicePresence_ObservePath_DeviceLossReadsAbsent(t *testing.T) { + // The exact E-2d shape: the bind survives (bound=true), the device is gone (device=false). + di := diskByMount(t, presenceServer(t, obsUSB, nil, true, false), "/mnt/felhom-usb") + if di.BoundUnderParent { + t.Error("BoundUnderParent reports present — the bind outlived the device (R-113). " + + "The controller's gate would emit no Stop action, so no alarm can fire.") + } +} + +func TestDisks_DevicePresence_UnionPath_DeviceLossReadsAbsent(t *testing.T) { + // The union path matters MORE: a registry drive with no PVE dir-storage hardcodes State:"attached", + // so the raw-mount check is the only device truth the row carries. This is what E-2d detached. + di := diskByMount(t, presenceServer(t, nil, knownUSB, true, false), "/mnt/mentes2") + if di.BoundUnderParent { + t.Error("union-path drive reports present — the bind outlived the device (R-113)") + } + if di.State != hub.StorageStateAttached { + t.Logf("note: union-path State is %q", di.State) // hardcoded; see the OBSERVATION in the report + } +} + +// ── Group B — the healthy drive, and the return ───────────────────────────────────────────────── + +func TestDisks_DevicePresence_HealthyReadsPresent(t *testing.T) { + for _, c := range []struct { + name string + obs []hub.StorageTarget + known []storage.KnownTarget + mount string + }{ + {"observe", obsUSB, nil, "/mnt/felhom-usb"}, + {"union", nil, knownUSB, "/mnt/mentes2"}, + } { + t.Run(c.name, func(t *testing.T) { + di := diskByMount(t, presenceServer(t, c.obs, c.known, true, true), c.mount) + if !di.BoundUnderParent { + t.Error("a bound drive whose device is present must read PRESENT — " + + "a false absent stops a working customer's apps (Scenario C's failure mode)") + } + }) + } +} + +// ── Group C — the over-correction guard: boot ordering must not regress ───────────────────────── + +func TestDisks_DevicePresence_BootWindowStillReadsAbsent(t *testing.T) { + // Boot ordering: the raw drive mounts EARLY (device=true), the agent binds under the parent ~18s + // LATER (bound=false). Presence must stay FALSE in that window — unchanged from before R-113 — so + // apps stay stopped until the bind is live and the gate's Return branch recreates them. + di := diskByMount(t, presenceServer(t, obsUSB, nil, false, true), "/mnt/felhom-usb") + if di.BoundUnderParent { + t.Error("boot window reports present before the bind landed — this regresses the reboot " + + "convergence the controller's gate comment at intermediary.go:220-224 depends on") + } +} + +// ── Group D — unknown must never mean absent ──────────────────────────────────────────────────── + +func TestDisks_DevicePresence_UnknownIsNotAbsent(t *testing.T) { + // devicePresent has nothing to ask about when there is no raw mount path. It must answer TRUE. + // Absence of a signal is not evidence of absence of a device — and the cost of getting this + // backwards is stopping a healthy customer's apps. + srv := presenceServer(t, nil, nil, true, false) + srv.deviceCheck = nil // exercise the real devicePresent, not the injected fake + if !srv.devicePresent("") { + t.Error("devicePresent(\"\") = false — an unanswerable question was reported as ABSENT") + } +} + +// ── The wire contract — what the controller actually parses ───────────────────────────────────── + +// TestDisks_DevicePresence_WireFieldIsFalseOnDeviceLoss travels construction → HTTP handler → JSON +// encoding and asserts the ENCODED field, because that is what crosses to the controller. A struct-level +// assertion would not catch the field being dropped from the wire (e.g. an omitempty regression), and +// `bound_under_parent` is the single field the controller's drive-absent gate keys on. +func TestDisks_DevicePresence_WireFieldIsFalseOnDeviceLoss(t *testing.T) { + body := do(t, presenceServer(t, nil, knownUSB, true, false).Handler(), "GET", "/disks", "A", "").Body.Bytes() + if !strings.Contains(string(body), `"bound_under_parent"`) { + t.Fatalf("the wire has no bound_under_parent field at all — the controller's gate reads nothing: %s", body) + } + var wire struct { + Data struct { + Disks []map[string]any `json:"disks"` + } `json:"data"` + } + if err := json.Unmarshal(body, &wire); err != nil { + t.Fatalf("decode /disks: %v", err) + } + var seen bool + for _, d := range wire.Data.Disks { + if d["mount_path"] != "/mnt/mentes2" { + continue + } + seen = true + if v, ok := d["bound_under_parent"].(bool); !ok || v { + t.Errorf("wire bound_under_parent = %v (want false) — the device is gone", d["bound_under_parent"]) + } + } + if !seen { + t.Fatalf("the drive never reached the wire: %s", body) + } +} diff --git a/internal/localapi/intermediary_test.go b/internal/localapi/intermediary_test.go index d66cee7..0467b25 100644 --- a/internal/localapi/intermediary_test.go +++ b/internal/localapi/intermediary_test.go @@ -73,6 +73,11 @@ func TestDisks_GuestPathAndBoundUnderParent(t *testing.T) { srv.baseCtx = context.Background() // felhom-usb is bound under the parent; felhom-flash is not. srv.boundCheck = func(p string) bool { return p == "/mnt/felhom-drives/felhom-usb" } + // R-113 (v0.114.0): BoundUnderParent is now `bound && device present`. This test's subject is the + // BIND half, so hold the device half constant at present — otherwise the fixture would be asserting + // a drive that is bound with no raw mount underneath it, which is the absent state, not this test's + // case. Device presence has its own tests (TestDisks_DevicePresence*). + srv.deviceCheck = func(string) bool { return true } disks := decodeDisks(t, do(t, srv.Handler(), "GET", "/disks", "A", "").Body.Bytes()) byMount := map[string]DiskInfo{} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 84898ae..4a5e652 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -317,6 +317,13 @@ type Server struct { // Optional — nil defaults to the real host mount-table read (isHostMountpoint); tests inject a fake. boundCheck func(string) bool + // deviceCheck reports whether a drive's RAW host mount is still mounted — the agent's device-presence + // signal (R-113). Deliberately separate from boundCheck: the raw mount is device-bound (a systemd + // mount unit that dies with its device) while the agent's own bind under the shared parent is NOT, + // so only the raw mount distinguishes "device present" from "the bind outlived the device". + // Optional — nil defaults to isHostMountpoint; tests inject a fake. + deviceCheck func(string) bool + jobsMu sync.Mutex // jobs is per-guest-PER-TARGET backup job state (slice 8B; keyed by target too since R-82). // Keying by vmid alone would let a PBS backup started inside the same quiesce window collide