From b01bb4b88d901c0bf83410edcb06b65727537ede Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 24 Oct 2026 15:26:56 +0200 Subject: [PATCH] Review round 2: a media restart no longer starts an app the household stopped (R-721); the file manager's header name file appears whole (R-1100 race, 44/300 -> 0/300); both red-proved; unreleased --- CHANGELOG.md | 14 ++++++ REPORT.md | 34 ++++++------- controller/README.md | 4 +- controller/internal/infra/filebrowser_sso.go | 34 +++++++++---- controller/internal/stacks/media_library.go | 15 ++++++ .../internal/stacks/media_library_test.go | 27 ++++++++++ controller/internal/web/media_events.go | 17 ++++++- .../web/r2_media_recreate_wiring_test.go | 49 +++++++++++++++++++ 8 files changed, 163 insertions(+), 31 deletions(-) create mode 100644 controller/internal/web/r2_media_recreate_wiring_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1df0761..617e339 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,20 @@ brief 2026-10-24, Part 4 (commit only; the release rides the next normal train). `…OnlyWhenTheAgentWaitsAndAnAppRuns`, `…QBLimitRuleSkipsAHeldLimit`, `…TheTwoAppsReadAndWriteTheirLimit`, `TestLineTestRunOutlastsTheClientTimeout`, `TestStartUpwatchRecoversTheHoldFirst`, `TestUpwatchHoldGetsBothAgentCalls`. +### Review round 2 (2026-10-24, unattended; commit only, no release) — two small fixes, each red-proved + + +- **A media restart no longer starts an app the household stopped (R-721).** „Újraindítás most", „Később" (03:30) + and the safe removal recreated every media app with `up -d --force-recreate`, which STARTED a stopped one. + `stacks.MediaAppsToRecreate` leaves those out (logged „NOT recreating … the household stopped it"); their override + is already written, so their next start sees the drives. Pinned by `TestMediaAppsToRecreate_SkipsAppsTheHouseholdStopped` + (stacks) and the wiring pin `TestR2_MediaRecreateLoopsSkipStoppedApps` (web) — each seen failing with the rule off. +- **The file manager's per-box header name file appears whole (R-1100).** `infra.LoadOrCreateFileBrowserSSOHeader` + created the file and then wrote it; a second caller at the same start (the stacks manager and the web server) could + read it empty and get an error — 44 of 300 runs of the existing parallel-writers check failed, 4 of 50 under + `-race`. Now a 0600 temp file is written, synced and hard-linked into place (the link is the exclusive create): + 0 of 300, 0 of 50 under `-race`. + ## v0.342.0 (2026-10-24) — RELEASED on the operator's yes (attended release run; image sha256:d002d3b7…, from `42cda07`, CI 2283) — the upload and CPU watch, the tested line and qBittorrent's limit **MinAgent: 0.158.1** (unchanged). **Hub dependency:** the four new event types need hub 0.169.0 (committed the same diff --git a/REPORT.md b/REPORT.md index bb50e85..4310310 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,20 +1,18 @@ -# REPORT — the line test for a box that seeds all day (R-1250), 2026-10-24 (attended) +# REPORT — review round 2 (2026-10-24, unattended): two small fixes, commit only -Brief: „two DooPlex backup alarms …", Part 4 (commit only; the release rides the next normal train). Design: felhom.eu -`architecture/08-alarm-ladder.md` §9 item 5. Commit `046b808` (CI 2287 success). Not released, no image built. +**What:** found by the review round's readers and verified at `file:line` by the main session (felhom.eu +`documentation/audits/review-round-2-2026-10-24/`). No release; both ride the next normal train (next v0.343.0). -- **What:** when the agent says the weekly test is due and waits only for quiet, and qBittorrent or Transmission runs, - the controller records the app's upload limit first, holds the app at 1 KiB/s, asks the agent for a test on request, - then puts the exact old limit back. Deferred release; a failed hold is undone first; a household change during the - test stays; an unreachable app keeps the record; a killed controller's start-up check puts it back; the 50 % rule - waits while a record exists. `agentapi.LineTestRun` (own 2-minute timeout via `postSlow`); Transmission's RPC shared. -- **Tests:** 13 new (CHANGELOG lists them), each seen failing: seven hold rules broken one by one, the slow POST, the - start-up order, the wiring of both agent calls. -- **Live (demo-hp, endpoint level, probes from this commit and agent `1e087bb`):** a real 10 Mbit/s seed from scratch - guest 9202 to DooPlex's qBittorrent. Without the hold the agent refused (`not-quiet`, 11.1 Mbit/s). With it the uplink - fell to 0.02–0.16 Mbit/s, the test read 94.2 Mbit/s, the hold lasted 56.8 s, and qBittorrent was back at 1 249 280 B/s - exactly. A `kill -9` in the middle left the record and the 1 KiB/s; the start-up check put the limit back. The pinned - agent client is unit-tested only (the probes spoke plain HTTP). Evidence: felhom.eu - `audits/dooplex-alarms-2026-10-24/part4-r1250-live-proof.txt`. -- **Teardown:** the probe and its store removed from 9202's controller container; qBittorrent removed through the product. -- **MinAgent:** 0.158.1 unchanged (an older agent never reports `waiting_for_quiet`, so nothing is held). +1. **A media restart no longer starts an app the household stopped (R-721).** `restartMediaApps` and + `mediaSafeRemoval` recreated every media app (`RestartStack` = `up -d --force-recreate`), skipping only held ones. + Now both take `stacks.MediaAppsToRecreate`, which leaves out an app whose recorded intent is „stopped" (logged). Its + override is already written, so its next start sees the drives. Tests: `TestMediaAppsToRecreate_SkipsAppsTheHouseholdStopped` + (red with the rule off) and `TestR2_MediaRecreateLoopsSkipStoppedApps` (source pin of the two loops; red with + `s.mediaApps()` back in the safe removal). +2. **The file manager's header name file appears whole (R-1100).** `LoadOrCreateFileBrowserSSOHeader` created the file + empty and wrote it after; a parallel caller read "" and got an error. Measured on clean `main` `321df1f`: 44 of 300 + runs of the existing parallel-writers check failed (4 of 50 under `-race`). Now: temp file, sync, hard link into place + — 0 of 300, 0 of 50 under `-race`. + +`go vet ./...` and `go test ./...` green. Design docs read: `15-media-library.md` §10–11 (the restart), `01` §5 / `02` +(the file manager sign-in). diff --git a/controller/README.md b/controller/README.md index f5a9994..04cf87b 100644 --- a/controller/README.md +++ b/controller/README.md @@ -3518,7 +3518,9 @@ no key into the household's account). Code: `internal/stacks/after_unclean_boot. - **Drive events** (`internal/web/media_events.go`, `/api/storage/media/{status,restart,wire}`): added / removed / returned are recorded on the drive; the storage page and the dashboard show the notice with „Újraindítás most" (stream check first) and „Később" (03:30), and per drive „restarted since". A returned drive recreates the file - browser at once; a dead or removed drive is not bound into it. **Safe removal** with media apps: pause that drive's + browser at once; a dead or removed drive is not bound into it. „Újraindítás most", „Később" and the safe removal + recreate every media app EXCEPT one the household stopped (R-721; it reads the new binds at its next start — + `MediaAppsToRecreate`, unreleased, next v0.343.0). **Safe removal** with media apps: pause that drive's torrents → recreate the apps and the file browser without it → unmount → keep it registered; its return needs no household step, and the torrents resume after the restart. - **Plex:** the claim code is an `after_start` field — asked on the Plex page once Plex runs (`POST /apps/plex/plex-claim`); diff --git a/controller/internal/infra/filebrowser_sso.go b/controller/internal/infra/filebrowser_sso.go index 37078eb..e3252ef 100644 --- a/controller/internal/infra/filebrowser_sso.go +++ b/controller/internal/infra/filebrowser_sso.go @@ -64,22 +64,36 @@ func LoadOrCreateFileBrowserSSOHeader(dataDir string) (string, error) { return "", err } h := "X-Felhom-User-" + hex.EncodeToString(rb) - f, err := os.OpenFile(p, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + // The name file appears WHOLE or not at all (review round 2, 2026-10-24): it is written to a private temp file + // first and then hard-linked into place — the link is the exclusive create, so a second writer never reads an + // empty, half-written file (measured: 44 of 300 runs of the parallel-writers check read "" with the old + // create-then-write; pinned by TestFileBrowserSSOHeader_R1100_ABoxSecret). + tmp, err := os.CreateTemp(dataDir, fileBrowserSSOHeaderFile+".tmp-*") if err != nil { + return "", err + } + defer os.Remove(tmp.Name()) + if err := tmp.Chmod(0o600); err != nil { + tmp.Close() + return "", err + } + if _, err := tmp.WriteString(h + "\n"); err != nil { + tmp.Close() + return "", err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return "", err + } + if err := tmp.Close(); err != nil { + return "", err + } + if err := os.Link(tmp.Name(), p); err != nil { if os.IsExist(err) { return LoadOrCreateFileBrowserSSOHeader(dataDir) } return "", err } - if _, err := f.WriteString(h + "\n"); err != nil { - f.Close() - _ = os.Remove(p) - return "", err - } - if err := f.Close(); err != nil { - _ = os.Remove(p) - return "", err - } return h, nil } diff --git a/controller/internal/stacks/media_library.go b/controller/internal/stacks/media_library.go index 81e1734..4f94ac9 100644 --- a/controller/internal/stacks/media_library.go +++ b/controller/internal/stacks/media_library.go @@ -283,6 +283,21 @@ func (m *Manager) MediaApps() []string { return out } +// MediaAppsToRecreate is MediaApps without the apps the household stopped (R-721, „a Stop holds"): the media restart +// and the safe removal recreate with `up -d --force-recreate` (RestartStack), which would START a stopped app. Their +// override is already written for the drives present now, so their next start sees them. `stopped` is returned for the +// caller's log. Found by review round 2 (2026-10-24); pinned by TestMediaAppsToRecreate_SkipsAppsTheHouseholdStopped. +func (m *Manager) MediaAppsToRecreate() (recreate, stopped []string) { + for _, name := range m.MediaApps() { + if m.WantsStopped(name) { + stopped = append(stopped, name) + continue + } + recreate = append(recreate, name) + } + return recreate, stopped +} + // MediaAppsNeedingRestart renders every media app's override for the drives present NOW and returns the apps whose // binds changed — the apps that must restart to see a drive that was added, removed or came back (the notice of // decision 238). It writes the files; the restart itself is the household's „restart now" (RestartStack). diff --git a/controller/internal/stacks/media_library_test.go b/controller/internal/stacks/media_library_test.go index 0499ac2..6659b6f 100644 --- a/controller/internal/stacks/media_library_test.go +++ b/controller/internal/stacks/media_library_test.go @@ -187,3 +187,30 @@ func TestAfterStartField_NotAskedAtInstall(t *testing.T) { t.Fatalf("the template shape must parse as required + after_start: %+v %v", meta.DeployFields, err) } } + +// Review round 2 (2026-10-24, DOC-D2 U2): the media restart („Újraindítás most", „Később") and the safe removal +// recreate every media app with `up -d --force-recreate`, which STARTED an app the household had stopped — against +// R-721 („a Stop holds"). COMPANION RED-PROOF: return every MediaApps() entry from MediaAppsToRecreate → the stopped app +// is in `recreate` and this test fails. Restored. +func TestMediaAppsToRecreate_SkipsAppsTheHouseholdStopped(t *testing.T) { + for _, tc := range []struct { + desired string + wantRecreate bool + }{{DesiredStateStopped, false}, {DesiredStateRunning, true}, {"", true}} { + m, _ := newInstalledManager(t, sonarrLike, "deployed: true\nenv: {}\ndesired_state: "+tc.desired+"\n") + dir := filepath.Dir(m.stacks["bookstack"].ComposePath) + if err := os.WriteFile(filepath.Join(dir, ".felhom.yml"), []byte("media_library:\n read_only: true\n"), 0o644); err != nil { + t.Fatal(err) + } + if got := m.MediaApps(); len(got) != 1 { + t.Fatalf("fixture: want one media app, got %v", got) + } + recreate, stopped := m.MediaAppsToRecreate() + if tc.wantRecreate && (len(recreate) != 1 || len(stopped) != 0) { + t.Fatalf("desired %q: want recreated, got recreate=%v stopped=%v", tc.desired, recreate, stopped) + } + if !tc.wantRecreate && (len(recreate) != 0 || len(stopped) != 1) { + t.Fatalf("desired %q: a stopped app must NOT be recreated (it would start), got recreate=%v stopped=%v", tc.desired, recreate, stopped) + } + } +} diff --git a/controller/internal/web/media_events.go b/controller/internal/web/media_events.go index bc26d90..a5af726 100644 --- a/controller/internal/web/media_events.go +++ b/controller/internal/web/media_events.go @@ -50,6 +50,19 @@ func (s *Server) mediaApps() []string { return s.stackMgr.MediaApps() } +// mediaAppsToRecreate is the list the media restart and the safe removal recreate: every media app except the ones the +// household stopped (R-721) — a recreate would start them; their next start reads the override already written. +func (s *Server) mediaAppsToRecreate(why string) []string { + if s.stackMgr == nil { + return nil + } + recreate, stopped := s.stackMgr.MediaAppsToRecreate() + for _, name := range stopped { + s.logger.Printf("[INFO] [media] %s: NOT recreating %s — the household stopped it; it sees the drives at its next start", why, name) + } + return recreate +} + func (s *Server) presentMediaDrives() []mediawire.Drive { if s.stackMgr == nil { return nil @@ -195,7 +208,7 @@ func (s *Server) MediaNotice(checkStale bool) MediaDriveNotice { // recreates the file browser, records the restart on every drive, then wires the present drives in. func (s *Server) restartMediaApps(reason string) []string { var failed []string - for _, name := range s.mediaApps() { + for _, name := range s.mediaAppsToRecreate("restart (" + reason + ")") { if s.appHeld(name) { continue } @@ -304,7 +317,7 @@ func (s *Server) mediaSafeRemoval(path, durableID string) []string { if err := s.settings.SetEjected(path, durableID, paused); err != nil { s.logger.Printf("[WARN] [media] safe removal %s: recording it: %v", path, err) } - for _, name := range s.mediaApps() { + for _, name := range s.mediaAppsToRecreate("safe removal " + path) { if s.appHeld(name) { continue } diff --git a/controller/internal/web/r2_media_recreate_wiring_test.go b/controller/internal/web/r2_media_recreate_wiring_test.go new file mode 100644 index 0000000..60cd8b2 --- /dev/null +++ b/controller/internal/web/r2_media_recreate_wiring_test.go @@ -0,0 +1,49 @@ +package web + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "strings" + "testing" +) + +// Review round 2 (2026-10-24, DOC-D2 U2): both web paths that recreate the media apps must take their list from +// mediaAppsToRecreate (which leaves out the apps the household stopped, R-721), never from mediaApps(). A source pin: +// the decision itself is tested in stacks (TestMediaAppsToRecreate_SkipsAppsTheHouseholdStopped); this pins the +// WIRING, the half a seam test cannot see. COMPANION RED-PROOF: put `s.mediaApps()` back in either loop → this fails. +func TestR2_MediaRecreateLoopsSkipStoppedApps(t *testing.T) { + src, err := os.ReadFile("media_events.go") + if err != nil { + t.Fatal(err) + } + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "media_events.go", src, 0) + if err != nil { + t.Fatal(err) + } + want := map[string]bool{"restartMediaApps": false, "mediaSafeRemoval": false} + for _, d := range f.Decls { + fn, ok := d.(*ast.FuncDecl) + if !ok { + continue + } + if _, tracked := want[fn.Name.Name]; !tracked { + continue + } + body := string(src[fset.Position(fn.Body.Pos()).Offset:fset.Position(fn.Body.End()).Offset]) + if strings.Contains(body, "range s.mediaApps()") { + t.Errorf("%s ranges over s.mediaApps(): it would recreate (start) an app the household stopped", fn.Name.Name) + } + if !strings.Contains(body, "s.mediaAppsToRecreate(") || !strings.Contains(body, "RestartStack(") { + t.Errorf("%s no longer recreates from s.mediaAppsToRecreate", fn.Name.Name) + } + want[fn.Name.Name] = true + } + for name, seen := range want { + if !seen { + t.Errorf("function %s not found in media_events.go — the pin is stale", name) + } + } +}