Restore: acquire before destroying, and stop one dead volume from silencing a node

[MorningLightMountain713]

Overview

On 2026-08-04 a customer's Palworld world was destroyed by a restore. This PR fixes the path that destroyed it — and four further ways data could still be lost, which only became visible once these tests were run against real nodes for the first time.

What went wrong. One node had a single app whose backing volume had been deleted. That one unrepairable volume jammed a node-wide gate in the syncthing monitor, so no syncthing folder was ever configured for any app on that node, and no g: primary election ever ran there. A customer's Palworld instance was stranded on it — never synced, holding 373 bytes. Someone backed that instance up and restored it the next day. The restore deleted appdata before fetching anything, then fanned out a forced hard redeploy to every other instance, which rm -rf'd the 35 GB volume on the one node that actually held the world.

What this delivers:

• 5 product fixes — the node-wide gate, the backup gate, the restore rewrite, the mount-safety dedup, and a syncthing restart that could take the daemon down permanently.
• 6 new harness suites (34, 89–93), covering both ends of the incident for the first time — including one that boots a real syncthing daemon.
• Three further review rounds serviced (2026-08-11, 2026-08-24 ×2): 17 further commits — restore/monitor safety fixes in the same incident family, the HTTP status carried structurally instead of regexed out of error prose, streaming-runner hardenings, a harness supply-chain fix, an atomic claim primitive, and a dead g: election guard brought back to life. Details in "Review rounds two to four" below.
• Verification: 5,515 unit tests passing, 18 pending, 0 failing on the current head (d2a79fabe), and the full harness gate green on this branch — 85 suites.

Four of the five product fixes were found by running the new suites, not by writing them.

First of four in the stack: development ← this ← #1780 ← #1781 ← #1782. Everything that was below has merged; this targets development directly.

---

What this changes

The gate. syncthingAppsCore abandoned the whole cycle on any unmounted folder, and set syncthingInitializedSuccessfully only after that point — so syncthingAppsFirstRun latched forever, and that flag gates the g: primary election node-wide. One deleted volume stopped every masterSlave app on the node from ever electing. An unsafe mount is now an app-level fault: that app is demoted and held out of the pass, and the rest of the pass proceeds.

The backup. Refuses to archive a copy that is not synced (force overrides), checked before any stop. Holds its folders by pausing them rather than deleting them by an app-level id that matched no composed app's folder — the freeze had never once run for a v4+ app.

The restore. Acquires before it destroys: validates the request, refuses where the writer isn't, claims the app once, holds the folders, stops and verifies the containers, downloads, reads the whole archive, checks free space after the download, and only then clears and extracts. On success it apprestarts r:/s: peers — never a redeploy, hard or soft. Two of its inputs reached a root shell through tar; both are validated.

That "reads the whole archive" is a full decompress that writes nothing, and the extract later is a second one — deliberately, not by oversight. The first pass proves the archive is complete and readable and measures its true uncompressed size, all while appdata is still intact, so a corrupt or oversized backup is caught before anything is cleared. Gzip's own size footer can't stand in for it: ISIZE is stored mod 2³², so it wraps at 4 GiB and is simply wrong for a 35 GB volume, and it says nothing about integrity. The alternatives that decompress only once — into a temp dir then swap, or to a plain .tar then extract — both need the old and new copies on disk at once, up to 2× peak, and disk is the hard per-app quota on a node where CPU is not. So the restore spends a second decompress to stay within one quota's worth of space rather than risk a restore that cannot fit. On the 35 GB incident volume this roughly doubles restore wall-clock; that is the cost of validating and measuring before destroying, not a regression.

A restore keeps the owner's own archive, and only deletes the one it fetched. On success the restore removes an archive it downloaded — a transient copy it pulled for the job — but leaves an uploaded or already-local one in place: that is the owner's restore point, and deleting it after one use would destroy the very thing they may want to restore from again. The deliberate consequence is that an uploaded archive then sits on the app's volume and counts against the owner's disk quota until they remove it themselves. A UI affordance to delete a kept archive is worth adding but is its own change, not folded in here.

Syncthing is no longer restarted on a latch we cannot have set. FluxOS asked /rest/config/restart-required after changing folder config and restarted the daemon if it said yes. Validated against the syncthing source at v2.0.15 and v2.1.3-rc.2 (the relevant files are byte-identical): that flag is a one-way latch with a single write site and no reset, set only for two Options fields, auditEnabled and auditFile. FluxOS sets neither, and every folder operation it performs is handled in-process by model.restartFolder. The endpoint never answered "did my change need a restart" — it answers "has anything, ever, needed one", so any true FluxOS saw belonged to something else, hours earlier, and never cleared. On a normal node it returned false and did nothing; set once, it would have restarted the daemon on every pass — and once per folder in stopSyncthingApp — until syncthing's own supervisor gave up after four restarts in sixty seconds and exited. All three call sites are gone. Two were in the monitoring path; the third was in adjustSyncthing, which the sentinel calls every eight minutes rather than once at startup, so it had the same per-pass shape as the other two.

Mount safety is one implementation instead of three. The monitor's block, a startup sweep and the folder state machine each derived the same verdict on the same trigger over the same folders. One authority now, with the deeper phantom-index check where it belongs and the promotion-count reset it was missing. The check before a flip to sendreceive stays separate and uncached — that answers a different question at a different moment.

What running the tests found

Four product defects, none reachable by reading, unit tests or a rebase. All are in the restore's failure handling.

found by	defect
suite 87	the monitor swept an app's folders mid-restore, taking the syncthing index, peer devices and any standing safety demotion with it
suite 87	a failed folder pause did not stop the clear — appdata sits inside the replicated scope, so clearing it under a live folder broadcasts the deletions to every healthy peer
unit	one failed container stop skipped every remaining component, and nothing verified the outcome, so the clear could run with containers still writing
suite 91	the hold was scoped inside the demotion branch, so an unsynced component was restarted by the reconciler onto a half-replaced directory

The first is a direct consequence of rebasing onto this stack: the monitor's exemption for suspended apps was correct against code that deleted its own folders, and this branch's pause-instead-of-delete rewrite silently invalidated its premise. A textual merge cannot show that.

A fifth was found by the full gate, in this PR's own new code rather than in FluxOS: the container-stop guard read an unreachable docker daemon as "the container is still running", so a backup running across a dockerd restart was refused — and refusing releases the lease it was holding, handing the app back to the reconciler mid-operation. That is precisely what suite 44 exists to catch, and it caught it. The guard now uses appReconciler.dockerActual, which probes the daemon rather than pattern-matching an inspect error, so "could not ask" is no longer read as an answer.

The last two share a shape worth naming. Both protections were conditioned on something incidental — one on the sync mode, one on nothing at all — when what they guard against is simply an app running on data that is neither the old copy nor the new one. The gating on the fourth was actively backwards: a component that syncs has peers to be put right by, so holding it costs minutes; one that does not sync has no repair path at all.

Every precondition of a restore is now checked while the data it protects is still on disk.

Review rounds two to four

Round two — eight commits (servicing the 2026-08-11 review)

Each with a test that failed on the code it fixes:

commit	fix
ee0bf46d4	a denied folder pause read as "folder absent" — ERR_BAD_REQUEST spans every 4xx, so a 403 from a stale api key opened the gate a 404 is for; only a bare HTTP 404 means absent now
17762eee6	the restore failure path never checked its own receiveonly demotion; it now patches straight at the folder id, checks the answer, and leaves an undemoted folder paused and out of the resume
7cbdc214a	an app whose encrypted spec fails to decrypt was held out of mount safety entirely; the verdict is id-derived, so its sendreceive folders now come from syncthing's own list and unsafe ones are demoted the same way
9eabd4025	the HTTP status travels as a number from the one place errors are wrapped, replacing three sites that regexed it out of axios's message wording
d973bacea	the streaming runner's idle kill could not reach a sudo child from an unprivileged FluxOS; the kill now goes through sudo for runAsRoot children
bf26b35dc	a throwing onLine consumer settles the run with its error instead of hanging the awaiting operation; runCommand's doc block returns to its function
4011973c3	getVolumeInfo's doc states its real three-way contract — the code was left alone deliberately, three callers read .length unguarded and null would throw where false does not
bd8749e88	the harness's syncthing checksum line comes from gpg's verified output — the raw-file grep accepted a validly-signed checksum list from a different release with our filename appended outside the signed block, proven with a live forgery

Round three — six commits (servicing the 2026-08-24 13:36Z review)

