246 lines
13 KiB
Markdown
246 lines
13 KiB
Markdown
# REPORT — green-gate restoration + F7 export-page fix + remediation-arc cleanup
|
||
|
||
**Date:** 2026-07-20 · **Repo:** felhom-controller (v0.149.0 → **v0.150.0**) · Trunk, pushed to `main`.
|
||
**Origin:** the three open hygiene items from the 2026-07-20 remediation arc (REPORT §Green gate,
|
||
§Observations) + R-53.
|
||
|
||
---
|
||
|
||
## 1. Baselines used
|
||
|
||
| Repo | Baseline @ start | Gate | After |
|
||
|------|------------------|------|-------|
|
||
| felhom-controller | `4646be1` | `HEAD == origin/main`, tree clean ✔ | `9f436c8` (code), then this docs commit |
|
||
| felhom.eu | `30fd9d3` | `HEAD == origin/main` ✔ | `1e11819` (ROADMAP) |
|
||
|
||
Deployed controller **before**: `felhom-controller:0.149.0` (Up, healthy) — matched the baseline.
|
||
Deployed controller **after**: `felhom-controller:0.150.0` (Up 6 seconds, healthy).
|
||
|
||
---
|
||
|
||
## 2. Part 1 — the 7 red `internal/backup` tests
|
||
|
||
### 2.1 Classification (1.1) — one class, not several
|
||
|
||
The v0.149.0 REPORT's device-identity hypothesis is **confirmed**, and the code said so in Hungarian
|
||
without anyone having to infer it. `TestSharesTier2MirrorsBothClasses` failed with:
|
||
|
||
```
|
||
LastStatus:no_target
|
||
LastError:/tmp/…/hdd_1: nincs másik fizikai meghajtó — a 2. mentéshez 2. meghajtó szükséges
|
||
```
|
||
|
||
Tier-2's whole purpose is to refuse a target on the source disk; it asks
|
||
`system.SamePhysicalDevice` → `isSameBlockDevice` → `statA.Dev == statB.Dev`
|
||
(`internal/system/mounts_linux.go:225`). On DooPlex every `t.TempDir()` lands on one filesystem, so a
|
||
fixture's `hdd_1` and `hdd_2` share an `st_dev`, the guard correctly refuses, and the test can never
|
||
reach the behaviour it exists to check. **The tests were right and the code was right — the
|
||
environment could not express the fixture's premise.**
|
||
|
||
| Test | Failing assertion | Root-cause class |
|
||
|---|---|---|
|
||
| `TestSharesTier2MirrorsBothClasses` | shares not mirrored; payload + marker missing; `LastStatus != ok` | device-identity |
|
||
| `TestSharesTier2SkipsDeadMountAndContinues` | "the healthy share must still be mirrored" | device-identity |
|
||
| `TestSharesTier2ReconcilePrunesRemovedShare` | precondition: dest dir absent after first run | device-identity |
|
||
| `TestSharesTier2NotifierNeverLeaksReservedKey` | "precondition: the notifier should have fired" | device-identity |
|
||
| `TestTier2V2_MigrationAndMarkerLast` | flat `appdata/` not removed; legs not mirrored; no marker | device-identity |
|
||
| `TestTier2V2_Reconcile` | stale sibling / unrelated dirs not reconciled away | device-identity |
|
||
| `TestTier2V2_NetworkExclusion` | picked `…/sys/felhom-data` instead of the local drive | device-identity |
|
||
|
||
**No test failed for a non-environmental reason** — no real defect was found hiding behind the red,
|
||
so there is no §1.1 STOP finding.
|
||
|
||
### 2.2 Fix — tool 2 (minimal seam), once, for all seven
|
||
|
||
Tool 1 (test-only) was considered and rejected: the only way to get two genuinely distinct devices
|
||
on this host is to place one fixture drive on `/mnt/5_hdd` and the other on `/tmp`, which hard-codes
|
||
DooPlex's disk layout into the tests — non-hermetic, and it would re-break on any other machine. Tool
|
||
3 (`t.Skip`) would have hidden seven real assertions behind an environment excuse.
|
||
|
||
**The seam** (`internal/backup/backup.go`) — one field + one wrapper, modelled on the package's
|
||
existing `tier2Mirror` / `tier2SSDFits` / `sharesPassdbCapture` nil-defaulted field seams:
|
||
|
||
```go
|
||
samePhysicalDevice func(a, b string) bool // nil → system.SamePhysicalDevice
|
||
|
||
func (m *Manager) sameDevice(a, b string) bool {
|
||
if m.samePhysicalDevice != nil { return m.samePhysicalDevice(a, b) }
|
||
return system.SamePhysicalDevice(a, b)
|
||
}
|
||
```
|
||
|
||
**Seam justification (one line, per §9.4):** the off-drive predicate is the single environmental fact
|
||
the tier-2 tests cannot supply on a one-filesystem host; routing it through a nil-defaulted field
|
||
makes it injectable while leaving production on the identical `st_dev` call.
|
||
|
||
Nil-check lives in the wrapper rather than in `NewManager` deliberately: `newSharesEnv` builds
|
||
`&Manager{…}` by literal, so a constructor-only default would leave the field nil and panic.
|
||
|
||
Seven call sites now go through `m.sameDevice(...)`: `tier2.go` ×5 (`selectTier2TargetFrom` ×3,
|
||
`RunTier2`, `Tier2Info`), `tier2_shares.go` ×1 (`RunSharesTier2`), `backup.go` ×1
|
||
(`hasOffDriveTarget`). Behaviour with a nil seam is byte-for-byte the previous code.
|
||
|
||
**Test-side** (`device_seam_test.go`, new): `oneDrivePerSubtree` models one drive per directory
|
||
subtree — two paths share a device only when one contains the other, which is exactly how real
|
||
mountpoints behave. Installed in the two fixtures only (`newTier2V2`, `newSharesEnv`).
|
||
|
||
Nothing was weakened: the guard still runs and still refuses same-device targets — the
|
||
same-drive refusal test `TestSharesTier2NeverTargetsItsOwnSourceDrive` passes under the seam, and it
|
||
is the test that would catch a fake that simply said "always different".
|
||
|
||
**Surviving `t.Skip`s: none.** No test was skipped, deleted, renamed, or had an expected value
|
||
changed.
|
||
|
||
### 2.3 Scenario B — mutation table
|
||
|
||
Every one of the seven had the defect it guards re-introduced in **production** code, one at a time,
|
||
then reverted:
|
||
|
||
| Test | Mutation | Result |
|
||
|---|---|---|
|
||
| `TestSharesTier2MirrorsBothClasses` | skip the payload mirror to the target | **FAIL** ✔ |
|
||
| `TestSharesTier2SkipsDeadMountAndContinues` | stop skipping `Disconnected/Decommissioned` drives | **FAIL** ✔ (`dead-mount share reached a mirror call`) |
|
||
| `TestSharesTier2ReconcilePrunesRemovedShare` | `tier2ReconcileRoots` → no-op | **FAIL** ✔ |
|
||
| `TestSharesTier2NotifierNeverLeaksReservedKey` | pass `SharesPseudoStack` raw on the success path | **FAIL** ✔ (`the reserved key reached the notification boundary raw: "_shares"`) |
|
||
| `TestTier2V2_MigrationAndMarkerLast` | skip the old-flat-`appdata/` migration cleanup | **FAIL** ✔ |
|
||
| `TestTier2V2_Reconcile` | `tier2ReconcileRoots` → no-op | **FAIL** ✔ |
|
||
| `TestTier2V2_NetworkExclusion` | disable the auto `sp.IsNetwork()` skip | **FAIL** ✔ |
|
||
|
||
Honest note on method: my first attempt at two of these mutated the wrong code path (the
|
||
mirror-failure notify branch instead of the success one; the unregistered-root skip instead of the
|
||
disconnected-drive one) and both tests stayed green. That was a bad mutation, not a hollow test — the
|
||
corrected mutations bite, and the notifier one reproduces that test's own documented red-proof
|
||
verbatim. Tree verified free of mutation residue afterwards (`grep -c "// MUT:"` → 0).
|
||
|
||
### 2.4 Scenario A — green gate, twice
|
||
|
||
| Run | Command | Result |
|
||
|---|---|---|
|
||
| build / vet | `go build ./...`, `go vet ./...` | **OK** |
|
||
| 1 | `go test ./... -count=1` | **exit 0 — 23 packages ok, 0 FAIL** |
|
||
| 2 | `go test ./... -count=1` | **exit 0 — 23 packages ok, 0 FAIL** |
|
||
|
||
`grep -c SKIP` over run 2 → **0**. The gate is genuinely green, not green-by-omission.
|
||
|
||
---
|
||
|
||
## 3. Part 2 — F7 / R-53, the export page
|
||
|
||
`app_export.html` L93 read `var domain = '{{.Stack.Meta.Subdomain}}' ? '{{…}}.{{$.CSRFToken}}' : '';`
|
||
— the session CSRF token where the customer domain belongs.
|
||
|
||
**A spec premise turned out to be wrong, and the fix needed one more line than planned.** §5 states
|
||
`{{$.Domain}}` is "set by `baseData`". It is (`handlers.go:120`) — but `exportPageHandler`
|
||
(`handler_export.go`) never calls `baseData`; it builds `{"Stack", "Drives"}` and `executeTemplate`
|
||
injects only `CSRFField` / `CSRFToken`. Swapping the token alone would have rendered an empty
|
||
domain — trading a wrong link for a broken one. So the fix is two lines:
|
||
|
||
- template: `{{$.CSRFToken}}` → `{{$.Domain}}`;
|
||
- handler: `"Domain": s.cfg.Customer.Domain` added to the map, mirroring `baseData`'s own line.
|
||
|
||
`csrfH()` and the meta-tag CSRF flow are the correct usage and are untouched.
|
||
|
||
**Tests** (`app_export_domain_test.go`, new — v0.149.0's render harness): the `var domain` line is
|
||
isolated first so an assertion cannot match the token where it legitimately appears (the meta tag).
|
||
|
||
| Scenario | Test | Result |
|
||
|---|---|---|
|
||
| C — joined from the customer domain, token absent | `TestAppExportDomainUsesCustomerDomainNotCSRFToken` | **PASS** |
|
||
| C — empty-subdomain branch still `''` | `TestAppExportDomainEmptyWithoutSubdomain` | **PASS** |
|
||
|
||
**Red-proof:** restored the pre-fix template line → both tests fail, showing the defect verbatim:
|
||
|
||
```
|
||
--- FAIL: TestAppExportDomainUsesCustomerDomainNotCSRFToken
|
||
export link must be built from the customer domain, got:
|
||
var domain = 'photos' ? 'photos.deadbeefcafebabe…' : '';
|
||
the CSRF token must NEVER appear in the export URL, got: … (same line)
|
||
--- FAIL: TestAppExportDomainEmptyWithoutSubdomain
|
||
```
|
||
|
||
Template restored; both green.
|
||
|
||
**Live verification (Scenario C)** — authenticated endpoint fetch of the real export page
|
||
`/stacks/immich/export` (no browser on DooPlex), ASCII-safe grep per the new gotcha:
|
||
|
||
```
|
||
line 237: var domain = 'photos' ? 'photos.demo-felhom.eu' : '';
|
||
meta tag: csrf-token" content="f7f928f1… ← the token lives here, and only here
|
||
```
|
||
|
||
The rendered link is correct and the token appears nowhere in it. Note this also confirms the
|
||
handler-side `Domain` key, which the render tests alone could not prove.
|
||
|
||
---
|
||
|
||
## 4. §3.4 — the orphaned `dhclient`
|
||
|
||
Evidence first. Exactly one match, on an interface that does not exist:
|
||
|
||
```
|
||
BEFORE: 1922 /sbin/dhclient -pf /run/dhclient.eth0.pid -lf /var/lib/dhcp/dhclient.eth0.leases eth0
|
||
PPID 1652, started Mon Jul 20 07:25:30 (boot)
|
||
interfaces: lo enp1s0 wlp2s0 tailscale0 vmbr0 wg-felhom veth9201i0 ← no eth0
|
||
vmbr0: iface vmbr0 inet static / address 192.168.0.162/24 ← no dhclient should run at all
|
||
neither /run/dhclient.eth0.pid nor /var/lib/dhcp/dhclient.eth0.leases existed
|
||
```
|
||
|
||
Killed. Re-checked immediately and again ~25 min later: `pgrep -a dhclient` → **none, no respawn**.
|
||
`vmbr0` still `192.168.0.162/24`, `felhom-agent` still `active`. No other host mutation.
|
||
|
||
---
|
||
|
||
## 5. Files changed
|
||
|
||
| File | Change |
|
||
|------|--------|
|
||
| `internal/backup/backup.go` | seam field + `sameDevice` wrapper; 1 call site routed |
|
||
| `internal/backup/tier2.go` | 5 call sites routed through the seam |
|
||
| `internal/backup/tier2_shares.go` | 1 call site routed; unused `system` import dropped |
|
||
| `internal/backup/device_seam_test.go` | **new** — `oneDrivePerSubtree` |
|
||
| `internal/backup/tier2_v2_test.go`, `shares_test.go` | seam injected in the two fixtures (1 line each) |
|
||
| `internal/web/templates/app_export.html` | 1 token: `{{$.CSRFToken}}` → `{{$.Domain}}` |
|
||
| `internal/web/handler_export.go` | `"Domain"` added to the export page's data map |
|
||
| `internal/web/app_export_domain_test.go` | **new** — 2 tests + 2 helpers |
|
||
| `CLAUDE.md` | 2 gotchas in the live-validation section |
|
||
| `CHANGELOG.md` / `CONTEXT.md` / `REPORT.md` | v0.150.0 bookkeeping |
|
||
|
||
**Commits:** felhom-controller `9f436c8` (code) + this docs commit; felhom.eu `1e11819` (ROADMAP).
|
||
|
||
---
|
||
|
||
## 6. ROADMAP corrections (felhom.eu)
|
||
|
||
- **R-50 factual correction.** The entry claimed an address move needs the cert's SAN set to cover
|
||
the new address. Read against source (`internal/agentapi/client.go` L105–129): the controller→agent
|
||
leg sets `InsecureSkipVerify: true` and replaces chain verification with a `VerifyPeerCertificate`
|
||
that does a raw **SHA-256 match on the leaf DER**. Hostname/SAN never enters verification on this
|
||
leg, so an address move most likely needs **no cert re-issuance** — only the endpoint the guest
|
||
dials. The entry now says so, and still requires the spike to confirm it empirically.
|
||
- **R-53 collapsed** to its one-liner + **SHIPPED (controller v0.150.0)**, noting the handler-side
|
||
`Domain` key as part of the fix.
|
||
|
||
---
|
||
|
||
## 7. Not done / open
|
||
|
||
R-50 (durable F1 — spike-first, not started), R-51, R-52, R-39(b)/F6 — all untouched, as scoped.
|
||
|
||
---
|
||
|
||
## 8. Observations (noticed, not acted on)
|
||
|
||
1. **Four files in `internal/backup` are not `gofmt`-clean at HEAD** — `offbox_3a_test.go`,
|
||
`recovery_unit.go`, `tier2.go`, `tier2_v2_test.go`. I verified this is **pre-existing** (checked
|
||
each file's HEAD blob through `gofmt -l` before my edits) and left them alone: `gofmt` is not part
|
||
of the green gate (`build`/`vet`/`test`), and reformatting them would have buried this task's real
|
||
diff in noise. Worth a one-shot `gofmt -w` pass in its own commit.
|
||
2. **`system.SamePhysicalDevice` is mount/device-granular, not disk-granular** — its own doc comment
|
||
says two partitions on one physical disk look "different" here, with the agent's durable-id as the
|
||
stronger guarantee. Unchanged by this work, but it means the tier-2 off-drive promise is
|
||
partition-level on a box that partitions one disk.
|
||
3. **The tier-2 tests silently stopped exercising their subject rather than failing loudly at the
|
||
premise.** Each failed on a downstream assertion ("share was not mirrored") instead of on "this
|
||
fixture needs two devices and this host has one". A fixture-level precondition check would have
|
||
turned a week of ambiguous red into one clear message.
|