Caught during the round-4 e2e validation on real PG: every
successful restore landed with `status='completed', was_successful=false`
because the success-branch update only wrote `status` but not
`was_successful` (column default is false). Visible side effect: the
BackupDashboard's "last successful restore" filter would skip the
row + any future audit query gating on was_successful would miss it.
One-line cure: include `was_successful: true` in the success-branch
update payload. Inline comment explains why and references the
review note so future edits keep the two fields together.
Source-inspection test in restoreService.pgBranch.test.js pins the
contract: after `performPostRestoreVerification(...)`, the
`status: 'completed'` update payload must also contain
`was_successful: true`. Future refactors of the success payload that
drop the flag fail the test before merge.
36/36 backup-related integration tests pass.
End-to-end DR cycle surfaced one more PG-only landmine — and it
turned out to be a side-effect of the round-1 replay placement, not
a new bug. Round 2 fixed the comparison logic; round 3 fixes the
ordering.
Symptom on real PG install:
[install-from-backup] FAILED — Post-restore verification failed:
Table app_settings row count mismatch: expected 190, got 191.
Trigger file left in place for retry.
Root cause: the operator-meta replay (introduced in round 1) ran
INSIDE performDatabaseRestore, lined up BEFORE the post-restore
verification step in the parent restore() method. So:
1. psql restores app_settings → 190 rows (matches backup)
2. Replay upserts `restore_allow_force_auto_upgraded` (which the
fresh-install seeded but the backup didn't have) → 191 rows
3. performPostRestoreVerification counts 191, manifest says 190,
verification fails the row-count check.
Replay is doing the right thing (preserving operator policy). The
verification is doing the right thing (counts must match). They
disagree because the replay landed in the wrong sequence relative
to verification.
Cure: move the replay out of performDatabaseRestore and into
restore() AFTER `performPostRestoreVerification` passes.
Verification now sees the as-restored DB (matches the backup
exactly), replay layers on top once verification has signed off.
Mechanism: snapshot stashed on `this.preservedMetaSnapshot`
(initialised in constructor, reset per run at the top of restore()).
performDatabaseRestore writes it in the PG branch before DROP;
restore() drains it after verification. SQLite leaves it empty,
both steps no-op there.
Tests:
- Updated `restoreService.pgBranch.test.js` to pin the new shape:
* `this.preservedMetaSnapshot` is initialised in the constructor
* No stray `let preservedMeta = []` local declarations anywhere
* Replay drain (`this.preservedMetaSnapshot.length > 0`) sits in
restore() AFTER `performPostRestoreVerification(...)` and is
lexically OUTSIDE `performDatabaseRestore`.
- The bigint-as-string contract from round 2 still holds.
34/34 backup-related integration tests pass.
pg-driver serialises `bigint` (which is what `COUNT(*)` returns) as a
JavaScript STRING to preserve precision for huge counts. The manifest
stores `expected.rowCount` as a JS number (parseInt'd at
databaseBackup.js:118). Strict `!==` in performPostRestoreVerification
flagged every match as a mismatch on PG:
Table activity_logs row count mismatch: expected 16, got 16
Table admin_users row count mismatch: expected 1, got 1
Table app_settings row count mismatch: expected 165, got 165
... (every table, all matching)
Symptom matched the preservedMeta scope leak from round 1: install-
from-backup logged FAILED, trigger file wasn't cleaned, data was
actually intact. Caught on PR #596 e2e re-run.
Cure: coerce both sides with `Number(...)` at the comparison AND in
the interpolated value so the warning text renders `16` not `"16"`.
Pre-emptive: lines 448 + 458-459 had the same string-vs-number issue
masked by `>` (JS coerces operands for `>`), but the warning text
printed `"5"` on PG vs `5` on SQLite, and a future patch changing
`>` to `=== 0` or `!== expectedCount` would silently break on PG.
Coerced at the read site into `eventCountN` / `activeUsersN` locals
+ added a comment block explaining the contract so future edits
don't drop the Number() calls without re-auditing.
New source-inspection test: pins the contract that every `.count`
result in restoreService.js MUST be wrapped in `Number(...)` when
used in a comparison (===/!==/>/</>=/<=). Same source-inspection
pattern as the preservedMeta test added round 1 — pragmatic until
the real-PG integration test follow-up lands.
The maintainer's audit of the rest of the backup/restore surface
(_installFromBackupBoot, _restoreSettingsBoot, _backupPathsBoot,
backupCoverageService, backupIntegrityService, backupService,
databaseBackup) confirmed no other bigint-as-string sites — the
class is now closed in the audited scope.
Two nice-to-haves from the PR #596 review.
1. Install-from-backup logging mirrors to stdout
The winston logger writes to /app/logs/combined.log and may not
tee to stdout. Operators tailing `docker logs picpeak-beta-backend`
after a `compose up` saw the migration sweep + npm notice and
nothing about the restore. Three key events now also fire through
`console.log` with a `[install-from-backup] ` prefix:
- "trigger file detected → <manifest>"
- "starting restore from <manifest>"
- "restore completed successfully" / "FAILED — <reason>"
Plus the "skipping — existing data" branch.
docker-logs surface now tells the restore story without requiring
an `exec into the container` step.
2. ADMIN_CREDENTIALS.txt flags stale creds when restore is queued
Migration 001 detects a pending `RESTORE_ON_INSTALL` file BEFORE
writing the fresh-install credentials file. If a trigger will fire
on the next boot, the file now opens with a clear warning:
⚠️ RESTORE_ON_INSTALL TRIGGER DETECTED ⚠️
These credentials are temporary. An install-from-backup run is
queued to fire on the next server start, which will REPLACE
this admin row with the one from the backup. After the restore
completes, log in with your ORIGINAL pre-disaster credentials
— not the ones below. If the restore fails for some reason,
the credentials below remain valid as a fallback recovery path.
Doesn't skip the file (so a failed restore still has the fallback
credentials), just annotates it. Closes the maintainer's "stale
junk credentials" observation.
`preservedMeta` was declared with `let` INSIDE the PostgreSQL else
branch of performDatabaseRestore (~L850), then read AFTER the else
block closed at the shared replay site (~L1030). On every real PG
restore, this threw:
ReferenceError: preservedMeta is not defined
after psql had already loaded the data successfully. Knock-on
effects per the maintainer's review:
- Loud `Install-from-backup: FAILED` line in combined.log even
though the data restored cleanly
- Trigger file in `_installFromBackupBoot.js` was left in place
because the success branch never ran — admin had to manually
rm it before the next boot
- The operator-meta replay (restore_allow_force,
restore_allow_force_auto_upgraded) silently dropped, exactly
the chicken-and-egg the snapshot was added to close.
`restore_allow_force` reverted to the backup's value on every
PG restore.
CI missed it because integration tests around `performFullRestore`
only exercise the SQLite branch (`this.dbType === 'sqlite'`). The PG
branch requires a real psql binary + cluster, which lives in the
"real-PG integration test in CI" follow-up.
Cure: hoist the `const PRESERVED_META_KEYS = [...]` + `let
preservedMeta = []` declarations above the SQLite/PG split. SQLite
leaves them empty; PG branch fills them; replay block at the bottom
reads them on both paths (no-op on SQLite).
New test: `restoreService.pgBranch.test.js` pins the scope contract
via source inspection. Two assertions:
1. Exactly one `let preservedMeta = []` declaration in the file,
positioned before the SQLite/PG branch split
2. The replay block `if (preservedMeta.length > 0)` sits outside
the else block (closing ` }` exists between the branch
opener and the replay site)
Source-inspection beats a runtime test here because (a) it doesn't
need a real PG cluster + psql binary, (b) it pins the EXACT property
that broke, more directly than a runtime test would.
Closes PR #596 review blocker.
The AdminHeader "Clear All" notifications button has been 404'ing for
a while: frontend `notifications.service.ts` calls
`DELETE /admin/notifications/clear-all`, backend only defined
`DELETE /admin/notifications/clear-old`.
The /clear-old route was misleadingly named anyway — it tried to
delete read OR >30-days-old rows, then had a fallback that nuked
EVERY row when nothing matched. Both the frontend and the existing
test expect a simple Clear All shape, so just rename to /clear-all,
drop the tiered logic, and return the plain
`{ message, deletedCount }` payload the test asserts on.
The test (adminNotifications.test.js) was hiding the breakage —
it was on CI's --testPathIgnorePatterns ignore list and so never
ran. Two reasons it failed locally before this fix:
1. Route path mismatch (the actual #597 bug).
2. The mock only stubbed adminAuth — requirePermission lives in
its own middleware module and ran for real, 403'ing before
the handler. Add a passthrough mock for that too.
With both fixed, the test passes. Drop adminNotifications from the
CI ignore list so future regressions in this route fail loudly
instead of going to ground.
upstream/beta independently shipped 108_seed_sl_email_template_translations.js
(Slovenian email template translations) using the migration number
this branch had already claimed for 108_add_backup_paths.js. Knex's
filename-based ordering would have caused both to attempt the slot
at merge time.
Renamed via `git mv` so file history is preserved. All five
references updated in lockstep:
- backend/src/services/_backupPathsBoot.js (require + comments)
- backend/src/services/backupService.js (LEGACY_BACKUP_PATHS comment)
- 3 integration test files (require + "migration 108" prose)
- migration's own header comment, with a paragraph explaining the
rename so reviewers don't wonder why the number jumped
**No data-migration impact for installs that already ran the
108-named version** (Ralf's beta, primarily): the migration's body
is idempotent — createTable is guarded by `hasTable`, and the seed
uses `onConflict('path').ignore()`. So when 109 runs against an
install whose backup_paths table is already populated, both the
schema step and the seed step no-op cleanly. The orphaned
`108_add_backup_paths.js` row in the `migrations` tracking table
sits harmlessly alongside the new `109_add_backup_paths.js` row.
No data lost, no double-insert, no schema drift. Mechanical rename
ahead of the PR opening.
The previous split (separate docs/install-from-backup.md + separate
README link for "Disaster Recovery") fragmented what's conceptually
one workflow: backup → restore. DR is a specific scenario of restore
(the destination is wiped), not a separate feature.
This merge:
- Folds install-from-backup content into docs/backup-restore.md
as a "Disaster recovery (install from a backup)" section with
its own table-of-contents anchor.
- Adds an explicit ToC at the top so admins land on what they
need in one click.
- Frames the two restore paths up front: "live install" → wizard,
"fresh / wiped install" → trigger file. Admins encountering DR
in panic mode don't need to know to look under a separate link.
- Drops the duplicate "Disaster Recovery" README bullet. The
"Backup & Restore" blurb now mentions DR explicitly so it's
still findable via Ctrl+F on the README.
- Removes docs/install-from-backup.md (its content is now in
backup-restore.md's DR section).
Single source of truth = less risk of one doc going stale relative
to the other when the feature evolves. Maintainer-facing surface
on docs.picpeak.app shrinks back to one /guides/backup-restore page.
The four backup admin panes (BackupHistory, BackupDashboard,
BackupCoverageCard, BackupIntegrityCard) used raw date-fns
`format()` with hard-coded tokens like 'p' (12-hour AM/PM), 'PP',
'PPP', 'PPp', and 'yyyy-MM-dd HH:mm:ss' — ignoring the admin's
configured `general_date_format` and `general_time_format`
settings.
Net effect on a 24h-configured install: backup History row showed
"11:25 PM" instead of "23:25", and the Coverage tab's "Last dump"
+ "Coverage generated" timestamps were stuck on
yyyy-MM-dd HH:mm:ss regardless of the admin's date-format choice.
All four panes now route through `useLocalizedDate()` which honors
both settings + the active i18n locale (per the existing
[[feedback_respect_general_format_settings]] pattern).
Tokens replaced:
format(date, 'p') → formatTime(date)
format(date, 'PP') → format(date)
format(date, 'PPP') → format(date)
format(date, 'PPp') → formatDateTime(date)
format(date, 'yyyy-MM-dd HH:mm:ss') → formatDateTime(date)
format(date, 'yyyy-MM-dd HH:mm') → formatDateTime(date)
No backend changes — settings already shipped via /admin/settings;
this just makes the consumers actually read them.
Rekoo-PS's v3.59.0-beta.0 screenshot showed a different shape than
the truncate fix in e7cf834 addressed. Their company name ("Arkan
Studio") isn't unusually long, but with logo_and_text display mode
on a phone-width viewport the wordmark wrapped to two lines and the
LanguageSelector button — sitting in the right action cluster —
landed visually on top of the wrapped second line.
Truncate alone left "Arkan Studio" rendered as "Ar..." after the
logo image. Functional but ugly, and on accounts where the wordmark
reaches the right cluster the visual overlap returns. Match what
LanguageSelector does for its language name in #527: hide the
wordmark on <sm when a logo is also showing (the logo carries the
identity), keep it on sm+. text_only mode is unchanged — wordmark
shows on every width, otherwise nothing would render.
Truncate stays in place as defensive depth for the text_only path.
Closes the six-step DR dance ("onboard throwaway admin → restore via
wizard → log out → log back in with originals") by letting admins
recover an install with zero clicks past `docker compose up`.
Convention: drop a file named `RESTORE_ON_INSTALL` (no extension OR
.txt) into the existing `/backup` bind mount. On next container
start, the new boot hook detects it, runs the restore, and starts
the server with the restored state. Admin opens the browser, login
works first try.
Payload variants:
- empty file → auto-picks newest backup-manifest-*.json from
/backup/manifests/. Useful for "restore the latest".
- path inside the file → uses that specific manifest. Useful for
"I want this older backup, not the most recent".
Safety gates (three layers):
1. Trigger file must exist — no auto-magic, admin signals intent
2. DB must be empty (no events, ≤1 admin) — refuses to clobber
production data
3. Restore failure leaves the trigger file in place for retry on
next container start. Success deletes it so subsequent boots
don't redo the work.
Override hook: INSTALL_FROM_BACKUP_FORCE=true skips guard #2 for the
"I know what I'm doing" edge case (dev env rebuilds, etc).
No docker-compose changes required — uses the bind mount picpeak
already has, env vars are optional. The minimal admin workflow now
matches the bare-minimum mental model: "copy my backup files,
restart the container, log in with original credentials."
Tests: 7 scenarios covering trigger detection, payload variants,
safety gates, success/failure trigger-file lifecycle.
The #592 fix added a devtools-detection probe, and the #592 follow-up
added a require_password probe + a branding-defaults whereIn().select().
Both shift the db() call indices the existing #550 test relied on, and
the branding probe needed `.select()` to resolve to an array (the mock
chain wasn't thenable, so `for..of` on the result threw → 500 on every
test that hit BASE_BODY).
Add `whereIn` + `selectResult` to buildChain so the branding probe
yields an iterable. Factor the three pre-slug app_settings chains into
a baseSettingsChains() helper and update each test's queued sequence
and toHaveBeenNthCalledWith / toHaveBeenCalledTimes expectations to
match the new shape. No behaviour change in v1/events.js — only the
test scaffolding moves.
Closes the last gap from tonight's backup-hardening: backup_runs.
statistics now carries a `per_path` map keyed by backup_paths.path
(e.g. `events/active`, `business-docs`), with per-bucket count + size.
Backend (backupService.js):
- new `computePerPathStats(backedUpFiles, allFiles)` helper that
bucket-sorts each backed-up file into its owning backup_paths row
by longest-prefix match. Reuses the same backup_paths source the
walker reads, so toggling include_in_default off propagates
correctly. Falls back to LEGACY_BACKUP_PATHS if the table is
missing.
- runBackupInternal calls it after the destination implementation
reports back, includes the result in statistics under both
snake_case (`per_path`) and camelCase (`perPath`) keys for the
same alias treatment the existing fields get.
Frontend (BackupHistory.jsx):
- Backup History detail pane now renders one row per per_path entry
when present, with path label + count + formatted size.
- Falls back to the legacy Photos / Archives / "Other" rendering
when the field is absent (backups taken before this commit). No
breaking change for stored history.
Tests: new backupService.perPathStats.test.js — 2 scenarios pinning
attribution behaviour (single-path, nested-paths-don't-collide).
Plus a NOTE comment about overlapping-path walker behaviour (out of
scope; canonical seed doesn't hit it).
Closes the chicken-and-egg where `restore_allow_force` (and its
auto-upgrade tracking flag) got overwritten on every restore by
whatever value happened to be in the backup. Net effect:
1. Admin enables Force Restore (via tonight's default-ON migration
edit, or hand-SQL on older installs).
2. Restore runs successfully.
3. Restored DB has `restore_allow_force = <backup's old value>`.
4. Next restore attempt: "Force restore is not allowed by system
settings" — admin needs the SQL workaround AGAIN.
Cure: snapshot a small list of operator-meta keys BEFORE the DROP
DATABASE (while we still have a working pool against the OLD DB),
then UPSERT them back AFTER the psql restore + migrate.latest.
The preserved set is intentionally narrow — currently just
`restore_allow_force` and `restore_allow_force_auto_upgraded`. These
are about how the operator wants the install to behave, not user-
facing state. Adding more keys is a one-line addition to the
PRESERVED_META_KEYS constant.
Survives both:
- backup is OLDER than the operator's most recent setting change
- backup is NEWER but had a different operator policy
Either way, the post-restore install reflects the LIVE operator
policy, not the backup's snapshot of it.
The in-session toggle fix in d292b9f handles click 2 correctly, but
on a hard refresh likedPhotoIds was always initialized to an empty
Set — so previously-liked photos rendered un-filled until the user
opened the lightbox.
Backend: gallery.js GET /:slug/photos now mounts resolveGuest and
emits a per-viewer is_liked boolean per photo. Prefers req.guest.id
when a verified guest token is present (per-person identity), falls
back to the IP+UA hash that generateGuestIdentifier produces — same
identity model galleryFeedback.js uses for /my-feedback. Skipped
when feedback is hidden from guests.
Frontend: Photo type gains optional is_liked. Each of the 7 grid
layouts (Masonry / Grid / Justified / Timeline / Carousel / Mosaic /
Premium) seeds its lifted likedPhotoIds Set from photos.filter(is_liked)
on the first non-empty payload, gated by a seededRef so subsequent
React Query refetches don't clobber in-session optimistic toggles.
Mosaic uses photo.is_liked ?? false in its per-card useState initializer.
GalleryPremium also drops the buggy `|| like_count > 0` fallback at
line 521 that treated "anyone liked this" as "I liked it" — the
per-viewer seed is now the correct source.
GalleryStory had the same shape of bug in two places — same #590 fix:
- Seed switched from like_count > 0 (global) to is_liked (per-viewer),
with the same mount-only seededRef guard.
- handleToggleFavorite now calls submitFeedback on EVERY click, not
only when adding. The previous code skipped the unlike submit, so
the UI removed the heart while the server kept the like row.
Same class of bug as the devtools-detection gap landed in 2304b25.
v1 POST /events was hardcoding require_password=true in the destructure
default and skipping getBrandingDefaults entirely, so:
- Admins who disabled "require password by default" globally still
got password-required galleries through the API.
- API-created events ignored the global branding_logo_display_hero
and branding_logo_size toggles, defaulting to visible/medium
regardless of the admin's preferred branding chrome.
Mirror the readBooleanSetting + getBrandingDefaults pattern from
adminEvents.js inline (helpers aren't exported, and pulling them out
is out-of-scope for this fix). Adds validators, fallback resolution,
and the three resolved values to the events insert. hero_logo_position
stays at 'top' since #357 / migration 084 explicitly disconnected it
from the header-bar branding_logo_position setting. OpenAPI updated.
Closes the loop on the original 2026-05-29 data-loss class: Ralf had
four "Run Backup Now" manifests sitting on disk with
database.backup_file = null because Stage A wasn't yet in place.
The restore wizard would have happily restored any of those four,
bringing back files (photos, PDFs) but leaving the database empty —
silently re-creating the exact data loss the rest of this branch
prevents going forward.
The /restore/list-backups endpoint now returns `database_included`
per row (parsed from the manifest at discovery time). The wizard
uses it to:
- Per-row badge: red "No DB" pill next to any backup where
database_included === false. Tooltip explains the consequence
in plain English: "restoring this will NOT recover the database".
- Selected-card callout: full red banner under the chosen row
when database_included is false, restating the warning + giving
the admin a clear path: "pick a different backup if you have
one with a database dump, or proceed only if files-only is
what you want."
The wizard does NOT block the restore — the admin may genuinely want
a files-only restore (e.g. recovering a deleted photo while keeping
current DB state). The warnings make sure that choice is informed.
The dashboard widget used `lastBackup.created_at` for the "Last
successful backup: X ago" text — but lastBackup is the most recent
row of any status. So a crashed restore (status=running, never
updated) or a recent failure showed up labeled as the last
successful backup. Same "silent failure not surfaced" class the
restore wizard had.
Backend now returns:
lastSuccessfulBackup — most recent backup_runs with status='completed'
zombieRuns — running rows older than 30 min (likely crashed mid-flight)
lastBackup — unchanged (most recent any status)
Frontend renders:
- "Last successful backup: X ago" — always from lastSuccessfulBackup
- "Last attempt: Y ago · failed/running" — when lastBackup differs
from lastSuccessful. failed shows the first line of error_message
in red; running stays neutral.
- Zombie callout — "N backup(s) running >30min — may have crashed"
in amber, so admin sees stuck rows at a glance.
- Health score downgrades from "excellent" to "warning" if the
latest attempt failed, even when older successes keep the age
fresh — surfaces regressions without erasing the green history.
The progress step used a binary `isRunning ? "in progress" : "completed"`
check. So when the backend rejected the restore (pre-flight validator
threw, path error, etc.) the wizard cheerfully rendered "Restore
completed" with 0% progress and no error context — the admin had to
SSH into the server and inspect `restore_runs.error_message` to find
out what happened.
Now reads the most recent row from `restoreStatus.history[0]` and
renders one of three states:
- running → blue text, progress bar updates
- succeeded → green tick + post-restore actions (existing behaviour)
- failed → red banner with the first line of error_message, and
a callout if was_rollback_attempted is true so the
admin knows the destination is safe to retry on top of.
Net: the wizard now tells the truth about what just happened.
`db.destroy()` during restore tore down the in-process connection
pool to release PG sessions so DROP DATABASE could succeed. After
CREATE DATABASE + psql restore, the old code did
`require('../database/db')` expecting a fresh instance — but Node
caches require results, so it got the SAME destroyed instance back.
Every subsequent query in the process failed with "Unable to acquire
a connection" until the container was manually restarted, even
though the restore technically succeeded.
Net effect for admins: login showed "An error occurred", customer /
invoice / quote pages were blank, no surface hinted at the dead pool.
Cure: db.js now wraps the live knex instance in a Proxy that forwards
to a mutable internal reference, with a `reinitPool()` function that
destroys the old instance + builds a fresh one + probes with `SELECT 1`
so any reconnect failure surfaces immediately. The thousands of
existing `const { db } = require(...)` imports work unchanged — they
capture the Proxy once, and every call goes through to the current pool.
restoreService calls reinitPool() after CREATE DATABASE and before
migrate.latest(), so the rest of the request + every subsequent admin
action runs against the fresh pool. Container restart no longer
needed after restore.
Same class of bug as #550 part 2 (feedback default ignored on API
events): the events table column default for enable_devtools_protection
is true, so an admin who disabled detection globally still got it ON
for every API-created gallery.
Mirror the feedback fallback that landed in 1b521e7 — accept an
optional enable_devtools_protection body field, fall back to the
app_settings entry of the same name, and write the resolved value
explicitly on insert so the column default doesn't shadow it.
OpenAPI doc updated to match.
Default nginx is 4 8k — too tight when an outer Cloudflare /
corp-proxy injects long Set-Cookie / X-Forwarded-* headers, or when
a power-user accumulates many per-gallery gallery_token_<slug>
cookies over the 24h maxAge in tokenUtils.js. Either way users hit
"400 Request Header Or Cookie Too Large" and clearing cookies is
the only workaround.
4×32k is cheap RAM, matches what most reverse proxies do upstream,
and means PicPeak doesn't fail the request before the upstream even
sees it.
The /feedback like endpoint is a server-side toggle — the same one
the lightbox uses. Every grid layout's optimistic-UI setter only
ever did next.add(photoId), so click 2 on a liked tile fired a
server unlike but kept the heart filled in the UI.
Switch each setter to toggle (delete if present, else add). Covers
Masonry (default), Grid, Justified, Timeline, Carousel, Mosaic, and
Premium layouts — including their identity-modal callback paths for
shape consistency. Lightbox toggle is unchanged (already correct).
#527 hid the language *name* on <sm to free space for the title.
Since then the right cluster gained dark-mode toggle, notifications,
and the user avatar, and the brand block still had no truncation —
so a long branding_company_name would still push past the available
width into the action buttons on phones.
Defensive fix: min-w-0 on the brand-block wrapper, truncate on the
company-name span, flex-shrink-0 on the logo image. Long names now
ellipsis within the left cluster regardless of how many widgets
fill the right.
demo.picpeak.app sits behind Caddy + Cloudflare; Caddy replaces the
nginx CSP entirely with one that omits 'unsafe-inline' / hash / nonce,
so the #358 inline theme-bootstrap was being blocked there — admin
loaded a black page, the SPA bundle 404'd, link buttons did nothing.
Move the bootstrap to /public/bootstrap.js served as 'self' so the
script runs under every reasonable CSP without further coordination.
Vite copies /public/* to the dist root at build time (same pipeline
as /favicon-32x32.png), and it remains in <head> without defer/async
so it still runs before <body> paints. The OS-preference @media CSS
above still handles the first-frame dark/light baseline.
Root cause of the persistent "Force restore is not allowed by system
settings" error even on fresh installs after `docker compose down -v`:
migrations/core/032_add_restore_runs_table.js seeded the row with
`JSON.stringify(false)` = the literal string 'false'.
So every install (fresh OR upgraded) wrote restore_allow_force=false
at migration time. The boot self-heal added earlier today saw the row
and respected "admin policy" per its safety design — never noticing
that the row was the deprecated migration default, not an explicit
admin choice.
Cure follows [[feedback_migration_no_compensation]] +
[[feedback_self_heal_pattern]]:
1. Edit migration 032 IN PLACE — flip seed value from false to
true. Fresh installs forward get the correct default at install
time, no boot helper needed.
2. One-time auto-upgrade in _restoreSettingsBoot.js for installs
that already ran the OLD migration. Bumps restore_allow_force
to 'true' iff the current value is the deprecated literal
'false' AND the new tracking key
`restore_allow_force_auto_upgraded` doesn't yet exist. The
tracking flag is always written after the first boot pass, so
subsequent admin choices (e.g. deliberately disabling force)
are preserved on every boot after.
3. Defensive: adminRestore.js getRestoreSettings() now normalizes
'true'/'false'/'"true"'/'"false"' string shapes to JS booleans,
not just '1'/'0'. Belt-and-suspenders so any future seeder that
uses a different boolean serialization doesn't silently break
the !settings.restore_allow_force gate.
Net effect: any picpeak install pulling this image — fresh or
existing — gets restore_allow_force=true on first boot after the
upgrade. The catch-22 that forced every disaster-recovery admin to
hand-write SQL before their FIRST restore is closed.
Fresh installs of picpeak had `restore_allow_force` defaulting to
false (or missing entirely). Combined with the "1 active admin
user" pre-restore warning that the fresh-install admin auto-creates,
this meant the very first restore on every new install hit:
Force restore is not allowed by system settings
Admins then had to hand-craft SQL to flip the setting before they
could recover their data — at the worst possible moment, when they
were already mid-disaster.
This isn't security: the admin who can SQL the setting on can also
flip it via the UI. It's just a sharp edge that bites every new
install once.
Cure: boot-time self-heal that seeds restore_allow_force=true only
when the row doesn't exist. Existing installs that explicitly set
the row (true OR false) are NOT touched — admin policy wins.
Pattern mirrors _backupPathsBoot.js and _emailTemplateBoot.js.
Default-ON rationale matches Stage A's principle: the cost of
forgetting (= can't recover from a disaster) outweighs the friction
saved (= adversarial admins can't run forced restores). Audit
logging keeps the accountability story intact.
The "Content Backed Up" panel in Backup History only counted two
categories (Photos + Archives), so a 3-file backup that landed all
3 in business-docs (Ralf's case after the storage truncation +
restore tonight) showed:
Photos (0 of 0)
Archives (0)
→ total: 3 files
The discrepancy made admins wonder where the 3 files actually went.
Adds two rows:
- "Business documents & other" = files_processed - photos - archives
- "Total files" = files_processed
So the math adds up regardless of which Stage B path the files came
from. Properly per-path-category breakdown requires backend-side
per-path counters (separate follow-up); this commit closes the
visible-discrepancy gap without that schema change.
i18n: en + de added; other locales fall back to en until reviewed.
pg_dump emits setval() statements for SERIAL/IDENTITY columns, but
they don't always land cleanly: --clean ordering, knex pool sequence
caching, rows inserted mid-restore (the pre-restore safety backup
writes a database_backup_runs row before DROP), etc. Net result on
Ralf's install after a successful restore:
- "A record with this value already exists" on every CRUD action
- duplicate key value violates unique constraint
"database_backup_runs_pkey" on the next Run Backup Now
Same root cause: every SERIAL column's sequence was pointing at or
below MAX(id), so the next INSERT collided.
Fix: append a DO block after the psql restore that walks pg_class +
pg_attribute and setval()s every public-schema sequence to
GREATEST(MAX(<col>), 1). Cheap (a few ms even on large schemas),
safe (read-only on row data), idempotent — re-running it just
re-asserts the same values.
Seventh latent PG-restore bug discovered on Ralf's install tonight.
Manual hand-fix worked; this commit makes the fix automatic for
every future restore.
PostgreSQL refuses DROP DATABASE while any session is connected:
ERROR: database "picpeak_prod" is being accessed by other users
DETAIL: There are 6 other sessions using the database.
The backend's own knex pool holds 5-25 active connections to the
target DB. So even after closing the request that initiated the
restore, the pool keeps the DB busy and the DROP statement fails.
Three-layered cure, all in the restore service's PG branch:
1. Call `db.destroy()` first to close the in-process knex pool so
we don't fight ourselves. Knex will lazily re-open on the next
query via db.js's retry logic, so this is safe to do mid-restore.
2. SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE
datname=<target> AND pid<>pg_backend_pid() — evicts any sessions
from other processes (other server replicas, leftover idle
transactions, things our own pool destroy missed).
3. DROP DATABASE IF EXISTS "<target>" WITH (FORCE) — PG13+ kills
remaining connections atomically with the DROP. Falls back to
plain DROP on older Postgres where WITH (FORCE) is a syntax error.
Surfaced as the FIFTH latent bug in the restore path tonight: the
DROP DATABASE statement always assumed a quiescent destination, but
the live backend keeps the destination busy at all times. Every
previous PG install of picpeak that ever tried Restore would have
hit this — meaning the disaster-recovery feature has shipped broken
for a long time without anyone exercising it end-to-end.
`psql` with no -d connects to a database whose name matches the
connecting user. On installs where the user's home DB doesn't exist
(common pattern: DB_USER=picpeak, DB_NAME=picpeak_prod, no `picpeak`
DB), the restore's DROP DATABASE / CREATE DATABASE statements failed
with:
FATAL: database "picpeak" does not exist
even though the target DB (picpeak_prod) was alive and connectable.
And of course you can't connect to the target DB itself for DROP —
PostgreSQL refuses while a connection is open to it.
Fix: explicitly connect to `postgres` (the maintenance DB every PG
cluster ships with) for the DROP/CREATE statements. Override via
DB_CHECK_DB env var if the `postgres` DB is restricted to superusers
on the cluster — matches the pattern wait-for-db.sh already exposes.
Also quote the database name in the SQL so installs whose DB has
unusual characters (numbers, hyphens) don't break the statement.
Surfaced during Ralf's end-to-end restore validation — yet another
"never been tested on a real PG install" latent bug exposed by the
Stage A inline-dump path actually being able to produce a restorable
manifest for the first time on his install.
Two changes that close the disaster-recovery loop the Stage A-B-C
backup-hardening plan opened:
1. Resolve 'local' source to backup_destination_path
The wizard passes options.source = 'local' (the SOURCE TYPE
string). The old code assigned that verbatim to localBackupPath
and every downstream path.join() ended up with junk like
'local/database/<file>.sql.gz'. Fixed by looking up
backup_destination_path from app_settings when source='local',
plus a layered candidate fallback in performDatabaseRestore so
absolute paths in manifests are honoured first.
2. Auto-rollback on ANY failure during restore
Previously rollback only fired when post-restore VERIFICATION
failed (inside the try block). Anything that threw earlier —
path bugs, pg_restore failure, file copy errors — left the
destination half-clobbered with no automatic recovery. Now the
catch block always invokes attemptRollback if a pre-restore
backup exists, and persists rollback status in
was_rollback_attempted + an enriched error_message so the admin
can tell at a glance whether the destination is safe to retry
on top of or needs manual inspection first.
Surfaced during Ralf's validation of the end-to-end backup +
restore cycle (`docker compose down -v` then restore from disk).
Every prior failed attempt left stray PDFs behind that the next
attempt had to navigate around — exactly the "every failure makes
the next worse" pattern this fix kills.
Two stacked bugs in the disaster-recovery path:
1. The wizard passes `options.source = 'local'` (the source TYPE
string) and the service assigned it verbatim to `localBackupPath`.
Every downstream `path.join(localBackupPath, ...)` ended up with
junk like `local/database/<file>.sql.gz` and `local/events/...`.
2. performDatabaseRestore reconstructed the dump path from the
manifest by basename-only:
path.join(backupPath, 'database', path.basename(dbBackupFile))
discarding the absolute path the manifest actually recorded.
Cure:
- At the entry point, if `options.source === 'local'`, look up
`backup_destination_path` from app_settings and use that as the
local root. Honour s3:// downloads via the existing branch.
- In performDatabaseRestore, try the manifest's absolute path
first, then `localRoot + manifest_value`, then the legacy
`localRoot + 'database' + basename` reconstruct as a final
fallback. First hit wins; error message lists every candidate
so future failures are diagnosable.
Surfaced during Ralf's end-to-end validation of the Stage A-B-C
backup-hardening plan — restored fresh after `down -v`, the wizard
failed silently with `Database backup file not found: local/database/...`
even though the dump existed at the path the manifest recorded.
With this fix, the same destruction-and-recovery sequence completes.
The Restore wizard's "Choose Backup to Restore" list was driven only
by the backup_runs table. After `docker compose down -v` (the disaster
this whole hardening effort is designed to recover from), the DB is
empty and the wizard shows "No backups found in selected source" —
exactly when it's needed most. The manifest JSONs are still on disk;
the wizard just can't see them.
Adds disk-first discovery:
- Walks backup_destination_path AND backup_manifest_path (manifests
can live in a sibling directory under the canonical
<root>/manifests/backup-manifest-<id>.json layout). Depth-limited
recursion (3 levels) so the scan doesn't enumerate the photo tree.
- Matches backup-manifest-*.json|yaml AND legacy bare manifest.json.
- Parses each manifest for real metadata (timestamp, size, file
count, database.backup_file presence) instead of showing the
admin opaque filenames.
- Layers in surviving backup_runs rows, deduping by manifest_id.
Applied to both GET /available-backups (legacy) and POST /list-backups
(the one the frontend actually calls). Same helper, two call sites.
Side benefit: each returned row now carries `databaseIncluded` — so a
future Restore UI iteration can show a "this backup has no DB dump"
warning before the admin picks a files-only backup. Exactly the
surface that would have caught Ralf's original four files-only
manifests if it had existed.
`BackupHistory.jsx` opened `/admin/backup/download/<id>` via window.open,
which goes to the React SPA's router — no matching route, so it
rendered the "Page Not Found" screen.
The actual download endpoint lives at `/api/admin/backup/download/:id`
on the backend (adminBackup.js:685). Cookie-based admin auth already
supports the implicit cookie sent by window.open, so the URL prefix
was the only thing missing.
Predates today's backup-hardening work — the bug has existed since
this download button shipped. Surfaced now because Ralf finally has a
completed backup to try downloading after the Stage A inline-dump
guard started working.
pg_dump rejects `--single-transaction` — it's a pg_restore / psql flag,
never a pg_dump one. Triggered as soon as the inline-dump path landed
on Ralf's install:
pg_dump: unrecognized option: single-transaction
pg_dump: hint: Try "pg_dump --help" for more information.
pg_dump already wraps the entire export in a single REPEATABLE READ
snapshot automatically (since Postgres 9.x), so the original intent —
consistent snapshot of the live DB — is preserved by removing the
flag. Same "latent until Stage A wired it in" pattern as the three
prior bugs this rollout has surfaced (PG insert destructure → bind-
mount EACCES → Node 22 stdio strict mode → this).
spawnToFile and spawnFromFile passed an unopened WriteStream/ReadStream
directly as a stdio entry to child_process.spawn. Older Node versions
auto-extracted .fd; Node 22 throws synchronously:
The argument 'stdio' is invalid.
Received WriteStream { fd: null, path: '/backup/database/...sql', ... }
Bug bit Ralf's install once today's `bugfix/crm-backup` image landed —
Node 22 came with that image, and Stage A's inline-dump path is the
first caller of spawnToFile on this install. Latent on the previous
image (Node 20); fatal on this one. restoreService's pre-restore
safety snapshot uses the same helper and would have hit it next time
a restore ran.
Cure: stdio: ['ignore', 'pipe', 'pipe'] (and ['pipe', 'pipe', 'pipe']
for spawnFromFile) + manual pipe of child.stdout/stdin through the
file stream. Works on every Node version. Also wires the WriteStream's
'error' event to the promise via settleReject so a future EACCES /
ENOSPC reaches the caller's try/catch instead of becoming a process-
fatal unhandled error event — closing the same "Stage A guard
bypassed" hole noted in the spawned follow-up task.
Side benefit: outStream.end() now awaits flush before resolving, so
fast pg_dump runs can no longer produce a truncated dump.
databaseBackupService.backup() did `const [runId] = await db(...).insert({...})`
without a .returning() — works on SQLite (knex returns [lastInsertId]) but
throws "(intermediate value) is not iterable" on Postgres (knex returns
a non-iterable shape).
Bug was latent until Stage A of the backup-hardening plan wired this
method into the "Run Backup Now" inline-dump path. Before Stage A only
the scheduled cron + the dedicated admin-DB-backup page called it, and
Ralf's install had never exercised either — so the inline-dump default
landing in production was the first time the destructure ran on his PG.
Cure: same explicit .returning('id') + dual-shape coalesce pattern that
backupService.js uses for its own backup_runs insert (line 949).
Two more sibling files have the same anti-pattern (userManagementService,
customerAccountsService — invitation flows) and will bite under the
same conditions; spawned a follow-up task to fix them in a separate PR.
Paul Nothaft