commit	fix
de30f9f6a	the idle kill ran through a bare spawn, whose own spawn failure had no error listener and took the process down; it now goes through the wrapped command runner, which catches it
e1a906c9f	getVolumeInfo returns { error, mounts } — one shape instead of three. All six callers migrated; the two destructive restore paths refuse distinctly on mountError vs an empty mount list, and the backup path gained the guard it never had
bea8f7aa0	claiming an app for backup or restore becomes one atomic test-and-set, claimed last in the pre-try block so a validation throw never leaves a claim standing, and released in exactly one finally reached by success, unauthorized and error alike
aaa59c036	the monitor could un-pause a folder held by a live backup or restore mid-pass; the busy set is re-read at the folder write, busy folders are filtered out of foldersToWrite, and the reconciliation loop is narrowed to what was actually written
b65da19f5	a per-pass syncthing event, and suite 44 proves the monitor holds a backup's pause
10121b9a1	a missing spec is refused before anything is stopped, rather than surfacing as a TypeError deep in the flow

Round four — three commits (servicing the 2026-08-24 17:54Z review)

commit	fix
3c8a24bd0	the blocking finding. bea8f7aa0 made the busy-list getters hand out frozen snapshots, and serviceManager captured them once at boot into masterSlaveApps — a function that re-invokes itself from its own finally forever, re-passing the same empty photograph. The g: election's backup/restore guard could never fire again. All three state projections are now gone from the signature and read off globalState at the point of each decision
0e6b8ab8a	the monitor published its scan as the same Set the end-of-pass reconciliation then mutates, so one name meant two things mid-pass — across a module boundary, since appQueryService reads the published set and the mount-safety gate reads the local one. It publishes a copy now
6026e4b01	runStreamingCommand's idle message could overwrite a real error. A real error now wins the error slot, a kill-provoked exit stays idle-reported rather than surfacing as "exited with code 143", and the idle kill travels as its own flag instead of competing for the slot

Why that guard is load-bearing. A restore stops the app's containers — and "installed g: app, nobody running it" is exactly the state that makes the election want to promote a new primary. requestMasterStartWithPermissionsFix then does real work inline before the reconciler backstop ever sees the intent: claim, demote the syncthing folder to receiveonly, then a recursive chmod over the app directory — against a live restore's extraction. So the guard is exercised precisely when it matters, and the constraint it rests on is now stated at the signature: the busy-list getters return snapshots, so anything captured at call time is a photograph. They are read at the point of each decision.

Tests

Unit: 5,515 passing, 18 pending, 0 failing on the current head. Every fix has a mutation that kills its test and nothing else.

Harness: the full gate is green on this branch — 85 suites, run on a dedicated box.

Nine harness suites, six of them new, covering both ends of the incident:

suite	covers
34	one unmountable app does not stop the node electing
86	the incident: a restore stays on the node it runs on
87	per-component folders; the app-level id is never addressed
88	peers are restarted, never rebuilt; a failed unpack is held out of sync
89	real syncthing: distinct device ids, and a restore arriving on the peer by content
90	remote and upload — the branch the incident actually took
91	a restore with no peer to fall back on
92	a legacy v≤3 app, which has no compose array
93	the backup gate — the check that stops a bad archive existing at all

Suite 89 is the first thing to boot a real syncthing daemon in this harness (createTestEnv({ syncthing: 'binary' }), pinned to v2.0.15, the version the fleet runs). It exists because the stub moves no files: a green 86–88 proves nothing was destroyed, not that anything arrived.

Suite 93 covers the other end of the incident. The archive that destroyed the world was 373 bytes, taken from an instance that had never synced; the gate that refuses to make such an archive had unit tests and no end-to-end coverage at all — suite 44 drives a backup, but tests the lease. It asserts the incident's own shape, a partial copy, the force override, and, because the gate deliberately runs before anything is stopped, that a refusal leaves the app running.

Harness additions

• A syncthing binary profile. The binary is installed at build time and verified against the release signature — a vendored, fingerprint-pinned key signs the checksum list, which covers the tarball. The verdict is read from --status-fd, not gpg's exit code: the release carries a second signature from a retired key, so gpg --verify exits 2 on a perfectly good release.
• SYNCTHING_PATH now means what FluxOS means by it. It was set on every harness node including the legacy ones, so FluxOS's own supervision path had never been exercised by any suite.
• The stub records folder config writes in order, per node. Folder config was stored as current state, so a pause and its resume cancelled out, and "did this operation hold the right folder still" could not be asked.
• The external HTTP stub serves arbitrary artifacts, so a node can fetch a real archive over a real socket from inside the subnet — and can be told to promise more bytes than it delivers.
• buildSeedableLegacyApp emits a v≤3 spec. seed-helper built v8 and only v8, so no suite could produce an app that takes the legacy branch.

Notes for review

• A g: component is paced back up after a backup or restore on this branch, and that is deliberate. Neither task writes desired state, so the reconciler is what restarts the component it stopped — but recordRestart here counts every restart, so the task's own stop earns a rung on the backoff ladder and the restart waits it out. The ladder resets when the previous run provably lasted STABLE_RUN_MS, so a long-running app is unaffected; back-to-back operations are not. Reconciler: the only actuator for every operator command, and a ladder that paces faults not operators #1780 is where a rung is earned by evidence of a fault rather than by any stop.
• The external API is unchanged — same routes, same body, same streamed lines. Additive only, no frontend change needed.
• force is API-only by design. The UI never checks HTTP status, so a refusal renders as a green success toast and the caption truncates at 50 characters; refusal messages lead with the verdict for that reason. The UI can never send force, which is the right shape for a destructive override.
• No ratio gate on archive size. Refusing "this archive is much smaller than what is on disk" blocks a legitimate rollback to an early checkpoint, and since the UI cannot send force a wrongly-blocked restore would be a dead end. The numbers are logged. What stops a bad archive existing is the backup gate.
• buildSeedableLegacyApp and the artifact store are additive — no existing suite changes behaviour because of them.

[codecov]

Codecov Report

❌ Patch coverage is 80.75601% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.65%. Comparing base (e4a00d2) to head (1d82114).
⚠️ Report is 219 commits behind head on refactor/monitoring-single-store.

Files with missing lines	Patch %	Lines
ZelBack/src/services/IOUtils.js	0.00%	26 Missing ⚠️
...ack/src/services/appLifecycle/advancedWorkflows.js	92.85%	16 Missing ⚠️
...ack/src/services/appMonitoring/syncthingMonitor.js	66.66%	13 Missing ⚠️
ZelBack/src/services/backupRestoreService.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           refactor/monitoring-single-store    #1779      +/-   ##
====================================================================
+ Coverage                             61.45%   64.65%   +3.20%     
====================================================================
  Files                                   158      174      +16     
  Lines                                 30902    32526    +1624     
====================================================================
+ Hits                                  18990    21030    +2040     
+ Misses                                11912    11496     -416     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Cabecinha84]

