initPickRoots can only offer MOUNTED filesystems, so on a box whose second drive
is new — unmounted, often unformatted — it returned in silence and the "where
should LibrePortal keep things?" prompt never appeared. The users most likely to
want a separate disk were the ones told nothing.
Add a notice listing what is attached and how to use it, deliberately only a
notice: mounting or formatting someone's disk is not something an installer
should do unasked, and `storage add` does it properly later with the
empty-directory admission rule and the fitness checks behind it.
Two things it has to get right, and both bit during development:
* "has no mount point" is not "is free" — the disk holding root has no mount
point of its own, its partition does, so the naive check offered the user
the disk they booted from. Walk the parent chain and mark holders in use.
* lsblk -r renders an empty mount point as a run of spaces, which `read`
collapses, shifting every later column left; an LVM member then parsed as a
mount point and was offered as free space. Use -P.
scripts/dev/lp-installer-disks-test runs the real function against a stubbed
lsblk that honours the output flag it is passed — without that the stub answers
-P to everything and cannot tell the two parsers apart, which is the bug being
guarded against. Verified both regressions fail it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The wizard's Storage step builds its first entry from primaryRoot() — the
app-data root — and labelled it "System disk". On a default install those are
the same drive and the name is honest. Installed with --containers-dir on its
own disk they are not, and the step then showed the DATA drive's size under the
system disk's name while the actual system disk never appeared in the list.
Seen on a matrix case-2 install (apps on a 29.4G test disk, system on a 912G
root): "System disk — 26.7G free of 29.4G".
The generator now reports whether that root is really on the OS disk
(is_os_disk, by st_dev against /), and the wizard labels it from that: "System
disk" when they coincide, otherwise the mount point. The "system" badge stays —
it marks the default location, which is still what it is.
Also add lp-shot --token / --cookie-js. A screenshot answers "does it render";
"does this wizard step work" needs clicking, which needs a real browser, which
needs the session lp-shot already knows how to mint from the stored jwtSecret.
This bug was found that way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three roots reach running code three ways — the CLI wrapper exports them,
the task-processor unit carries them as Environment=, and anything those start
inherits them. An @reboot crontab entry is started by none of the three: it runs
a script by absolute path, so paths.sh fell through to
: "${LP_CONTAINERS_DIR:=/libreportal-containers}"
That entry is crontab_boot_app_reconcile.sh, which brings every installed app up
at boot. On a relocated install it therefore reconciled against the DEFAULT
root, and that does not fail — docker creates the bind-mount directories it does
not find, so every app comes back empty while the real data sits untouched on
the other disk. Nothing logs an error; the only symptom is opening an app and
finding it blank (storage-locations §10.1 calls this the top data-integrity
risk).
Verified on a case-2 install (apps on /mnt/lptest2): a bare environment resolved
containers_dir to /libreportal-containers/ where the real root was
/mnt/lptest2/libreportal-containers.
Recover them from the systemd unit, which is the authoritative baked record —
init.sh already reads it back the same way, libreportal-relocate rewrites it,
and it is root-owned, so this is not the manager reading a config it can edit.
An explicit environment still wins; with no unit the defaults are unchanged.
scripts/dev/lp-paths-roots-test pins all four cases; verified it fails when the
recovery is removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three roots are independently relocatable, and the failures that matter are
the ones where only ONE of them moves: paths are baked into root-owned helpers,
the systemd unit and the CLI wrapper at install time, so anything that resolves
a root at runtime instead works on a default install and points at the wrong
disk on a relocated one. Testing "all default" or "all moved" misses that.
scripts/dev/lp-testdisk loopback ext4 disks — a real superblock, its own
st_dev and free space, thrown away between runs
scripts/dev/lp-install-matrix installs across the four root combinations and
checks each landed on the intended DEVICE, that
the helpers were baked (no __PLACEHOLDER__ left)
and that the WebUI answers
First thing the harness turned up: lp-shot hardcoded /libreportal-containers for
both the compose file it reads the published port from and the .auth.json it
signs a session with. On an install whose app data is on another disk it fell
back to a default port and a missing auth file — which looks exactly like a
WebUI that failed to boot. It now reads the baked LP_CONTAINERS_DIR back out of
the CLI wrapper.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
grafana restored and then died with "attempt to write a readonly database",
repeatedly. Its database is recorded in the snapshot as 231543:1002 and landed
as 1002:1002 — the owner was lost, so grafana, running as 231543, could not
write it at mode 0640.
Restore runs as the backup user with no CAP_CHOWN, so it reinstates ownership
inside a user namespace. The prefix was
unshare --map-root-user --map-users=SUB:SUB:N --map-groups=SUB:SUB:N
and unshare accepts ONE range per option, so the backup user's own GID was never
mapped — while app data is written as <container-uid>:<backup-user>. The group
half of every such chown referred to an unmapped id, lchown returned EINVAL, and
the file kept the restoring user's ownership. restic reports those as "ignoring
error ..." and still exits 0, so nothing failed: 1626 of one 13-app restore's
2086 failed chowns were grafana's, under a restore that reported success.
restic-userns-exec uses newuidmap/newgidmap, which write the multi-range maps
unshare cannot express:
uid: 0 <- caller inner root, or caps are dropped at exec
SUB.. <- SUB.. identity, so restic can name the stored uid
gid: caller <- caller identity: the group half of app-data chowns
SUB.. <- SUB.. identity
The caller's own UID is deliberately not identity-mapped — that slot is spent on
inner root — and a file stored as <caller>:<caller> lands owned by the caller
anyway, because that is who inner root is outside. So the one case this cannot
map is the one case needing no mapping. `unshare --map-auto --map-current-user`
is not a shortcut: it maps the subuid range to low inner ids while restic needs
identity. Tested.
Measured live, restoring grafana: failed chowns 1626 -> 12 (the 12 being the
caller's own files, correct), grafana.db back to 231543:1002, grafana up and
writing. Falls back to running the command plainly when there is no subuid
range, no newuidmap, or the namespace will not start.
scripts/dev/lp-userns-ownership-test pins all three ownership cases; verified
the old prefix fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
portFindNextAvailablePort consulted LibrePortal's own network_resources table
plus a hardcoded list (8080, 6060) and CFG_RESERVED_PORTS_EXTRA — while the
comment above it claimed a picked port "can never collide with a host service at
compose-up time". It can: the list only covers what someone thought to write
down. Ask the kernel instead, via ss, read once per allocation rather than per
candidate. No ss => empty set => exactly the old behaviour.
Found while restoring 13 apps onto a desktop, though not the cause there:
stoat's livekit publishes a FIXED udp range (50000-50100, which it advertises to
clients and so cannot be re-rolled), and kdeconnectd held 50016. That collision
needs its own answer; this fixes the randomly-allocated ports, which had the
same exposure with no reason to.
Also make the bulk restore stop reporting a half-running app as a clean success.
continue-on-error lets a failed compose-up log and carry on, so restoreAppStart
returns 0 either way — which is how that run printed "13 apps restored" while
four of stoat's containers had exited 101. checkSuccess already appends every
failure to error_report.log, so watch it grow across each app and name the ones
that were noisy.
scripts/dev/lp-port-host-test binds a real socket and asserts the allocator
refuses that port; verified it fails when the check is removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
scripts/dev/lp-cli-argv-test builds stubs from the real invocation line in
init.sh and the real LP_CLI_ARGS line in start.sh, then pushes thirteen app
names through them — so editing either file is what makes it fail. Verified
against both regressions: dropping "$@" from the wrapper, and reading "$@"
instead of "${@:10}" in start.sh.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
storageSnapshotSourcePath resolved a snapshot's source path with
engineSnapshotsJson "$idx" "$snapshot_id"
but that function's second parameter is an app TAG filter. So it ran
`restic snapshots --tag app=<snapshot-id>`, matched nothing, and returned 1 —
every time, for every snapshot, since the file was written.
Nothing broke loudly, because both callers have a fallback:
* storageRestoreAppTo fell through to "restoring in place", reinstating the
exact cross-root bug the file exists to fix — restoring onto a host whose
containers root differs from the source's matched no include path and
restored nothing, silently
* the first-run preflight never read a manifest, so every app reported size
"?" and its fit and location checks passed unconditionally. Thirteen green
ticks that had checked nothing.
Add engineSnapshotPaths: restic answers it with a positional snapshot id, kopia
by filtering its list. borg has no adapter on purpose — it rebuilds its listing
from archive metadata that carries no paths — so a missing adapter is a quiet
"no" and those callers keep their in-place fallback.
Add scripts/dev/lp-preflight-test, which pins the cases that must say NO: an
app too big for the disk, one this version no longer ships, one whose storage
location is gone, and a resolver that reaches for the app-tag filter again.
Verified against both historical bugs — reintroducing either fails the test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per-app placement worked but had no default: a box with a big second disk
meant setting CFG_<APP>_STORAGE on every app individually. CFG_STORAGE_DEFAULT
fixes that, and the wizard asks for it in one line.
CFG_<APP>_STORAGE now has three states rather than two, and the third is
the point:
<name> this app goes there, whatever the default says
primary this app goes on the install-time root, explicitly
default no opinion — follow CFG_STORAGE_DEFAULT
Templates ship "default", so the setting reaches every app without
touching 37 configs, while an app that was deliberately placed keeps its
placement. "primary" is new, and needed: without it there was no way to
say "keep this one on the system disk" once the global default moved.
A default naming a location that has since been removed falls back to the
primary root rather than refusing — a disk that got unregistered must not
make apps un-installable.
The wizard asks only once a second drive is ticked; with nothing ticked
there is one possible answer and a control would be furniture. It sets a
default, not a placement, and the value stored is the location NAME, so it
survives the disk being remounted elsewhere.
scripts/dev/lp-storage-default-test covers all three states plus the
removed-location fallback.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to 928e244, which stopped configs/ subdirectories being sourced
without a .category marker. That closed the hole; this removes the thing
that fell into it.
storageIndexFile pointed at configs/storage/app_locations. The file's
requirements are only "manager-owned" and "not on a removable disk" —
configs/ satisfies both, which is why I put it there, and it was still
wrong: that tree carries a third property the file violates. sourceScanFiles
SOURCES what it finds under configs/, and sourcing means executing.
The index is a TSV of "<slug><TAB><root>", which bash reads as a command
and its argument. Harmless while no slug matched a real executable. The
row for the app named `libreportal` armed it, because that IS the CLI on
PATH: sourcing ran `libreportal /libreportal-containers`, which re-entered
the scan, which sourced the file again — one process pair per level until
the host OOMed and took the desktop session with it.
It now lives at $system_dir/storage/app_locations, with a one-shot
migration so an install that already has an index keeps knowing where its
apps live rather than silently forgetting. libreportal-ownership
reconciles the new directory, and scan_files.sh gained a note that
configs/storage/ carries no .category on purpose.
scripts/dev/lp-configs-guard-test covers both ends: the index never lands
in configs/, a legacy one migrates, and a file of the exact detonating
shape placed in an unmarked configs/ subdirectory is not executed while a
marked category still loads.
Also wires sourceStorageLocations into the config scan beside
sourceBackupLocations — per-location configs sit at depth 3, below the
generic scan, and need their own walker like the backup ones do.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 0 of docs/roadmap/storage-locations.md — the resolver layer. No
behaviour change yet: with no registry present, every function here
returns exactly what the old single-root code did, which is what makes
the ~200-site sweep that follows safe to land incrementally.
primaryRoot / webuiDir the install-time root, and the one tree that
never moves
storageRoots every registered root, primary first
storageRootAvailable marker present == drive mounted
pathIsContainerData replaces the `== "$containers_dir"*` idiom
that picks manager vs container-user elevation
appDir / appDirSlash THE resolver, memoised
storageLocationPath/Name name <-> path, via the root-owned registry
storageIndexGet/Set app -> location cache
appDir resolves discovery-first: whichever root actually holds
<slug>/<slug>.config wins, so a hand-move or half-finished migration
self-heals rather than corrupting.
The index exists because of a bug the unit test caught immediately.
Discovery cannot see an unmounted disk, so an installed app on an
unplugged drive looked identical to a brand-new app — and the fallback
handed back the PRIMARY root. Docker would then have created the bind
mounts there and booted the app empty on the wrong disk, which is the
precise failure the availability design exists to prevent. The index is
manager-owned (deliberately not on the removable disk: it must be
readable exactly when that disk is absent), consulted only when the scan
comes up empty, and rewritten by every successful scan so the disk stays
authoritative whenever it is actually present.
Availability is gated in appDir alone rather than at each caller: every
site reaches it by construction. It returns non-zero AND prints an
unusable sentinel path, so the many callers that will never check $?
still fail loudly on something harmless.
scripts/dev/lp-storage-test covers all of it against a throwaway tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three visible faults on the Tasks page, one shared root.
Upgrade tasks rendered as their raw command — "libreportal updater
upgrade rocketchat 8.7.1" beside properly named neighbours. The title
table had rows for updater check/apply/apply-all/rollback and none for
upgrade, because the upgrade command is assembled in task-actions.js
rather than task-commands.js — and lp-task-names, the guard built to
catch exactly this, only read task-commands.js. It certified 16 commands
and reported that as the whole surface; the surface was 29. The guard
now reads both dispatch sites (JS ${expr} interpolations become sample
placeholders; commented-out prose mentioning commands in backticks is
skipped, or it reports fictional commands), and all 29 pass.
A removed instance's tasks outlive it, and its slug rendered as a tech
identifier: "Bookstack_uitest - Remove Instance". getAppDisplayName
cannot help — it capitalises as its own fallback, so unknown is
indistinguishable from known-and-plain. The formatter now does the same
membership test the helper uses internally: slug absent from
window.apps, prefix before the underscore present -> render the way
live instances are shown, "Bookstack · uitest".
Same story for the icon: bookstack_uitest.svg is deleted with the
instance, and onerror="display:none" left a bare gap in the row. Now a
fallback chain — the TYPE's icon (which survives), then the LibrePortal
logo. Verified live: the dead instance's rows show bookstack.svg with
the fallback marker set, everything else keeps its own icon.
Verified in a real browser session — full render, zero console errors.
The 'add' verb also joins the app-action map so "Add Application" is
deliberate wording rather than the blind "<Verb> Application" compose.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task titles come from one table whose final fallback returns the raw command
string, so a dispatched command with no matching row does not error — it just
renders as "libreportal instance remove bookstack_work" beside properly named
neighbours. That silence is why this kept being fixed and kept coming back.
The guard reads BOTH files as source — the command templates from
task-commands.js and the pattern table from tasks-format.js — so it fails on a
command added without a name rather than leaving it to be noticed in the UI.
Two checks, both from source rather than guessed from rendered text:
1. Nothing falls through: a title equal to its command, or still starting with
"libreportal ", means the raw fallback was reached.
2. Every `libreportal app <verb>` verb has an actionMap entry. Without one the
generic branch composes "<Verb> Application", which is how "Up Application"
and "Down Application" shipped.
The second check reads the actionMap keys instead of pattern-matching the title,
which a first attempt did and which was wrong: "Reload Application" is both a
correct hand-written label and what the generic branch emits, so the rendered
text cannot distinguish them and the heuristic failed a title that was fine.
Verified by breaking it deliberately in both directions — adding a command with
no pattern, and deleting an actionMap verb. Each is caught, named, and pointed at
the file to edit; both files were restored byte-identical afterwards.
Lives in scripts/dev, which .gitattributes marks export-ignore, so it never ships
in a release tarball. Needs a node and borrows the running container's when the
host has none, the same constraint lp-shot works around for chromium.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The WebUI screenshot helper CLAUDE.md already tells agents to use only
ever existed on the maintainer's box. Vendoring it means it survives a
machine rebuild and the setup steps are written down.
It does NOT ship: make_release.sh builds with `git archive`, which honours
export-ignore, so scripts/dev joins scripts/release and docs on that list.
Verified — the staged tarball has 1666 files and none under scripts/dev.
Keeping it out of releases is deliberate, not incidental. lp-shot signs
itself a session from the jwtSecret in frontend/.auth.json, which is fine
on a box where you already own that file, and has no business sitting in
a user's install where it would read as a backdoor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
