fix: verify the cluster before treating the 0.3.5x relocation as done; bump Nextcloud to 33.0.7 (33.0.7:0)

[MattDHill]

@dr-bonez — review requested.

The bug

The Postgres entrypoint runs mkdir -p "$PGDATA" in docker_create_db_directories before docker_verify_minimum_env decides the database is uninitialized and exits 1 — verified against postgres:17-alpine:

_main() {
    docker_setup_env
    docker_create_db_directories        # ← mkdir -p "$PGDATA"
    ...
    if [ -z "$DATABASE_ALREADY_EXISTS" ]; then
        docker_verify_minimum_env       # ← "Database is uninitialized…" exit 1

So a Postgres start against a volume whose cluster hasn't been relocated yet leaves an empty data/ behind — and both setupMain and the init upgrade start Postgres.

Both guards in the 0.3.5x migration tested for that directory rather than for a database:

Guard	Test	Satisfied by
relocatePostgres	test -d $PGDATA → skip the move	empty data/
isNeverStarted	stat() on the three candidate paths	empty data/

An empty data/ therefore reads as "already relocated". The migration skips the move, runs the config import and the permissions walk, and deletes start9/config.yaml on its way out — the marker gating the whole block. From that point it is disarmed: it completes in ~3 seconds doing nothing, the cluster is never moved, and every Postgres start finds an empty PGDATA, recreates it and exits 1.

Commit 1 — the fix (e6d12ab)

• findCluster() locates the cluster by PG_VERSION across data, 17/main and 15/main, returning null when there isn't one. Replaces both directory-existence guards.
• Throws before touching anything when no cluster is found, so it cannot delete the marker on a run that would migrate nothing.
• The two failure cases are now distinguishable. The old message told the user to uninstall and reinstall the package — right for a never-started instance, wrong for a populated one, and with the corrected guard that message would start reaching populated instances. An instance with app data is instead told to restore from a backup and explicitly not to uninstall.
• rmdirs the empty PGDATA shell before the move. rmdir, not rm -rf: it refuses a non-empty directory, so an unexpected state fails loudly rather than being deleted. Without it, mv src $PGDATA moves the source inside the existing directory (data/docker) rather than into place.
• Same PG_VERSION test and rmdir applied to the 0.4.0-beta relocation (17/docker → data), which carried the identical latent bug.

Progress reporting

The SDK hands every migration a FullProgressTracker — "add a phase and update it to show progress in the update UI" — and this migration destructured only effects.

The permissions walk corrects permissions on every file under data/ one directory at a time (a single recursive chmod OOMs in the migration subcontainer), so its cost scales with file count. On a large instance it runs for hours while the update UI shows a bar that never moves.

Now reports a running count of directories processed, on the same 100-directory cadence as the existing log line. No total, deliberately: establishing one costs a second full metadata walk, about as expensive as the work itself.

Naming / structure

Both of these run during init and the names were near-synonyms for different systems, which made the above hard to reason about:

migrateNextcloud  ->  repairPermissionsFrom035x   (it chmods; it doesn't migrate)
relocatePostgres  ->  relocatePostgresFrom035x
migrateConfig     ->  importConfigFrom035x
upgradeNextcloud  ->  runUpstreamUpgrade

The 0.3.5x body moves out of current.ts into versions/from035x.ts, leaving current.ts as version + release notes + a one-line call (388 → 106 lines). Each file's header names the other and states the ordering: the StartOS layout migration is driven by the version graph, the upstream app upgrade by the bundled Nextcloud release, and versionGraph precedes bootstrapNextcloud in sdk.setupInit.

Commit 2 — hardening (4fe67fa)

No confirmed occurrence. Separate commit so it can be dropped without touching commit 1.

Postgres treats SIGTERM as a smart shutdown and waits for clients to disconnect; the SDK escalates to SIGKILL after 60s, so a stop that outruns that budget strands postmaster.pid. On the next start Postgres aborts with FATAL: lock file "postmaster.pid" already exists if the PID it names is alive — and each chain build gets a fresh PID namespace with a fresh, low PID assignment.

getBaseDaemons is the only place in the stack that starts Postgres without clearing it first: Backups.withPgDump does it before every pg_ctl start, and the 0.3.5x relocation does it too. Adds a pg-recover oneshot, with postgres gaining requires: ['pg-recover'], covering setupMain and both init paths in one place.

Unconditional removal is safe because nothing else can hold the data directory: backups only run once the service is stopped, both init paths run stopped, and the chain reconciler fully terms an entry before starting its replacement. Runs as root so ownership can't block the unlink and wedge the chain on the new oneshot.

Specifically worth your eyes

1. Is rmdir on PGDATA the right strictness? It only ever removes the empty shell the entrypoint leaves. I chose it over rm -rf so unexpected content fails the migration rather than being deleted — but it means a PGDATA containing junk now blocks the migration instead of being cleared. I think failing loudly is correct; say if you'd rather it were permissive.
2. The permissions walk is not resumable. Its completion marker and the work it gates aren't committed atomically, so an interruption discards all progress. Is there an OS-side primitive for a resumable init step, or should the package checkpoint this itself?

Test plan

1. Fresh install of 33.0.6:3 — installs, web UI loads, login works.
2. Normal update from 33.0.6:2 on a 0.4.0-native instance — completes, service healthy, no migration work runs.
3. 0.3.5.1 migration, happy path. From a 0.3.5.1 box on Nextcloud 32.0.11, update to 33.0.6:3. Confirm an "Updating file permissions" phase with a climbing count; the cluster moves from db/17/main to db/data; the service starts with files, users and shares intact.
4. The regression. Before updating a 0.3.5.1 instance, mkdir an empty db/data to simulate a prior failed Postgres start. Update. On 33.0.6:2 this silently completes and leaves an unusable instance; on 33.0.6:3 it must rmdir the shell, relocate the cluster and start normally.
5. No-cluster path. Remove the cluster entirely, leaving config.yaml and app data. Update. Must fail fast with the "restore from a StartOS backup / do NOT uninstall" message, and start9/config.yaml must still be present afterwards.
6. Interrupted walk. Cancel partway through the permissions phase, then retry. The retry must re-run the migration rather than skipping it.
7. Backup/restore round-trip still works on 33.0.6:3.
8. Stale lockfile (commit 2). Force-kill the service so Postgres is SIGKILLed, confirm postmaster.pid remains, then start it — it must come up rather than crash-looping on lock file "postmaster.pid" already exists.

[helix-nine]

Tested on a StartOS 0.4.0 VM (/etc/os-release VERSION="0.4.0", git e7ed5699).

Method. Three s9pks built from this repo with one workspace key: 33.0.6:2 from origin/master (BASE), 33.0.6:3 from 4fe67fa (FIX), and a control — master's code with only the version string bumped to 33.0.6:3 (CONTROL) — so every fixture can be run against old and new logic back to back. 0.3.5x state is synthesised on the volumes of a stopped 33.0.6:2 install (main/start9/config.yaml + password.dat, cluster moved to db/17/main, app files chgrp root), then the update is sideloaded; canMigrateFrom on :3 is >=32.0.11:0 <=33.0.6:3, so migrations.up runs on :2 → :3. Between cases the package is uninstalled, :2 reinstalled, and the volumes restored from a tarball of the pristine install. Canary data: a second user (helixtester) and a file in admin/files.

Your test plan

	Result
1. Fresh install 33.0.6:3	pass — healthy in 5s, occ status clean, cluster at db/data
2. Normal update :2 → :3, no 0.3.5x state	pass — migration no-ops (db/data untouched, no config.php.bak), healthy. pg-recover sequenced correctly: daemon postgres waiting on pg-recover → Launching pg-recover... → Launching postgres...
3. 0.3.5x happy path	pass — db/17/main → db/data (PG_VERSION 17), db/17 removed, default_locale/default_phone_region/maintenance_window_start imported, overwrite.cli.url + htaccess.RewriteBase gone, config.php.bak written, admin password recovered into store.json, perms ug+rw,o-rwx, marker deleted, service healthy, both users and the canary file intact
4. The regression	pass, with a control. CONTROL on 17/main + empty data/: update reports success, cluster still at 17/main, data/ still empty, marker deleted, then Database is uninitialized and superuser password is not specified on loop with health stuck waiting. FIX on the identical fixture: rmdirs the shell, relocates, comes up healthy with data intact. Your root-cause analysis reproduces exactly.
5. No-cluster path	pass — update fails, rolls back to 33.0.6:2, main/start9/config.yaml and password.dat still present. The restore-from-backup message reaches the OS verbatim (journald start_core::install).
6. Interrupted walk	not exercised — with 4 050 directories the walk finished in ~90 s, faster than I could interrupt it. Needs a much larger tree.
7. Backup/restore	not exercised — needs a backup target.
8. Stale lockfile	could not reproduce the failure — see below.

Findings

1. 15/main is a regression against master — the migration completes and disarms itself on a cluster Postgres 17 cannot open

findCluster (from035x.ts:83-88) accepts a location on the existence of PG_VERSION and never reads it, and PG_LOCATIONS (:69) includes 15/main.

Measured, same fixture (db/15/main only, PG_VERSION = 15), both builds:

• CONTROL (master): update fails. Marker preserved, cluster untouched at 15/main. Recoverable.
• FIX: update succeeds. Cluster moved to db/data, db/15 deleted, marker deleted. Then:

  FATAL:  database files are incompatible with server
  DETAIL: The data directory was initialized by PostgreSQL version 15, which is not compatible with this version 17.9.
  

  crash-looping, migration gone. Same self-disarming class this PR exists to close.

Master only ever moved 17/main, so its execFail(['mv', '17/main', …]) threw and it failed safe. Widening the search to 15/main without a version check turns that into a silent success.

This isn't hypothetical: origin/0351/32.0.11 runs its own pg_upgrade 15 → 17 in-container (PGDATA_OLD="/var/lib/postgresql/15/main", marker .pg17_upgrade_complete) and deletes 15/main on success. So a surviving 15/main is the un-upgraded case — precisely when the data really is PG15. And because PG_LOCATIONS is ordered, 15/main is selected only when there is no 17/main, i.e. only in that case.

Reading the file (PG_VERSION is one line) and refusing anything but 17 with its own message would make this fail safe again; running pg_upgrade would fix it properly.

2. rmdir doesn't guard anything — answers your question 1

from035x.ts:121 is sub.exec, not execFail, and the result is discarded, so the comment at :117-120 ("fails loudly rather than deleting a database") describes a branch that doesn't exist. Same at :311.

Fixture: cluster at 17/main, db/data present and holding one stray file, no PG_VERSION. FIX build:

--- db tree ---   .../db/data      .../db/data/main
--- marker ---    No such file or directory
version           33.0.6:3
logs              Error: Database is uninitialized and superuser password is not specified.

rmdir fails silently, mv nests the cluster at data/main, the migration reports success and deletes the marker. Not worse than master (which skipped the move and broke the same way), but not fixed either, and the comment says it is.

Note execFail alone would be wrong: on the happy path $PGDATA does not exist at all, so rmdir legitimately exits non-zero — that's why exec is there. The distinction you want is ENOENT-ok / ENOTEMPTY-fatal:

await sub.exec(['rmdir', PGDATA], { user: 'root' })
if (await exists(PGDATA)) throw new Error(/* … */)

So: yes, failing loudly is the right call — it just isn't what the code does today.

3. The fix does not reach instances the shipped migration already disarmed

findCluster() sits inside the if (configYaml) gate (:336, findCluster at :343), but :0/:1/:2 deleted config.yaml on the very run that skipped the relocation. Fixture reproducing a real victim — cluster at 17/main, empty data/, marker already gone — updated to :3:

version: 33.0.6:3        db: unchanged (17/main + empty data/)
Error: Database is uninitialized and superuser password is not specified.

Update completes silently, migration does nothing, service still crash-loops. Caveat on faithfulness: my fixture derives from a :2 install, so version.php is already 33.0.6; a real victim is still on Nextcloud 32.0.11 and would more likely hit the runUntilSuccess timeout in the init upgrade chain instead. Either way the migration doesn't repair them.

A Debian-layout 17/main/15/main can only exist on an instance that came from 0.3.5x, so calling findCluster() unconditionally (with data/ first, as it already is, making a healthy instance an immediate no-op) would cover them. Worth deciding deliberately whether that's in scope for this PR or a follow-up — right now the en_US note at current.ts:20 promises a clear explanation that only holds while the marker survives.

4. The climbing count never reaches the user

The phase is real and live — polling stateInfo through the walk:

Updating | phases=[… ["Updating", {"overall":{"done":0,"total":100},
  "phases":[{"name":"Updating file permissions",
             "progress":{"done":3900,"total":null,"units":"steps"}}]}]]

But progress-phase.component.ts:21-41 collapses total === null into the same branch as false:

} @else if (leaf === false || leaf.total === null) {
    <span>{{ 'in progress' | i18n }}...</span>

done and units are dropped; isIndeterminate returns true. The user sees the phase name, "in progress…", and an indeterminate bar — visually identical to setUnits/setDone never being called. The Updating sub-tracker's overall also sat at {done: 0, total: 100} for the whole walk (server-side progress_ratio scores a total: null leaf as 0), and the outer overall was pinned at 201/251.

The actual win — a named phase with a moving bar where master showed nothing — is real and worth keeping. But setUnits('steps') + setDone(dirCount) (:240, :247, :282) are currently invisible, so either drop them, or the OS needs to render done when total is null. (Related, and probably an OS bug: format-progress.ts:46-50 has no null guard and would emit 3900/null steps.)

5. Commit 2: could not reproduce the failure it prevents

A live postmaster.pid captured from a running instance names PID 3 — your "fresh, low PID assignment" holds. But on the next start the postmaster gets PID 3 again (deterministic container startup), so Postgres sees its own PID and skips the check entirely. Planting a PID that is alive and definitely not the postmaster (PID 1, start-container) also started cleanly — root-owned, so kill(1,0) returns EPERM, which Postgres explicitly treats as stale.

Both stale-lockfile fixtures started healthy in 10 s on the CONTROL build, i.e. without pg-recover. So the oneshot is harmless and correctly sequenced (verified in T2), but the README paragraph at README.md:50 asserts a mechanism I couldn't make fire. Given "no confirmed occurrence", I'd trim the 184-word justification to a sentence rather than document a misfire that didn't happen — it's also the only causal story a future reader will have.

6. Smaller things

• Long error message vs. the sideload stream. Both failing updates surfaced to the CLI as Network Error: WebSocket protocol error: Control frame too big (payload must be 125 bytes or less), not the message. The 305-character string lands correctly in journald and the rollback is clean, so it's an OS transport limit rather than a package bug — but it is the message users are meant to read, and it's what triggers it.
• default.ts:196 annotates key 134 // versions/current.ts: 0.3.5x migration progress; the only call site is from035x.ts:359, created by this same commit.
• from035x.ts:6-8 says the migration "runs at most once, on the first update away from 32.0.11:0". The auto-generated < current vertex means up runs on every update into :3 — including :2 → :3, which is how I ran most of this.
• AGENTS.md tells you to attach with -n nextcloud; the real names are nextcloud-sub, nextcloud-cron, postgres-sub. Pre-existing, but it's the one instruction in there I had to correct to use.

Nothing in 1-3 is caused by the change of direction in this PR — 2 and 3 are inherited, and 1 is the one place the widened search made a previously-loud failure quiet. The core fix does what it says: I reproduced the reported failure on master's logic and watched this build recover the identical instance.

[helix-nine]

Pushed 33fd8b2 with findings 1, 2 and 6. Re-tested on a StartOS 0.4.0 VM against the same fixture harness.

1 — the 15/main regression

Fixing this turned up a case my original report missed, so the guard is not the one I proposed.

Reading origin/0351/32.0.11:nextcloud-run.sh, 0.3.5x's pg_upgrade runs in copy mode (line 50) and 15/main is reaped on the next launch, gated on .pg17_upgrade_complete (lines 21-26) — not at completion. So a surviving 15/main is ambiguous:

• marker present → the upgrade finished, 17/main is authoritative, 15/main is a fallback that was never reaped. Migrating is correct.
• marker absent → the upgrade never finished. 15/main holds the real data, and any 17/main beside it is the empty initdb stub from line 47 — which findCluster prefers, because 17/main sorts before 15/main.

A pure PG_VERSION !== '17' check passes on that stub and relocates an empty cluster. So the gate is now the marker, with the version check kept as a backstop:

if ((await clusterVersion(`${POSTGRES_VOLUME_HOST}/15/main`)) &&
    !(await exists(PG_UPGRADE_MARKER))) throw /* upgrade never finished */
if (cluster.major !== PG_MAJOR) throw /* not PG 17 */

findCluster reads PG_VERSION through a clusterVersion helper instead of stating it. relocatePostgresFromBeta uses the same helper, so its comment claiming "Same PG_VERSION test as findCluster" is true again rather than quietly falsified.

2 — rmdir

clearPgdataShell keeps exec and checks the postcondition, since execFail alone would break the happy path where $PGDATA is legitimately absent:

await sub.exec(['rmdir', PGDATA], { user: 'root' })
if (await exists(`${POSTGRES_VOLUME_HOST}/data`)) throw new Error(PGDATA_NOT_EMPTY)

Used at both relocation sites. The comment promising a behaviour the code didn't have is gone.

6

• default.ts key 134 comment → versions/from035x.ts.
• File header no longer claims the migration "runs at most once, on the first update away from 32.0.11:0"; it says the version graph runs up on every update into current and the config.yaml marker is what makes it a no-op.
• AGENTS.md subcontainer names → nextcloud-sub, nextcloud-cron, postgres-sub, valkey.
• The 125-byte close-frame limit is OS-side, not fixable here — WebSocket::close_result puts the error's Display straight into the close-frame reason (start-core/src/util/net.rs:153-159) and RFC 6455 caps that payload at 125 bytes. Filed as Sideload errors longer than ~123 bytes are lost: close-frame reason exceeds the WebSocket control-frame limit start-technologies#3605. Every message this migration throws is over that limit, so until it's fixed these are journald-only on a CLI sideload.

Also updated, because the change made them inaccurate rather than merely incomplete: README.md's 0.3.x paragraph (it already stated the PG 15→17 precondition the code now actually enforces), UPDATING.md (a postgres major bump also means PG_MAJOR), and the releaseNotes closing clause in all five locales — "stops with a clear explanation if it cannot find one" → "if it cannot safely continue", since there are now three refusal conditions rather than one.

I did not reap the stale 15/ after a successful migration. It is a full copy of the database and now permanently dead weight, but deleting a user's only fallback isn't a call I wanted to make inside a bugfix — flagging it rather than doing it.

Test matrix

Every row: sideload onto a stopped 33.0.6:2 with the 0.3.5x state synthesised on the volumes, then start.

fixture	expected	result
17/main, no data/	migrate	migrated, healthy, users + canary intact
17/main + empty data/ shell (the PR's original bug)	migrate	rmdirs the shell, migrated, healthy, data intact
17/main + 15/main + marker (upgrade done, not yet reaped)	migrate 17/main	migrated, healthy, canary intact
17/main stub + 15/main, no marker (interrupted upgrade)	refuse	refused, rolled back to :2, 15/main + 17/main + start9/ all preserved
15/main only, no marker	refuse	refused, rolled back, cluster + marker preserved
17/main + non-empty data/	refuse	refused, rolled back, cluster + marker + the stray file preserved
no 0.3.5x state at all	no-op	no-op, healthy

The two refusals land as Nextcloud's PostgreSQL 15 to 17 upgrade never finished on StartOS 0.3.5.x… and Nextcloud cannot move its PostgreSQL database into place…, both verified in journald.

Unrelated but worth deciding before this merges: #123 rewrites the same current.ts to 33.0.7:0 off the same base and carries none of this work. Whichever lands second needs rebasing onto the other — if #123 goes first, 33.0.6:3 is below the published best and none of this reaches anyone.

[helix-nine]

Pushed 70f647c — this now releases as 33.0.7:0 rather than 33.0.6:3.

The bump

33.0.7 is the latest on the 33 line (cut 2026-07-23), so this stays a single major step from the 32.0.11 that 0.3.5.x instances run. Maintenance only:

• core/shipped.json is identical between v33.0.6 and v33.0.7, so the preserved-defaults list in disableUnstableApps and its README count are unaffected.
• lib/versioncheck.php is byte-identical — same PHP floor.
• nextcloud:33.0.7-apache publishes amd64 and arm64.

Content is security hardening (sabre/xml callable deserialization disabled, federated-share rate limiting, lost-password throttling, user-search disclosure tightening, code-signing revocation list + CA bundle), sharing/DAV/external-storage fixes, and perf work on PROPFIND, trashbin deletes and the photocache.

One user-visible regression comes with it, and the release notes now say so in all five locales: upstream disabled the ImageMagick preview providers as a security stopgap (nextcloud/server#62148 on stable33). PreviewManager::registerCoreProviders() gates the whole imagick block on IMagickSupport::hasExtension(), which returns false at v33.0.7, so PDF, SVG, TIFF, HEIC, PSD, EPS, TTF, TGA and SGI thumbnails are gone — not just HEIC. PNG/JPEG/GIF/BMP/WebP/video are unaffected. The re-enable merged to stable33 on 2026-07-29 (#62619) but is not tagged, so it lands in 33.0.8.

The revision resets to :0 because the upstream version moved.

What this changes about the update itself

:2 → :3 was a StartOS-only revision bump, so runUpstreamUpgrade early-returned. 33.0.6:2 → 33.0.7:0 is a real application upgrade, so occ upgrade now runs during init — and on a 0.3.5.x instance it runs after the layout migration, in the same update. That is a code path the previous rounds never exercised, so it is what I tested.

Tests

Re-ran on a StartOS 0.4.0 VM against a 33.0.6:2 baseline.

	result
33.0.6:2 → 33.0.7:0, no 0.3.5x state	Upgrading nextcloud from 33.0.6.2 … → Update successful; healthy in 5s; occ status reports 33.0.7; users intact
33.0.6:2 → 33.0.7:0 with the 0.3.5x fixture (cluster at db/17/main)	both run in order — Migrating 33.0.6:2 -> 33.0.7:0 relocates the cluster to db/data (PG_VERSION 17), deletes db/17 and the start9/ marker, imports default_locale/default_phone_region/maintenance_window_start, writes config.php.bak and recovers the admin password from password.dat; then occ upgrade takes the volume 33.0.6 → 33.0.7. Service healthy, occ status 33.0.7, both users and the canary file intact, occ back to -rwxrwx--- www-data:www-data

The migration logic itself is byte-identical to 33fd8b2 (git diff 33fd8b2 -- startos/versions/from035x.ts startos/utils.ts startos/init/ is empty), so the refusal matrix from the previous round — interrupted pg_upgrade, 15/main only, non-empty PGDATA, empty data/ shell, no-0.3.5x-state no-op — still stands and I did not re-run it.

One thing I found but did not fix

The "Remove stale config.php keys from 0.3.5.1" step is a no-op. configPhp.merge(effects, { 'htaccess.RewriteBase': undefined }) does not delete the key — start-sdk's FileHelper.merge is a deep merge and cannot remove keys. Verified by grepping config.php immediately after the update with the service never started: 'htaccess.RewriteBase' => '/' is still there.

Two reasons I left it alone: it is pre-existing on master (both migrateConfig and the tail of up do the same thing), and the premise may no longer hold anyway — a fresh 0.4.0 install has htaccess.RewriteBase too, so it is not obviously a stale 0.3.5.1 key any more. Deleting it for real means a read-modify-write rather than a merge. Happy to do that here or leave it for its own change.

Note for #123

#123 now conflicts — both PRs rewrite current.ts. It will need a rebase once this merges, and its 34.0.2:0 still cannot be the release a 0.3.5.x user lands on.