I pulled the PR, diffed it against its actual base (refactor/monitoring-single-store @ d1d0c2e, which is the
merge-base — so what I reviewed is exactly this PR's diff), read all ~880 lines of product change plus the
harness/test additions, verified every cross-module API it calls, and ran the affected unit suites in a throwaway
worktree.

Verdict: safe to ACK

Nothing I found is data-loss-unsafe, and the central safety argument holds under scrutiny. I have four comments I'd
post alongside the ACK — two of them I'd genuinely like addressed, none of them blocking.

State of the PR right now: no human reviews, one codecov bot comment. CI green (build, codecov patch 80.76% / project,
GitGuardian). 33 commits, last pushed 2026-08-07.

Tests I ran myself (worktree on 2859909, since removed — your tree is untouched):

• advancedWorkflows.test.js — 115 passing
• syncthingMonitor + syncthingFolderStateMachine + IOUtils + backupRestoreService + appReconciler — 203 passing

The harness claims (10 suites green on b5a710f, 82/82 at the top of the stack) I could not verify — they need the
docker harness.

---

What I verified as sound

The load-bearing claims check out against the code, not just the prose:

• Acquire-before-destroy ordering is real. Validate → FDM guard → claim → pause folders → stop and verify containers →
  download → verify byte count → inspectTarGz the whole archive → free-space check → only then clear + extract. Every
  throw before swapInFlight = target leaves appdata untouched.
• The double-claim window is genuinely closed. globalState.restoreInProgress.includes() is re-read with no await
  between the check and the push.
• The cache writes land where they're read. appCaches.receiveOnlySyncthingAppsCache is literally
  globalState.receiveOnlySyncthingAppsCache (same Map object), so the settle/demote writes are visible to the folder
  state machine. This was the obvious way for the fix to be silently inert, and it isn't.
• apprestart doesn't fan out. executeAppGlobalCommand(..., undefined, true) builds /apps/apprestart/ with no
  :global segment → each peer restarts locally only. And bypassMyIp is set.
• The mount-safety dedup is not a weakening. The state machine's removed block fired on syncthingAppsFirstRun ||
  erroredFolderIds.has(appId) — identical to the monitor's new appsToVerify trigger set — and the monitor now applies
  the deeper verifySendReceiveFolderSafety where the folder is currently sendreceive. Strictly stronger. The dropped
  mountSafetyBlocked/blockedReason/blockedAt cache fields are read nowhere in the tree.
• getVolumeInfo no-match returns false, so if (!volume) correctly guards the volume[0].mount deref.
  Number.isFinite(false) is false, so an unknown remote size doesn't trip the short-download check.
  downloadFileFromUrl's filename (backup_${component.toLowerCase()}.tar.gz) matches the computed archivePath.
• inspectTarGz shell use is safe — execFile argv, path arrives as "$1", never as syntax. The sized === 0 guard against
  a different tar column layout is a good catch.
• No new circular top-level requires from pulling syncthingFolderStateMachine into advancedWorkflows.
• The Dockerfile signature chain is correct: pinned fingerprint asserted via VALIDSIG on --status-fd, then sha256 read
  out of the signed file. The deliberate no-pipefail / no-exit-code choice is right and documented.

---

Comments I'd post

1. inspectTarGz and getDirectorySizeBytes inherit the 15-minute default child-process timeout. serviceHelper.js:24
   sets MAX_CHILD_PROCESS_TIME = 15 * 60 * 1000 and runCommand applies it unless overridden — neither call overrides it.
   tar -tzvf inflates the entire archive; on the 35 GB Palworld-class app this PR exists for, that can exceed 15 minutes
   on node-grade disks. On timeout you get result.error → "archive is unreadable" → refusal, after the app is stopped and
   the archive downloaded. Fail-safe direction, but the largest apps — the ones this was written for — could become
   un-restorable with a message that misdescribes why. I'd pass an explicit generous timeout to both.

2. The backup gate can't tell "no such folder" from "syncthing didn't answer". getFolderSyncCompletion returns null
   for both a 404 and a failed/unreachable API call. The refusal then asserts "${componentName}: no syncthing folder -
   this instance has never synced". Combined with force being API-only by design (which I agree with), a transient
   syncthing hiccup makes backup impossible from the UI, and the message tells the operator something false about their
   data. Worth separating the two, or at least retrying before refusing.

3. Cosmetic but it's the message at the moment of blocking: for globalBytes === 0, syncPercentage defaults to 100
   while isSynced is false, so the refusal reads "100.00% synced (0/0 bytes)".

4. The PR description says "Both call sites are gone" for the restart-required latch — there's a third.
   syncthingService.adjustSyncthing() (~line 2358) still does getConfigRestartRequired() → systemRestart(). Same
   reasoning applies to it. Startup-only and once, so the harm is far lower than the per-pass loop you removed — but
   either drop it too or correct the claim.

Asymmetry worth a question, not a change: backup deliberately refuses to restart g: components ("starting it here
would put a second writer on the shared volume"), while restore ends with a plain appDockerStart(appname), which
starts every component including elected ones. That's pre-existing, and the FDM guard covers the normal case — but the
guard is explicitly skipped when FDM is unreachable (fdmOk === false), which is exactly the path where restore then
starts a g: container here anyway. The PR just established the opposite rule on the backup side.

One operational note: a syncMode: 'none' component held by setControllerDesired(id, 'stopped', 'restore did not
complete') has no automatic release — the only caller of clearControllerDesired is an operator appstop. So the
recovery is stop-then-start. That's the intended trade (the comment argues it well), but the reason only reaches the
log and a stream that has already ended.

Nits: two lint-visible blank-line issues introduced in syncthingMonitor.js (double blank ~611, padded block before }
catch ~824 — CI doesn't run lint, so harmless); getVolumeInfo's new JSDoc says "null when no matching mount is found"
but that path returns false; sudo bash -c is a new binary in the sudo surface (safe as written, just new); and keeping
local/upload archives after restore leaves them inside the syncthing folder root (${appsFolder}${appId}, not
appdata), so they replicate to every peer — deliberate and arguably correct, but a storage-footprint change worth
stating.

---

Stack caveat: this is fifth in development ← #1774 ← #1775 ← #1778 ← #1777 ← this. An ACK here is an ACK of this diff;
it isn't mergeable to development until the four below it land.

[MorningLightMountain713]

Thanks for the ACK and the four notes — all four are addressed. Branch is now 2d55291f4.

1. The 15-minute child-process timeout — fixed, but not by raising it (4c2159b88). A larger number is still a clock, and these two operations scale with how much data they were given, so a total cap can only ever kill the largest apps: the ones this path exists for.

Both now stream. runStreamingCommand consumes output as it arrives and takes an idleTimeout, so a command is stopped once it has produced nothing at all for that long. A 35 GB archive listing for an hour completes; a read that has stalled does not. That is also why the directory walk drops -s — du reports each directory as it passes and only its last line is the total, so summarising would leave nothing to observe.

Two things fell out of it. Nothing is held whole any more, so the maxBuffer ceiling that shaped the old pipeline is gone; and with the aggregation done in-process there is no shell left at all — root was the only reason these are child processes, and root wants sudo, not an interpreter. A path is now an argument that cannot be read as syntax rather than one carefully kept away from it. A killed child also says which limit it reached, so "no progress" can no longer reach an operator dressed as a corrupt archive.

2. "No such folder" versus "syncthing didn't answer" — fixed (8db4fe669). Only an HTTP status proves syncthing replied, so that is what separates them: a plain 404 is the folder being absent; a transport failure, a 500 or an unreadable api key are unknown.

The refusal stands in both cases — an archive of a copy nobody verified is the one that looks fine now and loses data when it is restored months later, so "cannot tell" must not open the gate. What changes is what the operator is told. I deliberately did not add a retry: it would hide the condition rather than report it, and the honest message plus the API-side force already gives a route.

3. "100.00% synced (0/0 bytes)" — fixed (50018dbe6). With nothing in the global index there is nothing to be a fraction of, so the message now says the index is empty rather than quoting a number that stands for the absence of one. The percentage is untouched where it measures something.

4. The third call site — removed (ccff41ea6), and one correction worth carrying: it is not startup-only. adjustSyncthing is called from runSyncthingSentinel every eight minutes, so it had exactly the same per-pass shape as the two already removed — once anything latches the flag, it restarts the daemon on every pass until syncthing's own supervisor stops retrying and leaves it down. The harm was the same, not lower. Nothing replaces it, for the same reason as the other two.

One thing the gate caught in my own change. Suite 93 modelled "this instance has never synced" with an unreadable folder status and asserted that wording — so it was pinning the very conflation note 2 removes, and went red the moment the two were told apart. It now covers both shapes as they are (2d55291f4): an erroring daemon asserts sync state could not be determined and explicitly that the refusal makes no claim about the data; an empty index asserts the index is empty rather than the vacuous 100%.

The asymmetry you raised as a question — restore starting g: components where backup refuses to — is real and pre-existing, and I have left it rather than fold a second rule change into this PR.

Verified at the top of the stack: 85 of 85 suites green at f6c6bc399.

[Cabecinha84]

PR #1779 — review of the current state (head 4b02282)

Verdict: ACK-able, but I'd ask for two one-line fixes first. Both are in failure paths, both are the same class of bug
this PR exists to close, and both are cheap. Everything else is nits or follow-ups.

---

Where it stands vs. your last review

All four of your notes are genuinely fixed — I checked the code, not the reply:

┌─────────────────────────────────────────┬───────────────────────────────────────────────────────────────────────┐
│ your note │ state │
├─────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────┤
│ │ Fixed better than asked. New serviceHelper.runStreamingCommand │
│ 15-min child-process timeout on │ streams output and takes an idleTimeout (5 min of silence, not of │
│ inspectTarGz/getDirectorySizeBytes │ total runtime). sudo bash -c is gone entirely — sudo tar/sudo du as │
│ │ argv, no shell, no maxBuffer ceiling. │
├─────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────┤
│ │ Fixed. probeFolderSyncCompletion pulls the HTTP status out of the │
│ "no such folder" vs "syncthing didn't │ error and only treats a plain 404 as absent; everything else is │
│ answer" │ unknown. Distinct operator messages. Refusal stands in both cases, │
│ │ deliberately. │
├─────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────┤
│ 100.00% synced (0/0 bytes) │ Fixed — empty index now gets its own wording. │
├─────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────┤
│ third restart-required call site in │ Removed. And his correction is right, I verified it: │
│ adjustSyncthing │ syncthingService.js:2535 calls adjustSyncthing() from the sentinel │
│ │ every 8 minutes, not once at startup. Your note undersold the harm. │
└─────────────────────────────────────────┴───────────────────────────────────────────────────────────────────────┘

The lint nits you raised are also gone — npx eslint now exits 0 on all five changed product files.

CI

The red check is not the code. Run 31510268949 failed at "Push hashes to fluxhashes" (error: failed to push some refs
to .../fluxhashes.git) — a repo-permission step that runs before npm install and the test suite. Tests never ran. The
previous commit 2d55291 was green, and the only delta to head is 4b02282, which touches one harness file
(93-backup-sync-gate.js) and no product code.

I ran the suite locally against head: 5175 passing, 23 failing — all 23 in dockerService.test.js, which needs a live
docker daemon and the runonflux/website container CI provides and I don't have. Nothing from this PR fails. Just
re-run the workflow.

---

Two things I'd ask him to change

1. setSyncthingFolderPaused reads any 4xx as "folder absent" — advancedWorkflows.js:~1938

if (response.data?.code === 'ERR_BAD_REQUEST') {
// 4xx: syncthing has no such folder. Nothing is replicating it...
return 'absent';
}

I checked axios 1.13.6: ERR_BAD_REQUEST is the code for 400, 401, 403, 404 and 409 alike (ERR_BAD_RESPONSE starts at
5xx). So a 403 — stale cached API key (the axios instance caches it for 15 minutes), or CSRF — reads as "nothing is
replicating this, proceed". The folder is never added to pausedFolderIds, no refusal is raised, and the restore goes
on to removeDirectory(appDataPath, true) with a live sendreceive folder over it. Those deletions then go out to every
healthy peer. That is the incident mechanism.

What makes this worth blocking on: he already built the correct discriminator two commits later, for your note 2 —
probeFolderSyncCompletion reads the actual HTTP status and only lets a bare 404 mean "absent". This call site just
didn't get it. Same fix, and the message is already available on response.data.message.

(Backup is shielded by the sync gate running first — a 403 there yields unknown → refusal. But under force it falls
through to the same hole.)

2. The restore failure path discards changeSyncthingFolderType's return value — advancedWorkflows.js:~2848

In the catch, when a component's appdata was cleared but not fully unpacked:

await changeSyncthingFolderType(swapInFlight.folderId, 'receiveonly'); // returns boolean, ignored
globalState.receiveOnlySyncthingAppsCache.set(...);
appReconciler.setControllerDesired(swapInFlight.folderId, 'stopped', 'restore did not complete');
// ...then every paused folder is resumed

changeSyncthingFolderType pre-reads getConfigFolders() and returns false if that read fails or the folder isn't
matched by path. That's the exact shape the monitor deliberately refuses to use, in its own words at
syncthingMonitor.js:~577: "a safety action must not be conditioned on a fallible read whose failure silently reads as
'nothing to protect' (that exact silent no-op once cost a gate run)". The monitor patches directly by id and checks
the status. The restore's failure path uses the pre-reading variant and checks nothing.

If it returns false, two protections collapse together:

• The folder stays sendreceive, and the loop right below resumes it → the half-restored appdata is broadcast to the
  peers.
• The container hold dies with it. I traced this: with the folder still sendreceive, manageFolderSyncState
  short-circuits on folderAlreadySyncing (syncthingFolderStateMachine.js:1141) straight into ensureContainerRunning,
  which for an r: component calls setControllerDesired(appId, 'running', ...) (:1112) — overwriting 'stopped', 'restore
  did not complete' on the very next monitor pass. The container starts on the wreckage.

Suggested shape: patch { type: 'receiveonly' } directly by folderId as the monitor does, check the status, and if it
can't be demoted don't resume that folder — leave it paused and log loudly. A paused folder that the monitor later
resumes is a far better outcome than a sendreceive one resumed immediately.

---

Non-blocking

3. getVolumeInfo still returns false on no-match (IOUtils.js:272) while the new JSDoc says "null when no matching
   mount is found" — only the catch became null. The doc mismatch you flagged last time is still there. Functionally
   harmless: both new call sites use if (!volume) and backupRestoreService now uses !dfInfoData || !dfInfoData.length.
4. Empty restore selection isn't refused. If every item arrives restore: false, targets is [] and the task still stops
   the app, waits, and starts it — an outage that restores nothing. One early throw.
5. probeFolderSyncCompletion regexes the HTTP status out of axios's message string (/status code (\d{3})/). Correct
   for 1.13.6 and it fails safe (everything becomes unknown → refusal) if the wording ever changes, but it's coupling to
   a message format. Carrying error.response.status through performRequest would be structural.
6. runStreamingCommand's idle kill may not reach a sudo child — worth him verifying on a real node, I couldn't test it
   here. child.kill() signals the sudo process, which execs with real uid 0; an unprivileged parent usually gets EPERM,
   and Node then emits 'error' on the child. The promise still resolves with an error, so the verdict is right and
   fail-safe, but the tar/du may keep running. Note this applies equally to the existing runCommand's timeout for
   runAsRoot calls — not a regression.
7. runStreamingCommand's JSDoc was inserted between runCommand's doc block and runCommand itself, so there are now two
   stacked doc comments and runCommand is undocumented. Cosmetic.
8. If onLine throws it escapes a stream 'data' handler — unhandled, and the promise never settles. Both current
   callers are throw-free; a try/catch would make that structural rather than incidental.
9. First-run mount-safety coverage narrowed slightly. The removed startup sweep iterated syncthing's own folder list;
   the replacement iterates installed apps. A sendreceive folder whose app spec can't be read (ownedByUnreadableApp) is
   now neither verified nor swept on the first pass. Narrow, but it's a real gap that didn't exist before.
10. Harness only — the sha256 line is grepped out of the raw .asc, not out of gpg's verified output, so text outside
    the signed block would also be matched. It fails closed (two lines for one filename makes sha256sum -c fail), so it's
    safe as written; gpg --decrypt | grep is the cleaner form. The signature chain itself is correct: pinned fingerprint
    asserted via VALIDSIG on --status-fd, and the deliberate no-pipefail choice is right and well documented.

Follow-up, not this PR: the asymmetry he acknowledged — backup refuses to start g: components, restore's
appDockerStart(appname) starts everything — is still there, and the FDM guard that would cover it is explicitly
skipped when fdmOk === false, which is exactly the path where restore then starts a g: container anyway. Pre-existing,
correctly out of scope here, worth an issue.

[MorningLightMountain713]

All seven serviced. Six of them turned out to be more than one-liners, so each has a test that failed on the code it fixes.

1. The idle kill could crash the process — fixed (de30f9f6a). You were right that the bare spawn('sudo', ...) had no 'error' listener, and the trigger (EAGAIN under the same fork pressure that makes a du/tar idle out) is exactly when it would fire. Rather than add a listener, the kill now goes through serviceHelper.runCommand, which already prefixes sudo and wraps its spawn in a catch — a kill that cannot fork resolves an error instead of throwing an unhandled event, so the worst case is the orphan surviving, never the node dying.

2. getVolumeInfo's return type — fixed properly (e1a906c9f). It was returning three types into one slot — array on success, false on no-mount, null on read failure — and its six callers each guessed differently: three read .length (crash on null), two tested !x (which an empty array would have broken), one was correct, and the backup path read [0].mount with no guard at all. It now returns { error, mounts } like runCommand: an empty mounts is the answer "not mounted", a set error is the failure to answer. Every caller destructures it; the two destructive restore paths refuse distinctly on a read failure rather than reading it as absence; the backup path gained the guard it never had. The function had no return-shape test — all three states are pinned now.

3. The backup double-claim — fixed architecturally (bea8f7aa0). Same class as 307a414b9, one file over, and the restore's own fix for it was a re-read that is itself a TOCTOU band-aid. Both now claim through globalState.tryStartBackup/tryStartRestore: a synchronous test-and-set the event loop runs to completion before the next request, so a second caller finds the app taken — no early check, no re-check. Released once each in a finally. The lists stay the observable busy-state the monitor, election and reconciler read, but the getters now hand out a frozen snapshot so only the claim primitives can write.

4. The monitor un-pausing a folder mid-operation — fixed (aaa59c036). Real, and given daily backups it is regularly exercised, not insurance. The monitor writes every folder with paused:false and treats any paused folder as drift; the only thing that sets paused:true is a live backup or restore. The per-app skip catches an app already busy at its turn, but one that goes busy DURING the pass was processed as free and its folder is in the batch. The busy set is now re-read at the write and those folders are held back — the guard at the action it guards, not only at the loop that feeds it. The monitor only ever drops work for a busy app, never waits on it, so it cannot block an operation; a crashed op's orphaned pause holds no claim and is still cleaned up. A new syncthing:passComplete event (inert in production) lets suite 44 wait for a real pass inside the backup window and assert it never wrote the held folder — written and registered, but NOT YET RUN on cindy (see the gate note below).

5. The kept archive — stated (PR body). Deliberate: the restore removes an archive it downloaded but keeps an uploaded or local one, because that is the owner's own restore point. The body now says plainly that a kept archive sits on the volume against the owner's quota until they remove it, and names a delete affordance as a follow-up rather than folding it in.

6. The double decompress — stated (PR body). Deliberate and correct for this platform: the first pass validates the archive is whole and measures its true uncompressed size while appdata is still intact, so a corrupt or oversized backup is caught before anything is cleared; gzip's ISIZE wraps at 4 GiB and says nothing about integrity, so it can't stand in. The alternatives that decompress once both need old and new on disk at once — up to 2× peak — and disk is the hard per-app quota where CPU is not. The body now states this so it isn't read as a regression.

7. The two minors. The backup's missing-spec path is fixed (10121b9a1): it read the spec and handed it straight to syncedComponentsOfApp, which threw Cannot read properties of null; it now refuses with "no specifications found" the way the restore does, before any stop. The all-false restore, though, already refuses before doing anything — hasTrueRestore throws No restore jobs... in the validation block, before the main try and before any appDockerStop, so the app is never stopped.

Unit over all six new commits: green. All of it is subject to the full harness gate, which is queued and will run this evening — cindy is in use by another session right now, which is also why the suite-44 assertion above is written but not yet executed. The gate is the real bar here: the branch now carries the reworked claim path, the monitor write-time guard, a new per-pass event, and the { error, mounts } contract across the file operations, on top of the round-one and round-two work.

[Cabecinha84]

PR #1779 — review of the current head (10121b9)

Verdict: do not ACK yet. One blocking fix, ~3 lines. Everything else is sound.

Note first: the PR body describes the head as bd8749e, but there are 6 newer commits on top (de30f9f → 10121b9)
servicing your 2026-08-24 round. The blocker is in one of them.

What I did

Fetched refs/pull/1779/head, diffed against the real merge-base (383d8ae), read all product changes, traced every
cross-module contract the new code depends on, and ran the affected suites in a throwaway worktree (since removed).

• Unit: 505 passing, 0 failing across advancedWorkflows, syncthingMonitor, syncthingFolderStateMachine, serviceHelper,
  IOUtilsMeasurement, fileQueryService, syncthingService, appQueryService, volumeReadersDoNotFollowLinks.
• Lint: clean on all changed product files. (The one prefer-destructuring error in globalState.js:70 is pre-existing
  on development — not this PR.)
• CI: green (build + GitGuardian).
• The 85-suite docker harness I cannot run here — the harness claims remain unverified by me, as before.

🔴 Blocking: bea8f7a killed the masterSlaveApps backup/restore guard

Making the getters hand out a frozen snapshot was the right call for the claim primitives, but it silently broke the
one caller that captures the getter once and keeps it forever.

serviceManager.js:646-647 passes globalState.backupInProgress / restoreInProgress into masterSlaveApps. That function
is a self-recursing loop — it re-invokes itself in its own finally (advancedWorkflows.js:4691) passing the same two
references on for the life of the process. Before this PR they were the live backing arrays, so .some() at
advancedWorkflows.js:4187-4188 reflected current state on every 30-second cycle. Now they are a frozen copy of an
empty array, taken once at boot.

Reproduced directly:

captured at boot: []
tryStartBackup: true
live getter now : [ 'palworld' ]
captured ref now: []
masterSlaveApps backupSkip would be: false ← the guard is dead
frozen? true

Consequence. The g: election no longer skips an app under backup or restore. The good news is that the container start
is backstopped: appReconciler.isManagedElsewhere (appReconciler.js:355-356) re-reads the getter fresh each call, so
the reconciler still refuses to actuate. But requestMasterStartWithPermissionsFix (advancedWorkflows.js:2278) does
real work inline, before it ever hands the intent to the reconciler:

1. appReconciler.claimStarting(appname)
2. changeSyncthingFolderType(appId, 'receiveonly') — demotes the folder mid-restore
3. applyPermissionsFix(appId) — chmod -R 777 recursively over the app directory while the restore is untarring into
   appdata
4. only then setControllerDesired(...)

So a g: app being restored can have its syncthing folder demoted and a recursive chmod raced against the extraction.
Not the 35 GB catastrophe, but it is a deliberate safety guard in the exact incident family this PR exists to close,
now dead by accident — and dead silently, which is the property that makes it worth blocking on.

Why no test caught it: the unit tests call masterSlaveApps with literal arrays
(tests/unit/advancedWorkflows.test.js:626), so they exercise the parameter, never the wiring that feeds it.

Fix. Read the busy set from globalStateParam inside the loop rather than from the captured parameters — the same shape
the monitor already uses at its folder write in aaa59c0 ("the guard belongs at the action it guards"):

const backupSkip = globalStateParam.backupInProgress.some((item) => installedApp.name === item);
const restoreSkip = globalStateParam.restoreInProgress.some((item) => installedApp.name === item);

The two params then become vestigial (drop them, or leave them for the tests). The should skip apps in backup progress
test needs to claim via globalState.tryStartBackup(appName) instead of passing an array — which makes it a better
test anyway, since it would then actually cover the wiring.

Worth a grep pass for the same shape: serviceManager.js:646-647 is the only long-lived capture I found, but the class
of bug is "getter captured once into a long-running loop", and the freeze made every such site silently stale rather
than loudly wrong.

What I verified as correct

Everything else in the six new commits holds up:

• de30f9f — the bare spawn is gone; the idle kill goes through runCommand, which catches its own spawn failure. The
  unhandled-'error' crash path you flagged is closed, and closed better than the listener you suggested.
• e1a906c — getVolumeInfo → { error, mounts } is done properly. All six callers migrated (fileQueryService:29,
  fileSystemManager:319,394, backupRestoreService:87, advancedWorkflows:2455,2688,2758). The two destructive restore
  paths refuse distinctly on mountError vs !mounts.length; the backup path gained the guard it never had. No
  .length-on-null left anywhere.
• bea8f7a (apart from the above) — tryStartBackup/tryStartRestore are genuinely synchronous test-and-set, claimed
  last in the pre-try block so a validation throw never leaves a claim standing, released in exactly one finally
  reached by success, unauthorized and error alike. addToRestoreProgress/removeFromRestoreProgress were correctly
  rewired to the primitives.
• aaa59c0 — the mid-pass un-pause you raised as item 4 is properly closed: busyAppNames re-read at the folder write,
  busy folders filtered out of foldersToWrite, and the promotedFolderIds reconciliation loop correctly narrowed to
  what was actually written.
• bd8749e — the supply-chain fix is real. Reading the checksum line from gpg --decrypt output rather than the raw
  file defeats append-outside-the-signed-block, and the tarball filename carries the version, so cross-release
  substitution fails on the filename too.
• 10121b9 — if (!appDetails) throw before anything is stopped. Correct placement.
• The monitor's id derivation is consistent end to end: checkAppFolderMounts builds appIds via
  dockerService.getAppIdentifier, the unsafeFolderIds skip tests the same, and unreadableFolderEntries uses folder.id,
  which is that identifier. No mismatch.
• verifyAppFolderMountWithRepair(…, sending) passes the mount root, which is what syncthing's folder path actually is
  — so the deeper phantom-index check walks the same tree the old state-machine block did. Not a behaviour change.
• The three restart-required call sites are gone, including adjustSyncthing.

Non-blocking notes

1. syncthingMonitor.js — promotedFolderIds aliasing. globalState.promotedFolderIds = sendingFolderIds assigns the same
   Set the mount check reads, and the reconciliation loop then mutates it. Benign today (the mount check runs earlier
   in the pass, and the next pass rebuilds it), but it makes sendingFolderIds mean two things. A defensive copy on
   assign would cost nothing.
2. runStreamingCommand — the idle message can mask a real error. In finish, idleKilled wins over the passed error, so
   if a consumer throws and the idle timer had already fired, the operator is told "produced no output" instead of
   what actually failed. Narrow, log-quality only.
3. PR body is stale. It documents bd8749e as head and the round-two table stops there; the six commits servicing
   your last review aren't described. Worth a refresh before merge so the body matches what's being merged.
4. getDirectorySizeBytes returning null understates room (room = available + (appDataBytes ?? 0)), refusing a restore
   that would have fit. This is documented as the deliberate safe direction and I agree with it — noting it only so it
   is a stated choice rather than a discovered one.

[MorningLightMountain713]

Thanks — the blocker is real, and it is a regression this PR introduced. I verified every link myself rather than taking the reproduction on trust, and the one premise you asserted without proof turned out to be the load-bearing one: the pre-freeze getter did return the live backing array, so this is genuinely ours and not a pre-existing latent bug.

I also think the review understates it. A restore stops the app's containers, and "installed g: app, nobody running it" is exactly the state that makes the election want to promote a new primary. So the guard is not merely dead in general — it is dead precisely in the window it exists for.

The blocker — fixed as architecture, not as the three-line read (3c8a24bd0)

Your patch was correct and would have worked. I did the larger version because leaving two vestigial parameters behind means every future reader has to be told why they are there, and the failure class is "a getter result stored into something long-lived", not "these two particular parameters".

All three state projections are gone from the signature:

masterSlaveApps(globalStateParam, installedApps, listRunningApps, https)

The busy lists and receiveOnlySyncthingAppsCache are now read off globalStateParam at the point of each decision. That is also strictly fresher than the old per-call capture ever was: an app that goes busy mid-pass is now caught at its own turn rather than at the next cycle. A comment at the signature and at the serviceManager call states the constraint — the getters hand out snapshots, so anything captured at call time is a photograph.

The sweep you asked for, done. Every reader of both getters: appQueryService (reads the getter inside the per-request function), syncthingMonitor ×3 and syncthingHealthMonitor (these receive the globalState module, so property access hits the getter per read), appReconciler (fresh each call, as you found), and the two softRedeploy skips (read at use). serviceManager → masterSlaveApps was the only capture-once site in the codebase. Worth recording that the adjacent receiveOnlySyncthingAppsCache capture on the same call was safe only by luck — it is a plain mutable object rather than a snapshot getter — which is why it now reads off the module too rather than staying correct by accident.

Why no test caught it, and what now does. You identified this exactly: the old backup-skip test passed literal arrays, so it exercised the parameter and never the wiring. That is the whole reason a dead guard stayed green. The test now claims through the real globalState.tryStartBackup and releases via finishBackup in a finally, with a restore twin through tryStartRestore — the direction the incident actually took. The election fixture seeds the real globalState.receiveOnlySyncthingAppsCache (entry objects are shared, so tests that inspect their own maps still observe production's in-place mutations; I checked that production mutates entries rather than replacing them inside masterSlaveApps).

Mutation drill: guard neutered → both tests fail; restored → both pass. Unit suite: 5,501 passing, 18 pending, 0 failing.

And the wiring class that unit stubs cannot see got an end-to-end proof. Harness suite 35 gained a fourth test: elect a primary, wait Up, bulk 200MB into appdata, take counter baselines after a two-cycle settle so the staging start cannot race them, then start a real backup — whose stop phase takes the app down, which is the exact state a dead guard would promote through. It asserts that the masterSlave:decision/<app>/skippedBusy counter increments, that the started arm stays flat, that no masterSlave:started event is emitted, and that the app returns after release. The counter form is deliberate and self-proving: it can only increment if an election pass actually ran and considered the busy app, so a vacuous pass cannot produce a green. This test ran in the full harness gate and passed 4/4 on its first execution, so the fix is fleet-proven and not just unit-proven.

Note 1 — the aliased Set, fixed (0e6b8ab8a)

Fixed, and it was worse than benign. appQueryService.js:341 reads the published set from outside the pass, and checkAppFolderMounts — the mount-safety gate — reads the local one at line 584. So the two names genuinely diverged in meaning mid-pass across a module boundary, not only within the monitor. The monitor now publishes a copy: observation and publication are different objects, and the reconciliation loop mutates only what was published. One line plus a constraint comment.

Note 2 — the masked error, fixed (6026e4b01), with one correction

Fixed, but not quite the way the note suggests, and the difference matters. After an idle kill, the child's non-zero exit is the kill's own consequence — so letting that exit code win the slot would report "exited with code 143" and be strictly worse than the idle message. The shape now is: close() passes no error when idleKilled (a kill-provoked exit stays idle-reported), finish() lets any real error win the slot, and res.idleKilled carries the kill as its own fact unconditionally rather than competing for the error slot at all.

New test proves the masking case end to end — idle timer fires, buffered data arrives, consumer throws, and the real error is what surfaces with the flag also true. Mutation drill: restore the old precedence and that test fails with exactly the old masking. Flag assertions were added to the two existing idle tests.

Note 3 — the stale body

Correct, and fixed with this push. The body now describes the real head and carries the two review rounds that were missing from it.

Note 4 — the null-size understatement

Agreed on both counts, and it stays as it is. Recording it here so it is on the record as a chosen direction rather than a discovered one.

The harness gate — it ran, and what it found

The 85-suite claim in the old body was from the pre-rebase lineage. The full gate has now run on this stack, on a second dedicated box, at the top of the stack:

RESULT  suites_pass=83 suites_fail=4 FAILED:[ 44 62 72 92 ]

87 suites, 75 minutes. All four reds are test defects. None is a product defect, and every product fix in this PR passed everywhere it was exercised. Three of the four were tests executing for the very first time — which is the honest cost of a stack that carried never-run suites.

suite	what was actually wrong
44	the monitor-hold test polled nodeIp(0). Harness node numbers are 1-based, so .9 is an address nothing occupies and its write log reads empty forever. The product did everything right, proven from the captured node log: setSyncthingFolderPaused … paused=true landed on time and the monitor skipped the held app on every 3s pass
62	the suite's before() waited for exactly 2 samples while the strict thinned < full assertion needs ≥3 (hourly-thinning a sub-hour series keeps first+newest=2). Raced from birth, not a loaded box; it only ever passed when the intervening tests happened to span a sampler tick. The assertion itself is untouched
72	the Aug-10 signal rework deleted const STAGING_UUID and missed two tests that use it not as a wait signal but to plant fake .flux-op-<uuid> staging dirs as their sweep-actually-ran positive control. Both threw ReferenceError on first-ever execution. The const returns as a documented fixture
92	buildSeedableLegacyApp hardcoded height = 2100010 while this run's chain started at 2200000, so the seeded legacy app was expired before the fleet's first block — caught by the harness's own assertAliveOnThisChain guard. It now follows env the way buildSeedableApp already did

Suite 72's fix lands on #1781, which owns the commit that deleted the constant; the other three are on this PR. That keeps each PR's diff self-contained across force-pushes and independent merges.

The targeted re-run found one more, in the same suite. 62, 72 and 92 all went green — including suite 92's restore tests, which had never executed before. Suite 44 got past its fixed wait for the first time and failed on the next assertion down, which is exactly what a first execution is for.

It is a third test defect, not a product one. The stub's folder-write log is cumulative, and suite 44's two tests back up the same app on the same node with no reset between them — so the second test read the first's paused:true/paused:false pair. That made its hold-wait vacuous (satisfied before its own backup took any hold) and then failed its assertion with the previous backup's legitimate resume — the one appendBackupTask performs by design after the archive is written and before it clears the busy flag, so redundancy is restored at the earliest safe moment.

The node log settles it: exactly one pause pair in the window, both the first test's, and on every monitor pass in between FluxOS logged keeping folder … unprocessed this pass. The property the suite exists to prove — the monitor does not un-pause a folder a backup is holding — held throughout, and the pass.data.wrote assertion that tests it directly passed. The fix is resetFolderWrites(), which sibling suites 87 and 88 already call for this reason, plus moving the event-id capture inside the window (taken before the backup started it could be satisfied by a pass that completed before the folder was ever paused). Neither assertion was weakened.

With that fix, suite 44 is 2 of 2 green and the whole set of four is clean:

44:  suites_pass=1 suites_fail=0   (2 of 2 tests, both assertions past the old wait)
62:  suites_pass=1 suites_fail=0
72:  suites_pass=1 suites_fail=0
92:  suites_pass=1 suites_fail=0

Suite 44's second test now takes 140 seconds where the failing version took 70, because it genuinely waits through its own backup hold — and the two waits inside it (the hold, and a monitor pass completing after the hold) are fail-loud, so a window that never existed would time out rather than pass quietly. The unit-level mutation drills for the blocker and the idle-kill fix were both run and both bit.

Unit suite on the final head: 5,501 passing, 18 pending, 0 failing.

[Cabecinha84]

PR #1779 — review of the current head (6067bf2)

Verdict: ACK-able, with one fix I'd ask for first. It is ~3 lines, it is in the success path of the restore, and it
contradicts a rule this PR itself states and enforces on the backup side. Everything else I found is a nit or a
follow-up.

What I did

Fetched refs/pull/1779/head (6067bf2), diffed against the real merge-base (383d8ae) rather than the branch tip,
read all ~950 lines of product change plus the harness additions, traced every cross-module contract the changed code
depends on (globalState getters, folderNeedsUpdate, dockerActual, manageFolderSyncState's return shape,
getVolumeInfo's new shape across all six callers), and ran the full unit suite in a throwaway worktree.

Unit tests: 5,493 passing, 5 pending, 21 failing — all 21 in tests/unit/dockerService.test.js, all pre-existing. I ran
that file alone on development and it fails 23 there, so the PR fixes nothing and breaks nothing in it; the delta is
docker-daemon availability in my environment, not the branch. dockerService.js is not in the diff. Nothing
attributable to this PR fails.

Where it stands vs. my last review

The blocker from 2026-08-24 17:54 is genuinely fixed, and fixed the right way rather than papered over.

3c8a24b removes receiveOnlySyncthingAppsCache, backupInProgress and restoreInProgress from masterSlaveApps's
signature entirely and reads them off globalStateParam at each decision (advancedWorkflows.js:4189-4190, :4263, :4579,
:4619, :4649). The self-re-invocation at :4694 no longer re-passes anything captured. serviceManager.js:641-649
carries a comment stating the constraint at the call site, which is where it needed to be. I verified the thing the
fix rests on: globalState.receiveOnlySyncthingAppsCache (globalState.js:168) returns the live Map, so the
seedCache.designatedLeader = false write at :4619 still mutates real state — a frozen copy there would have been a
silent new bug, and it isn't one.

0e6b8ab and 6026e4b are both correct and both minimal. On the latter, the ordering in finish() is right: a
consumer throw or spawn failure wins the error slot, idleKilled rides separately, and a kill-provoked exit no longer
surfaces as "exited with code 143".

The four commits after that (9fbac88, b69bba2, d84024c, 6067bf2) are harness-only — suite 44, 62 and 92 plus
longer surfaces as "exited with code 143".

The four commits after that (9fbac88, b69bba2, d84024c, 6067bf2) are harness-only — suite 44, 62 and 92
plus seed-helper. No product surface.

The one fix I'd ask for

The restore starts g: containers the election owns — advancedWorkflows.js:2820

await sendChunk(res, 'Starting application...\n');
await appDockerStart(appname);

appDockerStart(appname) with a bare app name enumerates appSpecs.compose and starts every component (:2066-2071).
The backup path, forty lines earlier, deliberately does not do this:

// A g: component's run state belongs to the election, not to this task:
// starting it here would put a second writer on the shared volume.
const componentsToStart = componentsOfApp(appDetails)
.filter((comp) => syncModeOfComponent(comp.containerData) !== 'elected');

The restore has the same hazard and none of that guard. Three ways it bites:

1. appDockerStop(appname) stops every component, not just the targets. So restoring component A of a composed app
   stops component B — and if B is g: and correctly stopped on this node because the election put the primary
   elsewhere, the restore then starts it. A g: component that was never part of the restore gets started on a
   non-primary node.
2. The FDM guard at :2641-2647 only refuses on a positive answer. getMasterIpFromFdm returns { ip: null, fdmOk:
   true } when all three regions fail (:263) — it never returns fdmOk: false, so that half of the condition is
   vestigial. FDM unreachable → primaryIp null → guard does not fire → restore proceeds → g: container started
   blind.
3. force: true skips the FDM check entirely and then starts the g: container.

The folders are resumed at :2812 before the start, so the window is: a g: container writing into a live
sendreceive folder on a node that may not hold the primary — the exact "two writers on the shared volume" the
backup path names.

The reconciler is a partial backstop but not a reliable one: effectiveDesiredRunning returns awaitingController
and takes no action when controllerDesired is unset (appReconciler.js:371-373), which is precisely the state after
a FluxOS restart. masterSlaveApps converges it within ~30s once the app leaves the busy list — but only if FDM
answers, which is the case where this fires in the first place.

The fix is the backup path's own three lines. appDetails is already in scope at :2820, and componentsOfApp /
syncModeOfComponent are already imported.

Worth a suite-91-shaped test: a restore on an app with a g: component whose primary is elsewhere must leave that
component stopped.

Non-blocking

A failed restore with a failed demotion is un-paused by the next monitor pass. At :2872-2879 the restore leaves an
undemotable folder paused and out of the resume — deliberately, with a loud log, and the comment is honest that
this is damage limitation. But folderNeedsUpdate returns true on existingFolder.paused
(syncthingMonitorHelpers.js:294), the app is no longer busy so the busy-filter at :2814 doesn't hold it, and the
next pass writes paused: false. The partial data then goes out to healthy peers. It takes a double failure
(extract fails and the receiveonly PATCH fails while syncthing was reachable enough to have paused earlier), so I
would not block on it — but the mitigation is asymmetric with how carefully everything else here is sealed. A
durable quarantine the monitor honours would close it.

sendChunk costs 3s per line. :1799-1803 — the restore emits substantially more progress lines than it used to
(per-target pause, download, archive check, extract). On a five-component app that is a minute of pure delay().
Pre-existing design, newly amplified. Not correctness.

fdmOk is dead. getMasterIpFromFdm returns true on every path (:245, :263), so both call sites' fdmOk conditions
(:2645, :4234) test a constant. Pre-existing, not introduced here, but the new call site reads as if it
distinguishes something it cannot.

skipUpdate is a dead return field. manageFolderSyncState returns it at syncthingFolderStateMachine.js:1151;
processContainerData destructures skipProcessing and never reads skipUpdate (syncthingMonitor.js:321).
Pre-existing, but this PR deleted the only branch where the value mattered — it can go.

backup isn't shape-checked. The restore validates Array.isArray(restore) (:2576); the backup does not, so a
non-array backup surfaces as a TypeError from .some() rather than a stated refusal. Cosmetic — the outer catch
handles it.

addToRestoreProgress / removeFromRestoreProgress (:3137, :3145) now wrap the atomic primitives but have no
production callers, only tests. addToRestoreProgress discards tryStartRestore's boolean, which is the one thing
that primitive exists to return. Either delete them or have them return the verdict.

[MorningLightMountain713]

All six items are addressed. The blocking one is fixed differently from your suggestion, and the reason is in the first section.

Blocking: the restore started g: containers the election owns

Fixed. Your reading of the mechanism is exactly right, including all three routes in — the stop's fan-out reaching a component that was never a restore target, the guard refusing only on a positive answer, and force bypassing it altogether.

I did not take the backup path's filter, though. A g: restore is meant to run on the primary — the guard's own comment says restoring anywhere else is quietly undone by the writer — so refusing to start elected components outright taxes every legitimate restore with an election cycle of downtime. Instead the FDM answer is kept rather than discarded after the refusal check: a confirmed primary starts everything as before, and only an unconfirmed one leaves elected components to the election.

Both versions were mutation-drilled against each other. The original fan-out fails the three leave-alone cases while the confirmed-primary case passes; the blanket filter fails only the confirmed-primary case. An existing test — "removes the copy it downloaded only once the app is back up" — also fails under the blanket filter, so the tree already disagreed with that version.

Suite 86 gains the fleet case you asked for: a forced restore on the standby must leave the elected container down, asserted over a window rather than sampled once, with the standby's stopped state established first so a stopped container afterwards means something.

fdmOk

Fixed, and it turned out to be more than cosmetic.

masterSlaveApps contains a stand-down for exactly this case:

if (!fdmOk) { log.warn('All FDM services failed ... skipping primary selection for this cycle'); continue; }

Since the flag was never false, that branch had never run. "FDM could not be reached" and "FDM says this app has no primary yet" both arrive as a null ip, and the election reads the second: it logs "has currently no primary set", checks whether this node's copy looks synced, and moves to promote itself — on the observation that should have stopped it.

fdmOk now carries whether any region returned a verdict. A success body is one, with or without an ip. A 404 is one too, deliberately: FDM holding no record of an app is the answer a newly deployed g: app gets, and standing down on it would leave that app without a first primary for as long as FDM had no row — a deadlock rather than a guard. A 503 is not a verdict, since it is FDM reporting itself as not ready, and neither is a body that is not success. One region answering is enough, so losing a single region does not stand the fleet's g: apps down.

The behaviour change is deliberate and is a trade: during a total FDM outage a g: app now stands down instead of self-promoting. It costs availability across the outage and buys the guarantee that two nodes never write one volume.

A correction to something I wrote in the previous round. A comment in the restore path, and the body of the commit that added it, said masterSlaveApps skips primary selection when every region fails. It did not — it proceeded, because the branch that would skip is the dead one above. The restore guard is right for its own local reason, and now states that rather than citing another module's behaviour.

The FDM stub gained the ability to stop answering — closing its socket for a refused connection, or returning 503 — and suite 97 covers the property: with FDM silent, no holder promotes; when FDM answers again, one does.

The four non-blocking items

Quarantined folder un-paused by the next monitor pass — no change. Verified: folderNeedsUpdate returns true on existingFolder.paused, so the next pass writes paused: false, and it takes the double failure you describe. The obvious mitigation is not one — setControllerDesired('stopped') is an in-memory Map wiped by a restart, and stopping the container does not stop syncthing transmitting what is on disk. A durable quarantine the monitor honours needs a lifecycle design rather than a flag, so it is written up rather than patched here.

sendChunk's 3s per line — removed. The flush beside it is what delivers a line; the delay was padding on an outage, since the app is stopped and the folders paused for most of the lines it delayed.

skipUpdate — deleted, along with its export. Worth adding that wiring it up would have been the wrong repair: a folder already in sendreceive still needs its config rewritten when the app's device list changes, and folderNeedsUpdate already makes that decision by comparison. A caller honouring skipUpdate would suppress those writes. Its test asserted only the field's own value, so the path's two real effects — a stopped r: container asked to run, and the health monitor's cache entry carried forward — are what it asserts now.

backup not shape-checked — added, in the restore's wording. Both guards are covered, each by a test that fails when its own check is removed and is indifferent to the other's.

addToRestoreProgress / removeFromRestoreProgress — deleted rather than fixed, since nothing in production called them. You are right that discarding tryStartRestore's boolean is the flaw: a refused claim leaves the list looking exactly as the first claim left it, so a caller would have started a second restore and been told nothing. The one test that needed an app mid-restore now takes the claim through the primitive, and the primitive itself is covered — including the refused verdict — in globalState's own tests.

One scoping change worth flagging

Suite 35's mid-backup test ended by waiting for the elected primary to be running again within two minutes of the backup releasing its claim. That assertion has moved to #1780.

It is not this branch's property. recordRestart here counts every restart, deliberate or not, so each start and stop the suite performs earns a rung on the backoff ladder; by that test the component is on the five-minute rung, and the backup's own stop puts it there again. The election asks for a start every cycle and the reconciler defers each one — the app returns after the rung elapses rather than inside the window the assertion allowed. #1780 is where a deliberate stop stops earning a rung, so that is where the convergence assertion belongs and where it now lives.

The three assertions the test was written for are untouched and pass: an election pass ran and skipped the busy app, no start was decided while the hold was live, and no start event was emitted. The counter is still what stops the no-start assertion passing vacuously on a cycle that never ran.

Verification

The full harness gate runs green on this branch — 85 suites, including the restore family end to end and the new FDM-outage suite. The unit suite is green at 5,515.

[Cabecinha84]

Three things I'd raise, none blocking

1. d2a79fa deletes a convergence assertion, and the reason is production-visible. Suite 35 used to end by waiting
   for the elected primary to be running again within two minutes of the backup releasing its claim. It was removed
   because on this branch that doesn't happen: appReconciler:945 calls recordRestart for every restart, deliberate or
   not, so the backup's own stop earns a rung on the ladder [0, 30s, 5m, 15m, 30m]. The commit message says the fix
   belongs to another branch.

The production exposure is smaller than the harness makes it look — restartWaitMs resets the ladder when the previous
run provably lasted STABLE_RUN_MS (10 min), so a backup of a normally long-running app resets and restarts
immediately. What's left is back-to-back operations, or an app started under 10 minutes earlier. But it is a change
this PR makes: backup and restore no longer restart the g: component themselves and hand it to the election, which
goes through the paced reconciler. The direction is right (starting a g: container the election doesn't own is the
incident class), the pacing is the cost. Worth a line in the PR body next to the other deliberate consequences, and
worth confirming the follow-up branch actually exists.

2. A g: component that is not a restore target is stopped and left down. primaryConfirmedLocal is only computed when a
   target is elected, so restoring only the r: sidecar of an app that also has a g: component stops the g: one via the
   bare-name fan-out and then declines to start it. There's a test asserting exactly this, so it's a decision and not an
   oversight — but the cheap improvement is to run the FDM check when any component of the app is elected rather than
   only when a target is. In the common case (FDM names this node) that starts it back immediately instead of waiting an
   election cycle plus whatever item 1 costs.

3. An FDM outage now compounds silently. With FDM unreachable, the restore declines to start its elected components
   and the election declines to act. Both are individually correct; together, a restore performed during an outage leaves
   the g: component down for the whole outage with no log line saying so. One log.warn at the end of the restore —
   "elected components left to the election; FDM did not answer" — would make it diagnosable from the node's own logs.

Minor, mentioned only for completeness: the fdm-stub's /appips/:app can return success-with-empty-ips or 503, but
never 404 — so the "404 counts as an answer" branch, the one that keeps a newly deployed app electable, has unit
coverage only and suite 97 can't reach it.

[MorningLightMountain713]

None of the four changes the code, and three of them are the same item.

Items 1, 2 and 3 each describe a g: component that a backup or restore stops and then abandons — to the election, to an outage, to the pacing ladder. It is not abandoned. Neither task writes desired state, so controllerDesired still reads running and the reconciler restarts the component when the task releases its lease. The election is not in that path. What remains is the backoff ladder, which is item 1, which is #1780. Item 4 asks for end-to-end coverage of a branch with no outcome of its own.

1 — the moved convergence assertion

This is not a finding. It is this PR's own disclosure from the previous round, returned as a review item.

The round-five reply raised it unprompted, under "One scoping change worth flagging", and stated the whole of it: that the assertion was removed, that recordRestart on this branch counts every restart so the backup's own stop earns a rung, that the direction is deliberate, and that #1780 is where the assertion now lives. Nothing in this note is new information about the branch, and nothing in it disputes what was said.

What the note does add is one correct observation, and it argues the exposure down: restartWaitMs resets the ladder when the previous run provably lasted STABLE_RUN_MS (appsRuntimeState.js:191), so a backup of an app that has been up ten minutes restarts with no wait at all. That is the right reading, and it is why this was a scoping note rather than a defect.

One claim in it is wrong. "Backup and restore no longer restart the g: component themselves and hand it to the election" is true of the backup path and false of the restore: the restore starts the elected component itself, directly, wherever FDM confirms this node holds the primary. And neither hands anything to the election — see item 2.

"Worth confirming the follow-up branch actually exists": #1780 is open, is based on this branch, and carries the assertion in 2945592ad. It was named in the round-five reply.

The PR body gains the line.

2 — a g: component that is not a restore target

The observation is right; the mechanism is not, and the mechanism is what decides whether there is anything to fix.

Restoring an r: sidecar does stop the app's g: component — the stop fans out by bare app name — and the restore does then decline to start it. There is a test pinning exactly that, as you found. What does not follow is that the component is left down.

The stop writes no desired state: appDockerStop calls the docker service and returns. So on the node that was running the component, controllerDesired still says running, and the reconciler — the only thing that starts a container — restarts it once the task releases its lease. The election is not in that path; it re-asserts a desired state that was never lost.

What that costs is the backoff ladder, and nothing else. On this branch recordRestart counts every restart, so the task's own stop earns a rung and the reconciler defers the start until it elapses. That is item 1's cost reached by a second route, and it has item 1's answer: #1780 is where a rung is earned by evidence of a fault, and a deliberate stop is not one.

There is one case where the election really is the path, and it is worth stating because it is the one your description fits. controllerDesired is in-memory, so a FluxOS restart empties it, and the reconciler will not start a g: component whose desired state is absent. The next election pass reinstates it — FDM names this node, the readiness gate falls through to syncthing's live folder type rather than the emptied cache, and requestMasterStartWithPermissionsFix writes desired state back. So it recovers there too, one pass and an ownership pass later, rather than staying down.

The widening you suggest is not available in any case: the same gate feeds the refusal three lines above it, so an r:-only restore on a node that is not the primary would be refused for data every instance holds.

3 — the FDM outage

Two things here are wrong, and the second is the one that matters.

It is not silent. getMasterIpFromFdm logs an error per region, naming the app, at the moment the restore consults it — three lines for a total outage, on the restore's own timeline. masterSlaveApps then logs its stand-down for that app every cycle for as long as the outage lasts. An FDM outage is among the loudest things in a node's log.

And the g: component is not down for the outage. Neither the restore nor the stand-down writes desired state: the stop calls the docker service and returns, and the stand-down is a continue. So on the node that was running the component, controllerDesired still says running from before the outage, and the reconciler restarts it once the restore releases its lease. FDM's opinion is not consulted, because nothing has arrived to change the last one it gave.

The compound case does exist, but it needs a third thing: FDM unreachable and FluxOS restarted, so the in-memory desired state is gone and the election cannot reinstate it. That is the trade this PR stated when the stand-down was added — across a total FDM outage a g: app stays down rather than self-promoting, buying the guarantee that two nodes never write one volume. It is the cost side of a decision, not a compounding failure.

No change.

4 — the stub and the 404

Correct: the stub answers 200 with an empty ips array for an app it holds nothing for, and its outage modes are a closed socket and a 503. It has no 404.

It does not need one. A 404 and a success body with an empty ips array are the same event inside getMasterIpFromFdm — both set answered, neither yields an ip, and the function returns { ip: null, fdmOk: true } either way. There is no downstream difference for a test to assert on.

The property that branch exists for — an answer naming no primary must not stand the node down, or a newly deployed app never gets a first primary — is what suite 97's second test asserts end to end: it ends the outage, FDM answers "no primary yet", and the promotion follows. Same branch of the election, reached by the route real FDM actually takes for an app it holds no row for.

No change.

---

The branch is unchanged since round five — d2a79fabe, full gate green across 85 suites, unit suite 5,515 passing. Nothing from this round is outstanding.

[Cabecinha84]

ack