Triage: fix-spec for live-drive findings F1-F20 (re-diagnosed at file:line, sequenced batches)
This commit is contained in:
@@ -0,0 +1,446 @@
|
||||
# LIVE-DRIVE-FIXSPEC — triage of the 2026-06-14 live-drive findings
|
||||
|
||||
- **Date:** 2026-06-14
|
||||
- **Branch:** `audit/2026-06-14-live-drive` (companion to `LIVE-DRIVE-FINDINGS-2026-06-14.md`)
|
||||
- **Scope:** re-diagnose F1–F20 against **current source** at file:line, spec a fix for each worth fixing, then a sequenced plan. **Diagnose + spec only — no code changes, no deploys, no `main` commits.**
|
||||
- **Method:** every finding's stated cause was treated as a claim and re-rooted in code. Two report causes were found **wrong** (F20-BUG1 location; F17 volume-capture sub-claim) and one is likely a measurement artifact (F3). Runtime-only facts reference the findings report's captured evidence.
|
||||
|
||||
## Progress log
|
||||
|
||||
- **t0** — Confirmed branch + repos (`felhom-controller`, `felhom-agent`, `app-catalog-felhom.eu`, `felhom.eu`). Launched parallel code investigations.
|
||||
- **t1** — F17 + F20(agent) re-diagnosed: F17 DB-replay claim TRUE but volume-capture sub-claim mis-attributed; **F20-BUG1 relocated** (agent is correct; bug is in the controller's agentapi client).
|
||||
- **t2** — F9 + F2 re-diagnosed: provisioning never binds user-data drives; "misreporting" is a missing guest-attached field; F2 is the expected symptom.
|
||||
- **t3** — F1/F11/F13 + F5 catalog + controller-misc (F3/F4/F6/F7/F8/F15 + BUG1 controller mapping) re-diagnosed. F1 confirmed to defeat the deploy memory guard (not display-only). F5 catalog survey found additional at-risk healthchecks.
|
||||
- **t4** — Wrote per-finding specs + sequenced batches. (this doc)
|
||||
|
||||
---
|
||||
|
||||
## RE-DIAGNOSIS SUMMARY (verdict counts)
|
||||
|
||||
| Verdict | Count | Findings |
|
||||
|---|---|---|
|
||||
| **real** (fix as specced) | 11 | F1, F5, F9, F11, F17, F20-BUG1, F20-BUG2, F20-BUG3, F6, F7, F8 |
|
||||
| **mis-attributed** (report cause wrong → re-rooted) | 3 | **F20-BUG1** (controller not agent), **F17 sub-claim** (units DO capture volumes), **F3** (not in code) |
|
||||
| **already-fine / no independent action** | 9 | F10, F12, F14, F16, F18, F19 (PASS), **F13** (consequence of F9), **F2** (expected symptom of F9), **F15** (standard Docker semantics) |
|
||||
| **needs-runtime-confirmation** | 2 | **F3** (byte-level recheck), **F17 capture-side** (are volume tars actually written by `backup/run`?) |
|
||||
| **trivial / doc-only** | 1 | F4 |
|
||||
|
||||
> Findings whose **report cause was WRONG against the code** (the most important output): **F20-BUG1** (report blamed the agent's format handler; the agent is correct — the bug is the controller's `agentapi.FormatDisk` swallowing the agent's 502). **F17** (report said "recovery units don't capture volume data"; they do — `restoreDockerVolumes` + `VolumeDumps` exist; the real gap is the `.sql` dump is never replayed). **F3** (report said the JSON API double-encodes; no transcoding exists in the code path — likely a terminal/`curl` display artifact).
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL / HIGH — full specs
|
||||
|
||||
### F17 — Per-app restore re-creates config + restores Docker volumes but never replays the `.sql` DB dump
|
||||
```
|
||||
Report claim: "per-app restore never imports the DB dump; recovery units don't capture volume data"
|
||||
Re-diagnosis: real (DB-replay gap) + mis-attributed (volume-capture sub-claim is FALSE)
|
||||
True cause: POST /backup/restore → web/handlers.go:785 RestoreFromRecoveryUnit →
|
||||
backup/restore_unit.go:125-131 does StopStack + restoreDockerVolumes(:128) +
|
||||
RecreateStackFromUnit(:131). RecreateStackFromUnit (cmd/controller/main.go:953)
|
||||
copies docker-compose.yml/.felhom.yml + RedeployFromEnv (compose up -d). NO .sql
|
||||
replay anywhere in internal/backup/*. The captured romm-mariadb.sql is enumerated
|
||||
into the unit manifest (recovery_unit.go:105) but never piped into the DB.
|
||||
The reusable importer EXISTS but only in the .fab path: appexport/restore.go —
|
||||
importDBDump(:971), findDBServiceInCompose(:816), waitForDB(:928), findDumpFile(:795),
|
||||
restoreDatabase(:722) — all UNEXPORTED, package appexport.
|
||||
Correction to the report: RestoreFromRecoveryUnit DOES restore Docker named-volume
|
||||
tars (restore.go:84-137) and capture DOES populate VolumeDumps (recovery_unit.go:44).
|
||||
So "units don't capture volumes" is wrong; the romm row was lost because (a) the .sql
|
||||
is never replayed AND (b) romm's MariaDB data dir is under the appdata bind namespace
|
||||
(live dir, not a named-volume tar), so the volume-restore path doesn't cover it either.
|
||||
Severity: CRITICAL (data-loss on the headline disaster-recovery feature; matches report)
|
||||
Repo / layer: controller — internal/backup (+ reuse internal/appexport DB-import logic)
|
||||
Fix approach: 1) Extract the four free funcs (importDBDump/findDBServiceInCompose/waitForDB/
|
||||
findDumpFile) into a neutral pkg (e.g. internal/dbrestore) and have BOTH appexport
|
||||
and backup call them (no duplication). 2) In RestoreFromRecoveryUnit, after
|
||||
RecreateStackFromUnit + DB service healthy, for each <unit>/db-dumps/*.sql replay it
|
||||
via the shared importer (bring DB svc up, waitForDB, importDBDump, then continue the
|
||||
stack). 3) Make volume-restore failure NOT silently non-fatal: restore.go:64 currently
|
||||
logs WARN + continues and RestoreApp returns nil regardless — surface a partial-restore
|
||||
error/flag to the caller so the UI can't claim success on a silent data-restore failure.
|
||||
Effort: L (cross-package extraction + restore-flow change + tests)
|
||||
Risk: unattended-safe to WRITE/test (no live deploy); behaviour change is destructive-restore,
|
||||
so validate against a scratch app before shipping.
|
||||
Depends on / blocks: independent. Co-edits restore_unit.go/restore.go (see F-collision note: none in B1).
|
||||
Proof of fix: unit test: capture marker row → drop → RestoreFromRecoveryUnit → row present.
|
||||
Re-run the live romm marker repro from the findings report.
|
||||
Open question: For an app whose DB lives in a named volume AND has a .sql dump, replaying both could
|
||||
double-apply — decide precedence (prefer .sql replay for logical consistency, or skip
|
||||
.sql when the DB's named volume tar was restored). Needs a one-line policy decision.
|
||||
```
|
||||
|
||||
### F9 — felhom-usb HDD never bound into the guest; provisioning omits user-data binds; disk APIs lack a "guest-attached" signal
|
||||
```
|
||||
Report claim: "HDD not passed to guest; HDD apps undeployable; disk APIs misrepresent it as available"
|
||||
Re-diagnosis: real (two layers) — provisioning gap is primary; "misreporting" is really a MISSING field
|
||||
True cause: LAYER A (provisioning): agent reconcile/bringup.go runBringUp (:158-294) writes only
|
||||
mp0 (docker-data, :236-250) + spec.Mounts as fresh PVE volume specs (:348-356) + mp9
|
||||
(bootstrap). It NEVER adds a host-path bind for an external user-data drive. That bind
|
||||
lives only in a separate, explicit enroll step: localapi/disks.go handleDiskGuestAttach
|
||||
(:238-287) → guestbind.go AttachBind (:51-71, `pct set <vmid> -mpN <host>/felhom-data`)
|
||||
→ requires guest reboot (:77-84). It was never re-run after 9201 was re-provisioned.
|
||||
A self-heal watchdog exists (disks.go:535-559) but only reconciles drives previously
|
||||
ENROLLED (intent recorded) — a never-enrolled drive is invisible to it.
|
||||
LAYER B (reporting): /disks state/reachable (storage/observe.go:290-299) and role
|
||||
(storage/role.go:105-126) faithfully describe HOST presence/topology; /host-metrics
|
||||
(host_metrics.go:28-49) emits the raw PVE content string. There is NO field anywhere
|
||||
that asserts "bound into guest N", so host-presence reads as availability. The role
|
||||
(user-data) vs content (backup) "disagreement" is two orthogonal attributes, not a bug.
|
||||
Severity: CRITICAL (blocks 13/55 apps; silent OS-disk placement; unblocks F13 + data-migration)
|
||||
Repo / layer: agent — internal/reconcile (provision) + internal/localapi (enroll, reporting);
|
||||
+ golden/bootstrap; + small controller/agent reporting field
|
||||
Fix approach: A) Make user-data drive attachment part of (re)provisioning OR auto-re-enroll on
|
||||
bring-up: record the drive's durable-id intent at first enroll (already done) and have
|
||||
runBringUp re-assert known user-data binds for the guest after recreating it (reuse
|
||||
AttachBind + the intent store the watchdog already reads), so destroy+recreate restores
|
||||
the mp. B) Add a `guest_attached bool` (per-guest) to the DiskInfo/storage-target the
|
||||
agent returns, set by checking the guest LXC config for an mp whose source is the
|
||||
drive's mount_path; surface it in the controller so /api/disks and the UI distinguish
|
||||
"present on host" from "usable by your server". This also fixes the F2 disagreement
|
||||
presentation. (Operationally for THIS demo: run guest-attach + reboot for felhom-usb.)
|
||||
Effort: L (provisioning change + new reporting field across agent+controller)
|
||||
Risk: SUPERVISED — agent/provisioning/golden; touches pct/LXC config on a live host.
|
||||
Depends on / blocks: blocks F13 (3-2-1 needs a real 2nd drive), F11 (real HDD class), data-migration.
|
||||
Proof of fix: re-provision a scratch guest with an enrolled user-data drive → mp re-appears in
|
||||
<vmid>.conf and the drive is writable in-guest; /api/disks shows guest_attached=true;
|
||||
an HDD app deploys onto it.
|
||||
Open question: Should bring-up auto-re-enroll ALL previously-enrolled drives unattended, or require an
|
||||
operator confirm (a bind is a data-path change)? Recommend auto-re-assert for drives
|
||||
with a recorded intent + durable-id match; flag mismatches. Needs operator sign-off.
|
||||
```
|
||||
|
||||
### F20-BUG1 — Controller's `agentapi.FormatDisk` swallows the agent's error and returns `ok:true` (zero-value)
|
||||
```
|
||||
Report claim: "agent format handler maps mkfs failure to ok:true (swallowed error)"
|
||||
Re-diagnosis: real, but MIS-ATTRIBUTED — the AGENT is correct; the bug is in the CONTROLLER client
|
||||
True cause: Agent handleDiskFormat returns ok:false / HTTP 502 on mkfs failure (localapi/disks.go
|
||||
:437-440, :476-480 → writeErr → {"ok":false} 502) — correct. The controller's
|
||||
agentapi/client.go FormatDisk (:372-398) calls postWithStatus, which by design does
|
||||
NOT surface an ok:false business error (comment :402). On the agent's 502, env.Data is
|
||||
JSON `null` → json.Unmarshal leaves a zero-value FormatResult; the 403/NeedsConfirmation
|
||||
guards (:389,:393) don't match a 502, so it falls through to `return out, nil` (:397).
|
||||
web/agent_disk_handlers.go:191-207 then sees err==nil and writes
|
||||
{"data":{...zeros...},"ok":true} HTTP 200 — exactly the observed response.
|
||||
Severity: HIGH (a destructive op silently reports success on failure)
|
||||
Repo / layer: controller — internal/agentapi/client.go (FormatDisk / postWithStatus)
|
||||
Fix approach: Have postWithStatus also return the envelope's error string + ok flag; in FormatDisk,
|
||||
after the NeedsConfirmation/403 handling, add: if status >= 300 (or !env.OK) and not a
|
||||
confirmation case → return out, fmt.Errorf("agentapi: format HTTP %d: %s", status, msg).
|
||||
The existing web handler `if err != nil → 502` branch then surfaces it correctly.
|
||||
Effort: S
|
||||
Risk: unattended-safe (controller-only error propagation)
|
||||
Depends on / blocks: co-edits client.go FormatDisk with F20-BUG3 (async rework) — see collision note.
|
||||
Proof of fix: unit test: stub agent 502 → FormatDisk returns non-nil err; handler returns ok:false.
|
||||
Open question: none.
|
||||
```
|
||||
|
||||
### F20-BUG3 — Synchronous format; controller's 15s client timeout cancels the request context → SIGKILLs in-flight mkfs → corrupt disk
|
||||
```
|
||||
Report claim: "large-disk format times out → cancelled context kills mkfs → corrupt disk"
|
||||
Re-diagnosis: real (confirmed end-to-end)
|
||||
True cause: agentapi/client.go:87 sets http.Client{Timeout: 15s} for ALL agent calls incl.
|
||||
/disks/format. Agent runs mkfs under the REQUEST context: handleDiskFormat passes
|
||||
r.Context() (disks.go:437,:476) → hostops.go:346 → proxmox/privileged.go:57/59
|
||||
exec.CommandContext(ctx,...). When the 15s client timeout fires, Go cancels the request
|
||||
→ r.Context() done → CommandContext sends SIGKILL to mkfs mid-write → corrupt fs
|
||||
(matches agent log "mkfs.ext4 ... signal: killed").
|
||||
Severity: HIGH (destructive: corrupts a large drive)
|
||||
Repo / layer: agent — internal/localapi (format handler) + internal/storage; controller — agentapi
|
||||
Fix approach: NOT "raise the timeout". Make format a detached job + status poll, modelled on the
|
||||
agent's existing patterns: handleGuestReboot (disks.go:298-326) already runs a long op
|
||||
in a goroutine off s.baseCtx and returns 202; the backup job (StartBackup/BackupStatus,
|
||||
client.go:106-207, phases idle|running|...|done) is the status-poll template. Run mkfs
|
||||
under s.baseCtx (NOT r.Context()) so a dropped HTTP request can't kill it; add
|
||||
/disks/format → {job_id} + /disks/format/status; controller polls. The async worker
|
||||
MUST re-resolve the durable-id at execution time (preserve AGENT-001 anti-retarget,
|
||||
wipe_reresolve.go).
|
||||
Effort: L
|
||||
Risk: SUPERVISED — agent destructive path; design + live validation on a scratch device.
|
||||
Depends on / blocks: co-edits client.go FormatDisk with F20-BUG1.
|
||||
Proof of fix: format a large scratch device; drop the HTTP client mid-op → mkfs continues to
|
||||
completion; status endpoint reports done; device mounts clean.
|
||||
Open question: Status persistence across an agent restart (like BackupRecord)? Recommend yes, reuse
|
||||
the backup record pattern. Confirm with operator whether a format must survive agent
|
||||
restart.
|
||||
```
|
||||
|
||||
### F5 — uptime-kuma catalog healthcheck points at a nonexistent file → permanent unhealthy → Traefik withholds route → 404
|
||||
```
|
||||
Report claim: "broken catalog healthcheck → unhealthy → Traefik 404; design: unhealthy = total outage"
|
||||
Re-diagnosis: real (data fix) + a broader catalog risk + an unaddressed design question
|
||||
True cause: app-catalog-felhom.eu/templates/uptime-kuma/docker-compose.yml:24-29 overrides
|
||||
healthcheck with `["CMD","node","/app/extra/healthcheck.mjs"]`; image louislam/
|
||||
uptime-kuma:2 ships its healthcheck as a COMPILED Go binary at /app/extra/healthcheck
|
||||
(no .mjs) and already defines a working built-in HEALTHCHECK. The override fails →
|
||||
permanent unhealthy. Survey found MANY other apps using wget/curl healthchecks on
|
||||
non-Alpine bases (claper, calcom, rallly, wanderer, wishlist, plant-it, papra, gokapi,
|
||||
termix; kimai, komga, crafty-controller) where the tool may be absent — same failure
|
||||
class, unconfirmed. Several migration-heavy apps use start_period:30s (calcom, outline,
|
||||
docmost, rallly, tandoor, nextcloud, wger) → transient 404 right after deploy.
|
||||
Severity: HIGH (one confirmed total-outage; broader latent set)
|
||||
Repo / layer: app-catalog-felhom.eu (data) + DESIGN question spanning Traefik/controller dashboard
|
||||
Fix approach: (data) Delete the uptime-kuma compose healthcheck block (lines 24-29) → inherit the
|
||||
image's built-in; or set test:["CMD","extra/healthcheck"]. Then audit the wget/curl
|
||||
healthchecks: for each, `docker run --rm <image> which wget||which curl`; replace
|
||||
absent-tool checks with inline `node -e`/`python3` socket checks (mealie's pattern is
|
||||
the precedent already in-catalog). Bump start_period to 60-120s for migration apps.
|
||||
Effort: S (uptime-kuma alone) / M (full catalog audit)
|
||||
Risk: unattended-safe (catalog data; auto-syncs to controllers within 15m).
|
||||
Depends on / blocks: independent.
|
||||
Proof of fix: redeploy uptime-kuma → healthy within start_period → status.<domain> → 200.
|
||||
Open question: DESIGN (needs operator decision, do NOT silently pick): an unhealthy container makes
|
||||
Traefik withhold the route → hard 404, indistinguishable from "not deployed". Options:
|
||||
(a) leave as-is (fail-safe: don't route to unhealthy); (b) publish the route even when
|
||||
unhealthy so the app is reachable while degraded; (c) keep gating but surface
|
||||
"route unpublished because unhealthy" distinctly in the dashboard so it's not a silent
|
||||
404. Recommend (c). Decision required.
|
||||
```
|
||||
|
||||
### F1 — Memory metric reads host `/proc/meminfo` (16GB), not the LXC 2GB cgroup cap — and it defeats the deploy memory guard
|
||||
```
|
||||
Report claim: "system memory reports host RAM not the 2GB guest cap; unsafe headroom"
|
||||
Re-diagnosis: real — AND confirmed it is NOT display-only: it disables the deploy memory hard-block
|
||||
True cause: system/info_linux.go readMemInfo (:80-113) parses /proc/meminfo MemTotal/MemAvailable
|
||||
directly. No cgroup awareness anywhere (repo-wide: zero matches for cgroup/memory.max/
|
||||
limit_in_bytes/Sysinfo/lxcfs). The container has no lxcfs and unbounded cgroup
|
||||
memory.max → reads host 16GB. Consumed by the deploy gate: stacks/deploy.go:162
|
||||
system.GetMemoryMB() → hard block at :173 (usedMB+newReqMB > usableMB) and overcommit
|
||||
warning at :188. With totalMB=15771 the hard block effectively never fires → the
|
||||
controller will deploy apps the 2GB LXC cannot run. Memory bar UI uses the same value.
|
||||
Severity: HIGH (defeats the deploy OOM guard; not merely cosmetic)
|
||||
Repo / layer: controller — internal/system/info_linux.go
|
||||
Fix approach: Add a cgroup-limit read used as min(cgroupLimit, meminfoTotal): cgroup v2
|
||||
/sys/fs/cgroup/memory.max ("max" → fall back), v1 /sys/fs/cgroup/memory/
|
||||
memory.limit_in_bytes (ignore the ~unbounded sentinel). For used/available use v2
|
||||
memory.current / v1 memory.usage_in_bytes. Wire into readMemInfo so TotalMemMB/
|
||||
AvailMemMB/UsedMemMB/MemPercent reflect the cap; all accessors (GetMemoryMB,
|
||||
GetTotalMemoryMB, GetInfo) become correct with no signature change.
|
||||
Effort: S
|
||||
Risk: unattended-safe (read-only metric change in the controller).
|
||||
Depends on / blocks: independent.
|
||||
Proof of fix: unit test with a fake cgroup file → reports the limit; on the 2GB guest /api/system/info
|
||||
shows ~2048; deploy hard-block triggers when committed mem approaches 2GB.
|
||||
Open question: none.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM — specs
|
||||
|
||||
### F11 — `needs_hdd` app deploys onto the OS rootfs with no drive-class guard
|
||||
```
|
||||
Report claim: "HDD-required app silently lands on the 32G OS disk"
|
||||
Re-diagnosis: real
|
||||
True cause: Path validation is existence-only: stacks/deploy.go:278-283 (os.Stat of the value).
|
||||
isValidDrivePath (web/handler_export.go:356-364) only checks string-equality against a
|
||||
registered StoragePath. StoragePath has no drive-class attribute; the deploy dropdown
|
||||
(web/handlers.go:322-333 from GetSchedulableStoragePaths) has no class filter. NeedsHDD
|
||||
(stacks/metadata.go:58) is a UI badge only, never cross-checked against the chosen
|
||||
path's physical drive. sys_drive is the SSD fallback namespace
|
||||
(config.go:265 /mnt/sys_drive; appbackup/paths.go:24-29 + backup.go:105-107).
|
||||
Severity: MEDIUM (data-placement footgun; on a real customer it fills the OS disk)
|
||||
Repo / layer: controller — internal/stacks (deploy) + internal/web (dropdown) + internal/settings
|
||||
Fix approach: Classify each StoragePath by backing device (compare Stat_t.Dev against the rootfs
|
||||
device; system.SamePhysicalDevice already exists for Tier-2, and agentapi exposes
|
||||
durable-id/device info). In DeployStack after the path check (deploy.go:283): if
|
||||
meta.Resources.NeedsHDD and the chosen path resolves to the OS/rootfs device → return
|
||||
a deployWarning (the function already has that channel, :160/:191) or refuse. Annotate
|
||||
the dropdown likewise.
|
||||
Effort: M
|
||||
Risk: unattended-safe (controller-only).
|
||||
Depends on / blocks: best AFTER F9 (so a real HDD class exists to steer toward); works standalone as a
|
||||
warning even now.
|
||||
Proof of fix: deploy a needs_hdd app with only sys_drive available → warning/refusal surfaced.
|
||||
Open question: warn-and-allow vs hard-refuse when no real HDD exists? Recommend warn-and-allow (demo/
|
||||
single-SSD nodes are legitimate). Minor product call.
|
||||
```
|
||||
|
||||
### F20-BUG2 — durable_id scheme mismatch: `/disks` emits `uuid:…`, the wipe gate expects `byid:`/`byuuid:…`
|
||||
```
|
||||
Report claim: "durable_id scheme mismatch across endpoints; customer copying the disk-list id is refused"
|
||||
Re-diagnosis: real
|
||||
True cause: List path: storage/durableid.go:54-63 emits `uuid:`+fs-uuid for usb/local-dir
|
||||
(→ DiskInfo.DurableID in /disks). Gate path: storage/durable_device.go:34-51
|
||||
DeviceDurableID returns `byid:`+wwn (:43) or `byuuid:`+uuid (:47); ResolveDurableDevice
|
||||
(:55-74, the AGENT-001 anti-retarget resolver) only accepts byid:/byuuid: and refuses a
|
||||
bare/uuid: scheme. Gate compares the customer's ConfirmDurableID against DeviceDurableID
|
||||
(localapi/disks.go:450,454-457) → binding_mismatch. Three prefixes (uuid: vs byid: vs
|
||||
byuuid:) from two functions.
|
||||
Severity: MEDIUM (usability: the documented confirm flow can't succeed with the advertised id)
|
||||
Repo / layer: agent — internal/storage (durableid.go + durable_device.go); reflected in controller
|
||||
Fix approach: Make the disk-LIST advertise the SAME durable-id the gate will accept (the byid:/byuuid:
|
||||
scheme from DeviceDurableID), or include both an `id` (display) and a `wipe_durable_id`
|
||||
(gate-accepted) field so the UI/customer always confirm with the gate's scheme. Do NOT
|
||||
relax the gate to accept uuid: (that weakens AGENT-001 anti-retarget). Single source of
|
||||
truth: have handleDisks call DeviceDurableID for the wipe id.
|
||||
Effort: M
|
||||
Risk: SUPERVISED — touches the wipe binding semantics; validate against AGENT-001 invariants.
|
||||
Depends on / blocks: same files as F20-BUG3 region (agent storage/localapi) — sequence with Batch 3.
|
||||
Proof of fix: read /disks id → POST /disks/format confirmed with it → gate authorizes (no
|
||||
binding_mismatch) on a scratch device.
|
||||
Open question: expose one canonical id or a display+wipe pair? Recommend the pair (UI clarity).
|
||||
```
|
||||
|
||||
### F8 — Infra secrets in `controller.yaml` are plaintext at `0644`
|
||||
```
|
||||
Report claim: "cf_api_token / cf_tunnel_token / hub api_key plaintext (not enc: like app secrets)"
|
||||
Re-diagnosis: real, but largely by-design for runtime config; the concrete weakness is file perms
|
||||
True cause: config/config.go fields (CFTunnelToken/CFAPIToken :72-75, HubConfig.APIKey :175,
|
||||
GitConfig.Token :96, WebConfig.SessionSecret :88) are plain yaml; loadAndParse
|
||||
(:206-224) does ReadFile→ExpandEnv→Unmarshal, no decryption. App.yaml secrets ARE
|
||||
enc:-wrapped (deploy.go:683-692). Config is written 0644 on apply (router.go:1007).
|
||||
Severity: LOW-MEDIUM (these are runtime-required creds the guest legitimately holds; the file
|
||||
mode is the real gap)
|
||||
Repo / layer: controller — internal/config / config apply
|
||||
Fix approach: Write/chmod controller.yaml to 0600. (Encrypting at rest with a key co-located on the
|
||||
same rootfs adds little; if wanted later, derive the key from an env-injected secret,
|
||||
not a file on the guest.)
|
||||
Effort: S
|
||||
Risk: unattended-safe.
|
||||
Depends on / blocks: independent.
|
||||
Proof of fix: stat controller.yaml → 0600.
|
||||
Open question: Is at-rest encryption of infra creds a requirement, or is 0600 sufficient given the
|
||||
threat model (guest-rootfs compromise already = game over)? Recommend 0600 now.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## LOW / TRIVIAL — brief specs
|
||||
|
||||
### F6 — Deploy POST returns `"deployed"` before compose finishes (async by design; message misleads)
|
||||
```
|
||||
Re-diagnosis: real (by design). router.go:381 returns 200 after validation; DeployStack launches
|
||||
compose in a goroutine (deploy.go:339) — the documented anti-stale-button pattern; UI
|
||||
polls GET /api/stacks/{name} every 3s. Only the WORDING is wrong.
|
||||
Fix: change message to "Telepítés elindítva" / return 202 Accepted. Do NOT make it sync.
|
||||
Severity: LOW. Effort S. unattended-safe. controller internal/api.
|
||||
```
|
||||
|
||||
### F7 — Dashboard state lags Docker health by ~10s
|
||||
```
|
||||
Re-diagnosis: real (cache cadence). status-refresh ticker is 30s (cmd/controller/main.go:256);
|
||||
stack-scan 2min (:259); the dashboard list serves the in-memory map. Health is read
|
||||
correctly (manager.go:437), just polled slowly. (Deploy page already polls per-stack 3s.)
|
||||
Fix: lower status-refresh to ~10s, and/or fire an extra RefreshStatus() shortly after
|
||||
deploy/start completes (runComposeDeploy already calls it once at :401).
|
||||
Severity: LOW. Effort S. unattended-safe. controller.
|
||||
```
|
||||
|
||||
### F4 — `GET /api/stacks/rescan` → "stack not found: rescan"
|
||||
```
|
||||
Re-diagnosis: real but trivial. A rescan route EXISTS but POST-only (router.go:112 rescanStacks);
|
||||
GET falls through to GET /stacks/{name} (:116) → getStack("rescan") → "not found" (:314).
|
||||
Reachable via POST /api/stacks/rescan or POST /api/sync.
|
||||
Fix: add a default 405 for /stacks/rescan on non-POST so it doesn't masquerade as a stack
|
||||
lookup; fix the runbook to use POST. (Doc-first.)
|
||||
Severity: TRIVIAL. Effort S. unattended-safe. controller.
|
||||
```
|
||||
|
||||
### F3 — JSON API shows double-encoded Hungarian (`SzemĂ©lyes`)
|
||||
```
|
||||
Re-diagnosis: MIS-ATTRIBUTED / not-in-code → needs-runtime-confirmation. Source .felhom.yml is clean
|
||||
single-UTF-8 (verified hexdump); yaml.Unmarshal (metadata.go:145) and
|
||||
json.NewEncoder (router.go:1079) are UTF-8-native; git-sync copies bytes verbatim
|
||||
(sync.go:379-411). No transcoding exists. The rendered HTML being correct (same source
|
||||
string) is strong evidence the bytes aren't actually doubled — the mojibake is most
|
||||
likely a terminal/curl display artifact in the live measurement, OR a stale cache.
|
||||
Fix: none in code. Re-measure: `curl .../api/stacks | xxd | grep -i saj` and compare bytes to
|
||||
source; only if doubled, re-sync the catalog cache on the guest.
|
||||
Severity: LOW. needs-runtime-confirmation.
|
||||
```
|
||||
|
||||
### F2 — `hdd_configured:false` disagrees with `/api/disks` listing the HDD
|
||||
```
|
||||
Re-diagnosis: already-fine (expected symptom of F9, not a bug). hdd_configured comes from the
|
||||
controller's own config plane: system/info_linux.go:32-35 (true iff hddPath != ""),
|
||||
fed by cfg.Paths.HDDPath (router.go:711). /api/disks is the agent's HOST view. They
|
||||
legitimately differ because the HDD is host-present but NOT guest-bound and NOT in
|
||||
controller.yaml.
|
||||
Fix: no independent fix; the F9 "guest_attached" reporting field resolves the user-facing
|
||||
confusion. Optionally make /api/system/info reflect any registered guest storage path
|
||||
(it currently ignores GetStoragePaths fallback that monitor/report use).
|
||||
Severity: LOW (folds into F9).
|
||||
```
|
||||
|
||||
### F15 — Controller not auto-restarted after a manual `docker stop`/`kill`
|
||||
```
|
||||
Re-diagnosis: already-fine / standard semantics. Golden bakes `docker run --restart unless-stopped`
|
||||
(felhom-agent/configs/build-golden.sh:179) — restarts on crash/daemon-restart but NOT
|
||||
after a manual stop (by design). The systemd unit is Type=oneshot/RemainAfterExit
|
||||
(:189-204) — runs the bootstrap once, not a supervisor. Genuine crashes ARE covered.
|
||||
Fix: none required. If "survives manual stop" becomes a requirement: add a periodic
|
||||
agent/hub desired-state reconcile that re-asserts the container is running (preferred
|
||||
over fighting unless-stopped with systemd Restart=always).
|
||||
Severity: LOW (acceptable as-is).
|
||||
```
|
||||
|
||||
### F13 — 3-2-1 weakened: primary recovery units share the app's disk
|
||||
```
|
||||
Re-diagnosis: already-fine as code; pure consequence of F9. Backup path helpers
|
||||
(appbackup/paths.go:31-70) root the primary unit at the app's own nsRoot by design;
|
||||
the OFF-drive copy is Tier-2 (internal/backup/tier2.go) which already targets a
|
||||
different physical disk (system.SamePhysicalDevice) once one exists.
|
||||
Fix: no path-helper change. Resolved by F9 (attach a real 2nd drive → Tier-2 has an off-disk
|
||||
target). Until then Tier-2 correctly reports "needs 2nd drive".
|
||||
Severity: MEDIUM consequence, but no independent action (→ F9).
|
||||
```
|
||||
|
||||
### PASS findings — confirmed working, no action
|
||||
```
|
||||
F10 (crash-loop detection), F12 (lifecycle ops), F14 (CTRL-T2-1 crash-window), F16 (import/export +
|
||||
CTRL-001 traversal defense), F18 (removal + protected-stack guards), F19 (monitoring/settings/sync/
|
||||
hub reporting). Confirmed real PASSes against code + the report's evidence. Residual sub-issues that
|
||||
DID get specs: F6 (optimistic POST wording), F11 (OS-disk footgun), F7 (state lag). No other residuals.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## SEQUENCED FIX PLAN
|
||||
|
||||
### Batch 1 — quick contained wins → ship as controller **v0.61.0** (all S, unattended-safe)
|
||||
- **F1** (cgroup memory read — `system/info_linux.go`) — restores the deploy OOM guard; highest value-per-effort.
|
||||
- **F20-BUG1** (surface agent format error — `agentapi/client.go: FormatDisk`) — stop silent destructive-op "success".
|
||||
- **F6** (deploy message wording / 202 — `api/router.go`).
|
||||
- **F7** (status-refresh cadence 30s→10s — `cmd/controller/main.go`).
|
||||
- **F8** (controller.yaml → 0600 — config apply).
|
||||
- **F4** (405 for non-POST `/stacks/rescan` — `api/router.go`).
|
||||
- **Ship alongside (separate repo, auto-syncs): F5 uptime-kuma** healthcheck fix in `app-catalog-felhom.eu` — delete the broken override.
|
||||
- *Grouping rationale:* all controller-internal, no agent/provisioning, no destructive-path behaviour change, independent files. Each is independently testable and low-risk.
|
||||
- *File-collision warnings:* **F4 and F6 both edit `internal/api/router.go`** (different cases — coordinate one PR). **F20-BUG1 edits `agentapi/client.go: FormatDisk`, which Batch-3 F20-BUG3 will re-architect** — land BUG1 first (small error-surface change), and have BUG3 build on it (BUG3's async path must keep the error-surfacing).
|
||||
|
||||
### Batch 2 — controller, needs-spec (M/L), no live host risk
|
||||
- **F17** (replay `.sql` on per-app restore; extract shared `dbrestore` pkg from `appexport`; make volume-restore failure non-silent). The CRITICAL data-recovery fix.
|
||||
- **F11** (drive-class guard in `DeployStack` + dropdown).
|
||||
- *Sequencing:* F17 first (it's the critical correctness fix). F11 is best AFTER F9 lands (real HDD class to steer toward) but can ship a warning now.
|
||||
- *File-collision warnings:* **F17 co-edits `internal/backup/restore_unit.go` + `restore.go` + extracts from `internal/appexport/restore.go`** — keep this as one PR; nothing else in Batch 1/2 touches `internal/backup` or `internal/appexport`. F11 touches `internal/stacks/deploy.go` + `internal/web/handlers.go` + `internal/settings` — disjoint from F17.
|
||||
- *Runtime-confirm before/with F17:* verify whether `POST /backup/run` actually writes volume tars to `<unit>/volume-dumps/` (the live drive saw none for romm). If capture is also missing for some apps, the F17 fix must ensure capture, not just replay.
|
||||
|
||||
### Batch 3 — foundational, SUPERVISED (agent / provisioning / golden / destructive)
|
||||
- **F9** (provisioning auto-re-enroll user-data binds + `guest_attached` reporting field) — agent `reconcile` + `localapi` + golden/bootstrap + small controller field. Unblocks data-migration, F13 (3-2-1), and the real-HDD half of F11/F2.
|
||||
- **F20-BUG3** (async detached mkfs + `/disks/format/status`; run under `s.baseCtx`; preserve AGENT-001 re-resolve) — agent destructive path + controller `agentapi` polling.
|
||||
- **F20-BUG2** (durable_id scheme consistency: `/disks` advertises the gate-accepted id) — agent `storage`.
|
||||
- *Why last + supervised:* all touch the host agent / LXC config / destructive wipe path and need live validation on scratch guests/devices, not unattended pushes. They also carry the highest blast radius (provisioning, disk-wipe).
|
||||
- *File-collision warnings:* **F20-BUG2 and F20-BUG3 both touch `felhom-agent/internal/storage` + `internal/localapi/disks.go`** (durable-id derivation + the format handler) — do them in one coordinated agent PR. **F20-BUG3 re-edits controller `agentapi/client.go: FormatDisk`** already changed in Batch 1 (BUG1) — rebase BUG3 on the BUG1 error-surfacing. **F9 touches `reconcile/bringup.go` + `localapi/disks.go` + `guestbind.go`** — `disks.go` is also touched by F20-BUG2/3, so sequence F9 and the F20 agent work to avoid stepping on `disks.go` (recommend: F9 first, then F20-BUG2/BUG3 rebased).
|
||||
|
||||
---
|
||||
|
||||
## WRAP-UP
|
||||
|
||||
**Re-diagnosis verdict counts:** real = **11** (F1, F5, F9, F11, F17, F20-BUG1, F20-BUG2, F20-BUG3, F6, F7, F8); mis-attributed = **3** (F20-BUG1 relocated to controller, F17 volume-capture sub-claim, F3 not-in-code); already-fine/no-independent-action = **9** (F10, F12, F14, F16, F18, F19, F13, F2, F15); needs-runtime-confirmation = **2** (F3 byte recheck, F17 capture-side volume-tar question); trivial/doc = **1** (F4).
|
||||
|
||||
**Batch 1 (ship first, v0.61.0):** F1 (cgroup memory → restores deploy OOM guard), F20-BUG1 (stop silent format "success"), F6 (deploy message wording), F7 (status cadence), F8 (controller.yaml 0600), F4 (405 on rescan) + F5 uptime-kuma healthcheck in the catalog repo.
|
||||
|
||||
**Report-causes found WRONG against the code (the key output):**
|
||||
- **F20-BUG1** — report blamed the *agent's* format handler; the agent correctly returns 502. The bug is the *controller's* `agentapi.FormatDisk` (`client.go:393-397`) swallowing the agent's error and returning a zero-value `ok:true`.
|
||||
- **F17** — report said "recovery units don't capture volume data"; they do (`restoreDockerVolumes` + `VolumeDumps`). The real gap is the captured `.sql` dump is **never replayed** on per-app restore (and the DB-import logic already exists, unused, in `appexport`).
|
||||
- **F3** — report said the JSON API double-encodes Hungarian; no transcoding exists in the code path (source is clean UTF-8, encoder is UTF-8-native, sync copies bytes). Most likely a terminal/`curl` display artifact — re-measure with `xxd`.
|
||||
- (Refinement, not contradiction) **F9** — the disk APIs don't "misreport"; they faithfully describe *host* presence. The real defect is the *absence* of a guest-attached signal, plus provisioning never binding user-data drives.
|
||||
|
||||
**Open design questions needing the operator's decision before coding:**
|
||||
1. **F5 (Traefik/unhealthy):** publish routes for unhealthy containers, keep gating, or keep gating but surface "route unpublished because unhealthy" distinctly? (Recommend the last.)
|
||||
2. **F9 (re-enroll):** may bring-up auto-re-assert previously-enrolled user-data binds unattended, or must an operator confirm each data-path bind? (Recommend auto for durable-id-matched intents.)
|
||||
3. **F17 (double-apply):** when an app has BOTH a named-volume tar and a `.sql` dump, which wins? (Recommend `.sql` replay for logical consistency.)
|
||||
4. **F11 (no-HDD nodes):** warn-and-allow vs hard-refuse a needs_hdd app on a single-SSD node? (Recommend warn-and-allow.)
|
||||
5. **F20-BUG3:** must an in-progress format survive an agent restart (persisted job record)? (Recommend yes, reuse the backup-record pattern.)
|
||||
6. **F8:** is at-rest encryption of infra creds required, or is `0600` sufficient under the current threat model? (Recommend 0600 now.)
|
||||
Reference in New Issue
Block a user