Conversation
…API) Root cause of a real-world failed update: backup_current, deploy_source and deploy_release all plain-cp'd the LIVE panel database. On a busy panel that races the backend's own writes and the copy can capture half a transaction — 'database disk image is malformed' — which then sinks the migration in the new slot and silently poisons the pre-upgrade backup that was supposed to be the safety net. - New copy_sqlite_db() uses the SQLite online backup API via python (guaranteed by pre-flight) and refuses to fall back to a racy cp. - backup_current halts rather than proceed without a valid safety net. - deploy_source/deploy_release halt before any switch if the live DB cannot be snapshotted consistently. - backup_docker_db prefers a consistent snapshot via docker exec python, with the plain docker cp as a fallback. - locate_python rejects candidates whose version probe returns empty (broken shims pass 'command -v' but can't execute; an empty version string used to pass the sort-based range check). Tests: T10d exercises a real snapshot (integrity ok, all rows); a contract test bans plain-cp of the live DB; T10d fixture python detection validates the interpreter actually runs.
The venv binding check only matched the quoted VIRTUAL_ENV="/path" form; venvs created on some platforms write 'export VIRTUAL_ENV=/path' unquoted, so the binding read as 'unknown path' and always triggered a rebuild. Now matches both forms (and skips cygpath-style indirection), so fresh installs keep the fast prebuilt-venv path.
Postgres panels updated with zero safety net: backup_current found no SQLite file and just warned, while the migration runs IN PLACE against the shared external DB — a failed migration left no way back. - backup_current now pg_dumps (custom format) when .env points at PostgreSQL, sanitizing SQLAlchemy driver suffixes (postgresql+psycopg2://) that libpq can't parse. - Same rule as SQLite: no valid backup, no update — halt when pg_dump is missing (install postgresql-client) or the dump fails. Tests: T10e covers the dump invocation + URL sanitization and the halt-on-failed-dump path.
…retention A backup that was never verified is a hope, not a safety net. Closes the remaining ways the pre-upgrade safety net could silently not exist: - Every backup is verified immediately after creation: SQLite snapshots via PRAGMA integrity_check, PostgreSQL dumps via size + pg_restore TOC listing (when available). Invalid artifacts are deleted and the update halts — a malformed backup is never mistaken for a good one. - Partial artifacts from a failed backup are removed (rm on failure), so a half-written file can never be 'restored' later. - A failed IN-PLACE PostgreSQL migration now automatically restores the verified pre-upgrade dump (pg_restore --clean --if-exists) before halting; without it the half-migrated schema would stay live against the old code. If the restore itself fails, the halt message carries the exact manual pg_restore command. - backup_docker_db tries both python and python3 in the container, and now verifies the result too — the plain docker cp fallback can no longer pass off a torn copy as a backup. - Backup retention capped: default 5 (SERVERKIT_BACKUP_RETENTION overrides) across trees, .db and .dump files. Full-size DB copies pile up fast — a real 25G droplet carried 6.3G of stale backups. Tests: T10f (verification helpers accept/reject correctly), T10g (failed in-place PG migration auto-restores with the exact pg_restore args), T10h (retention prunes all three backup kinds to the configured cap).
…t state
verify_pg_dump only runs its structural `pg_restore -l` TOC check when the
binary is on PATH. The harness stubs every other external command it touches
(systemctl, nginx, npm, curl, docker, flask, pg_dump) but never pg_restore, so
two assertions were decided by the host:
* no postgresql-client -> branch skipped, check degrades to "non-empty",
and the synthetic `PGDMP fake contents` fixture passes
* postgresql-client present -> the real binary parses the version bytes
after the PGDMP magic, rejects the fixture, verify_pg_dump returns 1
That is the CI failure. T10f failed directly; T10e failed downstream because
backup_current correctly halts on an unverifiable backup. update.sh is right --
the tests leaked.
Add a make_stub_pg_restore factory mimicking the real -l semantics (rejects
missing/empty, rejects anything without the PGDMP magic, else prints a TOC) and
install it in T10e/T10f. Give T10f a non-empty non-archive fixture too: that is
the case separating the two branches inside verify_pg_dump -- the size check
passes it, only the TOC listing catches it -- so the path the test was named
for is now actually exercised.
Verified both with and without a pg_restore on PATH: 59 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
copy_sqlite_db and verify_sqlite_db reused locate_python, which enforces
3.11 <= v <= 3.12. That bound is correct for rebuild_virtualenv and
preflight_check, but snapshotting or integrity-checking a SQLite file needs
nothing beyond the stdlib sqlite3 module. Reusing it coupled the DATA-safety
paths to the venv-builder constraint and blocked updates for a reason unrelated
to the data:
* an all-Docker install returns before preflight_check ever runs and does
every real step in the container -- yet backup_docker_db snapshots with the
CONTAINER's python and then verified the extracted file with the HOST's.
On a stock EL9 (python3.9) or Ubuntu 22.04 (python3.10) host that halted
with "Could not obtain a VALID database backup", blaming the backup for a
missing interpreter.
* backup_current halts when it cannot snapshot, so those hosts could never
update at all.
Split out locate_sqlite_python: any candidate that can `import sqlite3` wins,
and the install's own venv interpreter is preferred -- on a real box it is
guaranteed present, guaranteed to carry the module, and independent of PATH.
locate_python keeps the 3.11/3.12 bound where it belongs.
Surfaced by the scripts-ci unit matrix: almalinux:9 and rockylinux:9 ship
python3.9, so the suite's own probe accepted an interpreter locate_python
rejected, and copy_sqlite_db/verify_sqlite_db failed there while passing on
python3.12 runners.
T10i pins the contract: on a box where every candidate reports 3.9 (locate_python
fails), the helpers still resolve an interpreter, prefer the slot's venv, and
round-trip 100 rows. Fails on the parent commit, passes here.
Verified on the full 7-image CI matrix (ubuntu 22.04/24.04, debian:12,
rockylinux:9, almalinux:9, fedora:40, opensuse/leap:15.5), all six shell suites:
0 failed. test_update is 63 passed / 0 failed on every image carrying a python3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the updater’s “pre-upgrade backup” layer so it reliably produces restorable database snapshots before proceeding, covering both SQLite (online backup API + integrity verification) and PostgreSQL (pg_dump + optional pg_restore validation + automatic restore on migration failure), while also limiting backup retention to prevent disk bloat. It also adds a GitHub Actions workflow to post merged PR summaries to Discord.
Changes:
- Replace racy live-SQLite
cpbackups with SQLite online backup API snapshots and enforce “no valid backup, no update”. - Add PostgreSQL pre-upgrade dumps, verification, and automatic restore on in-place migration failure.
- Add/update shell test coverage and stubs for the new backup/restore/retention behaviors; add Discord merged-PR notification workflow.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
VERSION |
Bumps ServerKit version to 1.7.71. |
scripts/update.sh |
Implements consistent SQLite snapshots + verification, PostgreSQL dump/restore safety net, and retention cap. |
scripts/test/test_update.sh |
Adds regression tests for SQLite snapshotting, PostgreSQL dump/restore, verification helpers, and retention pruning. |
scripts/test/stubs.sh |
Adds a deterministic pg_restore stub to make dump validation tests host-independent. |
.github/workflows/discord-notify.yml |
Posts merged PR info to Discord via webhook with event-payload parsing via jq. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -827,7 +932,40 @@ backup_current() { | |||
|
|
|||
| if [ -f "$db_file" ]; then | |||
Comment on lines
+842
to
+844
| # be the safety net. The SQLite online backup API snapshots a consistent DB | ||
| # even while the panel is writing. Python 3.11/3.12 is guaranteed by | ||
| # pre-flight; without it we fail rather than fall back to a racy cp. |
Comment on lines
+1643
to
+1645
| else | ||
| docker cp serverkit-backend:/app/instance/serverkit.db "$backup_file" 2>/dev/null || true | ||
| fi |
Comment on lines
+3
to
+5
| on: | ||
| pull_request: | ||
| types: [closed] # merged only (filtered in the job `if:`) |
…es exist
backup_current tested `[ -f serverkit.db ]` FIRST and only fell through to the
PostgreSQL branch when no file was there. An install migrated SQLite ->
PostgreSQL keeps its old serverkit.db, and both deploy paths copy it forward on
file-existence alone (deploy_source, deploy_release), so the stale file is
carried into every future slot and never ages out. Such a box backed up the
abandoned file, skipped pg_dump entirely, and left PRE_UPGRADE_DUMP unset --
which migrate_database reads as "${PRE_UPGRADE_DUMP:-}", so its auto-restore
silently did nothing. PostgreSQL is the one engine that migrates IN PLACE and
cannot roll back by switching slots, so that dump is its only way home: the
exact safety net a61eab1/b48256f added was disabled for it.
migrate_database has always keyed off DATABASE_URL (its sqlite check). Test the
declared engine first so the two functions agree.
Deliberately NOT `grep DATABASE_URL=sqlite && [ -f db ]` as the reviewer
suggested: backend/config.py defaults DATABASE_URL when absent, so any install
whose .env lacks the line would fall to the else branch and update with NO
backup at all -- trading a mismatched backup for none. An explicit `postgres`
URL is a positive signal; the absence of a `sqlite` one is not.
Also:
* name the engine when it is one we cannot dump. "No SQLite database ..."
read like a benign nothing-to-do on a MySQL install that in fact had no
safety net. Prints the scheme only -- a DATABASE_URL carries a password and
this text lands in the update log.
* drop the stale "Python 3.11/3.12 is guaranteed by pre-flight" comment,
contradicted by locate_sqlite_python two lines below it (664a5e6).
* discord-notify: skip cleanly when DISCORD_WEBHOOK_URL is absent. Secrets
are not exposed to `pull_request` runs from a fork, so a merged fork PR
failed the job on an empty curl URL. Not switching to pull_request_target
for this -- handing a write-scoped token and the webhook to an
outsider-triggered run is a poor trade for a chat notification.
Tests: a PostgreSQL install carrying a leftover serverkit.db must still dump
and set PRE_UPGRADE_DUMP (fails on the parent commit, rc=1); an undumpable
engine must be named without leaking the password. Neither needs python3, so
both run on all 7 images. Workflow verified end-to-end against a synthetic
event payload: fork PR exits 0 without calling curl, normal PR still builds the
same embed.
Verified on the full CI matrix (ubuntu 22.04/24.04, debian:12, rockylinux:9,
almalinux:9, fedora:40, opensuse/leap:15.5), all six suites: 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iccup
`docker run` pulls implicitly, so a registry timeout ("context deadline
exceeded" reaching registry-1.docker.io) killed the whole matrix leg with exit
125 before a single test ran, and cost a manual re-run. Seven images per matrix
run makes that a recurring tax rather than a one-off.
Pull up front with backoff (10/20/30s) and only then run the suites.
ONLY the pull is retried. Wrapping `docker run` would re-run the suites on
failure too, which would quietly turn a flaky TEST green -- the opposite of
what this job is for.
Left test-system-utils.yml alone: it uses a job-level `container:`, which the
runner pulls itself, so there is nothing to wrap.
Verified against the step text extracted from the YAML, under `bash -e` (the
shell GitHub actually uses for `run:`): registry down -> 3 retry warnings then
one error annotation and exit 1; flaky -> recovers and exits 0; healthy ->
exits 0 immediately with no retry noise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The retry added in 1f2381c behaved exactly as designed and still failed the job: Hub was unreachable for the full two minutes ("Client.Timeout exceeded while awaiting headers" on every attempt), not flaky. Retrying a single registry harder cannot survive that registry being down -- it only delays the failure. Two consecutive runs were blocked this way. Fall back to Google's public Hub mirror, a separate failure domain, and re-tag to the original name so the run step needs no changes (`docker run` resolves the local image; default pull policy is "missing"). Hub stays primary. Ref construction differs by image kind -- `mirror.gcr.io/library/<img>` for official images, `mirror.gcr.io/<ns>/<img>` for namespaced ones -- and all seven matrix images were confirmed to resolve on the mirror via `docker manifest inspect`, opensuse/leap included. ONLY the pull falls back. Wrapping `docker run` would re-run the suites on failure too, quietly turning a flaky TEST green. Verified locally end-to-end against the step text extracted from this YAML, under `bash -e`, with a docker shim reproducing the runner's exact Hub error: * Hub down, mirror up -> 3 warnings, REAL pull from mirror.gcr.io, re-tag, and `docker run <image>` then starts a container with the repo mounted while Hub is still blocked * both down -> 6 warnings, one error annotation, exit 1 (fails honestly) * Hub healthy -> exits 0 on the first pull, no mirror attempt, no noise Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The backup the updater took before every update turned out to be a photograph of a moving train. A real
1.7.35 → 1.7.65update failed and the root cause was a plaincpof the live SQLite database: on a panel that's actively writing, the copy can capture half a transaction, come outdatabase disk image is malformed, sink the migration in the new slot — and quietly poison the pre-upgrade backup that was supposed to be the way back. Pulling that thread found three more holes in the same layer: PostgreSQL installs got no backup at all (backup_currentlooked for a SQLite file, shrugged, and warned) even though their migration runs in place against a shared database; no backup of any kind was ever verified after being written; and ten full-size DB copies were kept forever, which is how a 25G droplet ended up carrying 6.3G of stale backups. The fix is the SQLite online backup API for every snapshot, apg_dumpfor PostgreSQL installs, an integrity check on every artifact before it's allowed to justify proceeding, and an automaticpg_restorewhen an in-place migration blows up — all under a deliberately blunt rule: no valid backup, no update. That rule then proved it could bite the wrong host, because the new SQLite helpers borrowedlocate_python, which enforces 3.11–3.12 for the venv builder; a stock EL9 or Ubuntu 22.04 box — or any all-Docker install, which returns before pre-flight ever runs — halted with "Could not obtain a VALID database backup" when the actual problem was the interpreter. Snapshotting a database needs the stdlibsqlite3module and nothing more, so the data-safety paths now resolve their own interpreter (preferring the install's own venv) and the 3.11/3.12 bound stays where it belongs. Rounding it out: two smaller updater bugs — a venv binding that read as unknown and forced needless rebuilds, and a broken Python shim that passed the version range check with an empty string — plus a workflow that posts merged PRs to Discord.Highlights
SERVERKIT_BACKUP_RETENTIONto change it) so old full-size copies stop quietly eating small disksTechnical changes
copy_sqlite_db()inscripts/update.shsnapshots through the SQLite online backup API (sqlite3.Connection.backup). It intentionally has no plain-cpfallback — falling back would reintroduce the exact race it exists to close.backup_current,deploy_sourceanddeploy_releaseall route the live DB through it. Both deploy pathshaltbefore any slot switch when the snapshot fails, anddeploy_sourceonly restores the preserved DB when the temp file is non-empty ([ -s "$tmp_db" ]), so a failed capture can't plant a zero-byte database in the new slot.backup_docker_dbsnapshots inside the container viadocker exec(tryingpythonthenpython3),docker cps the result out and cleans up/tmp/.sk-snap.db, keeping the rawdocker cponly as a fallback. Its previous softwarn-and-continue is now ahaltwhen no valid backup can be obtained.backup_currentgains a PostgreSQL branch:pg_dump -Fcwhen.envcarriesDATABASE_URL=postgres…, withpg_url_from_env()stripping SQLAlchemy driver suffixes (postgresql+psycopg2://→postgresql://) and wrapping quotes that libpq can't parse. A missingpg_dumphalts with an explicitpostgresql-clientinstall hint rather than proceeding unprotected.verify_sqlite_db()runsPRAGMA integrity_checkand requiresok;verify_pg_dump()requires a non-empty file plus a successfulpg_restore -lTOC listing whenpg_restoreis available. Both are called immediately after creation, and every failure pathrms the artifact so a partial file can never be mistaken for a good backup later.migrate_databaserecords the dump path inPRE_UPGRADE_DUMPand, when the migration fails against a shared external DB (use_slot_db != 1), runspg_restore --clean --if-exists -d "$pg_url"before halting — the slot model gives no untouched copy to fall back on when the schema change happened in place. If the restore itself fails, the halt message embeds the exact manualpg_restoreinvocation including the resolved URL and dump path.locate_sqlite_python()decouples the data-safety paths fromlocate_python's 3.11–3.12 bound: any candidate that canimport sqlite3wins, and caller-supplied hints are tried first.copy_sqlite_dbderives its hint from the source path (${src%/backend/instance/*}/venv/bin/python) andverify_sqlite_db— whose input lives inBACKUP_DIR, outside any slot — hints atactive_real_dir's venv instead. The install's own interpreter is guaranteed present, guaranteed to carry the module, and independent ofPATH.preflight_checkever runs and does every real step in the container, yetbackup_docker_dbsnapshotted with the container's Python and verified the extracted file with the host's. On python3.9/3.10 hosts the halt blamed the backup for a missing interpreter, and becausebackup_currenthalts when it cannot snapshot, those boxes could never update at all.require_venv's binding read swappedsed -n 's/^VIRTUAL_ENV="\(.*\)"$/\1/p'forgrep -m1 -oE 'VIRTUAL_ENV="?/[^")]+"?', matching both the quoted form and the unquotedexport VIRTUAL_ENV=/pathsome platforms emit, while skipping cygpath-style indirection. Previously the unquoted form resolved to an empty string, read as "bound elsewhere", and rebuilt the venv on every fresh install.locate_pythonrejects candidates whose version probe returns an empty string. A broken shim satisfiescommand -vbut can't execute Python, and""sorts before everything — so it passed bothsort -C -Vrange checks and got selected as the 3.11/3.12 interpreter.cleanupprunesserverkit-tree-*,serverkit-pre-upgrade-*.dband the newly addedserverkit-pre-upgrade-*.dumptoSERVERKIT_BACKUP_RETENTION(default 5, down from a hardcoded 10)..github/workflows/discord-notify.ymlposts an embed onpull_request: [closed], gated bygithub.event.pull_request.merged == trueso closed-unmerged PRs skip silently. Every PR field is read from$GITHUB_EVENT_PATHwithjqand passed to the payload via--arg, never interpolated into the shell — PR titles and bodies are attacker-controlled, and a$(...)or backtick in either would otherwise execute on the runner.[ \t]rather than[[:space:]]: the runner's awk is mawk, which silently mis-parses POSIX character classes. The description prefers the first five### Highlightsbullets, falls back to the first prose paragraph truncated to 300 chars, then to a placeholder, and is finally capped at 3900 chars to stay under Discord's 4096-char embed description limit.