From f81a80e3257a325496a5200c48ae5d58154014ab Mon Sep 17 00:00:00 2001 From: le king fu Date: Sat, 10 Oct 2026 19:03:48 -0400 Subject: [PATCH] fix(agent): anchor the backup rotation on the newest backup, not today The rotation cutoff was TODAY minus 29 days. When the VPS stops producing (dead cron, failing pg_dump) or this machine's clock jumps ahead, the last off-site copy aged out after 30 days, the pass exited 0 and the report said ok on a directory it had just deleted. The cutoff now comes from the newest dated directory held here, which is never removed. Review suggestions: - --prune-empty-dirs: a dated directory with no .age file is not created, and one found empty logs a warning; - files counts the dated directory only (-maxdepth 1), not partial files left in .rsync-partial/; - an EXIT trap removes the header file holding the ingest token, with TERM/INT/HUP turned into an exit so a stop by systemd runs it. Refs maximus/la-compagnie-maximus#220 Co-Authored-By: Claude Opus 5.5 --- __tests__/agent-pg-backup-pull.test.js | 72 +++++++++++++++++++++- agent/README.md | 16 +++-- agent/pg-backup-pull/run-pg-backup-pull.sh | 33 ++++++++-- 3 files changed, 110 insertions(+), 11 deletions(-) diff --git a/__tests__/agent-pg-backup-pull.test.js b/__tests__/agent-pg-backup-pull.test.js index e287398..1f00adb 100644 --- a/__tests__/agent-pg-backup-pull.test.js +++ b/__tests__/agent-pg-backup-pull.test.js @@ -128,6 +128,7 @@ describe("pull", () => { expect(argv.indexOf("--include=*/")).toBeLessThan(argv.indexOf("--exclude=*")); expect(argv.indexOf("--include=*.age")).toBeLessThan(argv.indexOf("--exclude=*")); expect(argv).not.toContain("--delete"); + expect(argv).toContain("--prune-empty-dirs"); expect(argv).toContain("pgbackup@100.86.30.21:"); const ssh = argv.find((a) => a.startsWith("ssh ")); expect(ssh).toContain("-p 2222"); @@ -167,6 +168,24 @@ describe("pull", () => { expect(output).toContain(`warning: no directory for ${TODAY}, latest is 2026-10-09`); }); + test("a dated directory without any .age file logs a warning", () => { + fs.writeFileSync(path.join(bin, "rsync"), `#!/bin/sh\nfor last; do :; done\nmkdir -p "$last/${TODAY}"\n`); + const { status, output } = run(); + expect(status).toBe(0); + expect(output).toContain(`latest=${TODAY} files=0`); + expect(output).toContain(`warning: no .age file in ${TODAY}`); + }); + + test("a partial file left by rsync is not counted", () => { + fs.writeFileSync( + path.join(bin, "rsync"), + `#!/bin/sh\nfor last; do :; done\nmkdir -p "$last/${TODAY}/.rsync-partial"\n` + + `echo x > "$last/${TODAY}/a.age"\necho x > "$last/${TODAY}/.rsync-partial/b.age"\n`, + ); + const { output } = run(); + expect(output).toContain(`latest=${TODAY} files=1`); + }); + test("no warning when today's directory is there", () => { const { output } = run(); expect(output).toContain(`latest=${TODAY} files=2`); @@ -174,10 +193,11 @@ describe("pull", () => { }); // The real rsync with the script's own filter, against a local outbox through - // the fake ssh: only .age files land, inside their dated directory. Skipped + // the fake ssh: only .age files land, inside their dated directory, and a + // dated directory holding no .age file is not created. Skipped // where rsync is absent (the CI image); the argv order test above runs there. test.skipIf(!["/usr/bin/rsync", "/bin/rsync"].some((f) => fs.existsSync(f)))( - "with the real rsync, only .age files cross, inside their dated directory", + "with the real rsync, only .age files cross, inside their dated directory, never an empty one", () => { fs.rmSync(path.join(bin, "rsync")); const outbox = path.join(tmp, "outbox"); @@ -185,6 +205,8 @@ describe("pull", () => { fs.writeFileSync(path.join(outbox, TODAY, "maximus_api.dump.age"), "cipher"); fs.writeFileSync(path.join(outbox, TODAY, "maximus_api.dump"), "clear"); fs.writeFileSync(path.join(outbox, "stray.sql"), "clear"); + fs.mkdirSync(path.join(outbox, "2026-10-12")); + fs.writeFileSync(path.join(outbox, "2026-10-12", "maximus_api.dump"), "clear"); const { status, output, dest } = run({ extraEnv: { FAKE_OUTBOX: outbox } }); expect(status).toBe(0); expect(fs.readdirSync(dest)).toEqual([TODAY]); @@ -220,6 +242,32 @@ describe("report", () => { expect(fs.readdirSync(path.join(tmp, "state"))).not.toContain("auth.header"); }); + // systemd stops a unit with SIGTERM. The fake curl sends it to the script + // while the header file exists; the trap must still remove it. + test("a TERM while the report is in flight still removes the header file", () => { + writeExecutable( + path.join(bin, "curl"), + `#!/bin/sh +cat > /dev/null +# Climb from the parent (the script, or a subshell of it, same cmdline) while +# the parent is still the script: the last one is the script itself. +ppid_of() { sed 's/.*) //' "/proc/$1/stat" | cut -d' ' -f2; } +script=$PPID +while parent=$(ppid_of "$script") && tr '\\0' ' ' < "/proc/$parent/cmdline" | grep -q run-pg-backup-pull; do + script=$parent +done +[ -f "$FAKE_DIR/state/auth.header" ] && echo present > "$FAKE_DIR/header.seen" +kill -TERM "$script" +sleep 1 +printf 204 +`, + ); + const { status } = run({ envFile: writeEnv() }); + expect(read("header.seen")).toBe("present\n"); + expect(status).toBe(143); + expect(fs.readdirSync(path.join(tmp, "state"))).not.toContain("auth.header"); + }); + test("a non-2xx answer exits 3", () => { const { status, output } = run({ envFile: writeEnv(), extraEnv: { FAKE_CURL_STATUS: "401" } }); expect(status).toBe(3); @@ -240,6 +288,26 @@ describe("rotation", () => { expect(fs.existsSync(path.join(dest, "notes"))).toBe(true); }); + // The VPS stopped producing on 2026-09-01 (dead cron, failing pg_dump): its + // outbox yields nothing new. Anchored on today, the rotation would delete the + // last off-site copy; anchored on the newest backup, it keeps it and the 29 + // days before, and only removes what is older. + test("never removes the newest backup, however old: the window ends at the newest directory", () => { + const dest = path.join(tmp, "dest"); + for (const d of ["2026-08-02", "2026-08-03", "2026-09-01"]) { + fs.mkdirSync(path.join(dest, d), { recursive: true }); + fs.writeFileSync(path.join(dest, d, "a.age"), "x"); + } + fs.writeFileSync(path.join(bin, "rsync"), "#!/bin/sh\nexit 0\n"); + const { status, output } = run({ envFile: writeEnv() }); + expect(status).toBe(0); + expect(output).toContain("latest=2026-09-01 files=1"); + expect(fs.existsSync(path.join(dest, "2026-09-01", "a.age"))).toBe(true); + expect(fs.existsSync(path.join(dest, "2026-08-03"))).toBe(true); + expect(fs.existsSync(path.join(dest, "2026-08-02"))).toBe(false); + expect(JSON.parse(read("curl.body"))).toMatchObject({ ok: true, files: 1 }); + }); + // The failure comes from a fake rm, not from file modes: the CI runs as // root, which ignores a read-only directory. test("a directory that cannot be removed fails loudly and reports rotation_failed", () => { diff --git a/agent/README.md b/agent/README.md index ee2a449..3fcc4b5 100644 --- a/agent/README.md +++ b/agent/README.md @@ -417,7 +417,8 @@ BACKUPS_INGEST_TOKEN= ``` The token reaches curl through a header file created `0600` and removed right -after, never through argv. +after, never through argv. An `EXIT` trap removes it too when the run is +stopped mid-report (systemd's `SIGTERM` is turned into an exit first). ### Check @@ -429,9 +430,16 @@ systemctl --user list-timers maximus-pg-backup-pull.timer A pass whose newest dated directory is not today's (UTC) logs `warning: no directory for , latest is `: the VPS stopped -producing, or the pass ran before its 03:00 UTC run. The report still says -`ok` with the newest directory's count; freshness is judged on the VPS side -by `GET /backups`. +producing, or the pass ran before its 03:00 UTC run. A dated directory with no +`.age` file is never created (`--prune-empty-dirs`), and one found empty logs +`warning: no .age file in `. The report still says `ok` with the newest +directory's count (partial files in `.rsync-partial/` not counted); freshness +is judged on the VPS side by `GET /backups`. + +**Rotation is anchored on the newest backup held here, not on today's date.** +It keeps that directory and the 29 days before it. If the VPS stops producing +(dead cron, failing `pg_dump`) or this machine's clock jumps ahead, the last +off-site copy stays put instead of aging out after 30 days. The script sticks to what busybox offers (no `find -printf`, no `date -d "... days"`): the CI runs its tests in `node:22-alpine`. diff --git a/agent/pg-backup-pull/run-pg-backup-pull.sh b/agent/pg-backup-pull/run-pg-backup-pull.sh index c4cc2c0..8e06838 100755 --- a/agent/pg-backup-pull/run-pg-backup-pull.sh +++ b/agent/pg-backup-pull/run-pg-backup-pull.sh @@ -57,6 +57,16 @@ if ! flock -n 9; then exit 0 fi +# The ingest token sits in this file for the length of one curl call. The trap +# removes it on any exit, a kill by systemd included: the signal traps turn +# TERM/INT/HUP into a plain exit, which runs the EXIT trap (dash does not run +# it on a signal by itself). +header_file="$STATE_DIR/auth.header" +trap 'rm -f "$header_file"' EXIT +trap 'exit 129' HUP +trap 'exit 130' INT +trap 'exit 143' TERM + # The env file is optional until the ingest route is deployed: without a token, # the pull still runs and only the report to the health API is skipped. BACKUPS_API_URL="" @@ -87,7 +97,6 @@ report() { body=$(printf '{"pulledAt":"%s","files":%s,"ok":%s}' "$pulled_at" "$2" "$1") fi # The token goes through a header file, never through argv (visible in ps). - header_file="$STATE_DIR/auth.header" (umask 077 && printf 'Authorization: Bearer %s\n' "$BACKUPS_INGEST_TOKEN" > "$header_file") status=$(printf '%s' "$body" | curl -sS -o /dev/null -w '%{http_code}' \ --max-time "$POST_TIMEOUT_SECONDS" \ @@ -103,7 +112,9 @@ report() { # --- Pull --------------------------------------------------------------------- # Only directories and .age files cross: a misconfigured outbox can never land # a clear dump here. No --delete: the VPS keeps 7 days, this machine keeps 30. -if ! timeout "$RSYNC_TIMEOUT_SECONDS" rsync -rt --partial-dir=.rsync-partial \ +# --prune-empty-dirs: a dated directory with no .age file (encryption failed on +# the VPS) is not created here, so it cannot pass for today's backup. +if ! timeout "$RSYNC_TIMEOUT_SECONDS" rsync -rt --prune-empty-dirs --partial-dir=.rsync-partial \ --include='*/' --include='*.age' --exclude='*' \ -e "ssh -p $SSH_PORT -i $SSH_KEY -o IdentitiesOnly=yes -o UserKnownHostsFile=$KNOWN_HOSTS -o StrictHostKeyChecking=yes -o BatchMode=yes -o ConnectTimeout=20 -o ServerAliveInterval=15 -o ServerAliveCountMax=4" \ "$SOURCE" "$DEST/"; then @@ -135,7 +146,8 @@ for dir in "$DEST"/*/; do done files=0 if [ -n "$latest" ]; then - files=$(find "$DEST/$latest" -type f -name '*.age' | wc -l | tr -d ' ') + # -maxdepth 1: a partial file left in .rsync-partial/ is not a backup. + files=$(find "$DEST/$latest" -maxdepth 1 -type f -name '*.age' | wc -l | tr -d ' ') fi log "pull ok latest=${latest:-none} files=$files" # The report counts the newest local directory: if the VPS stops producing, a @@ -144,13 +156,24 @@ log "pull ok latest=${latest:-none} files=$files" if [ "${latest:-}" != "$TODAY" ]; then log "warning: no directory for $TODAY, latest is ${latest:-none}" fi +if [ -n "$latest" ] && [ "$files" -eq 0 ]; then + log "warning: no .age file in $latest" +fi # --- Rotation (fails loudly) -------------------------------------------------- +# Anchored on the newest backup held here, never on today's date: when the VPS +# stops producing (dead cron, failing pg_dump), or this machine's clock jumps +# ahead, a TODAY-based cutoff would delete the last off-site copy. This keeps +# the 30 days before the newest backup, which itself is never removed. # Epoch arithmetic: `date -d YYYY-MM-DD` and `date -d @N` are read by GNU date # and busybox alike. -today_epoch=$(date -u -d "$TODAY" +%s) -cutoff=$(date -u -d "@$((today_epoch - (RETENTION_DAYS - 1) * 86400))" +%F) rotation_ok=true +cutoff="" +if [ -n "$latest" ]; then + latest_epoch=$(date -u -d "$latest" +%s) + cutoff=$(date -u -d "@$((latest_epoch - (RETENTION_DAYS - 1) * 86400))" +%F) +fi +# No latest means no dated directory: the loop below then compares nothing. for dir in "$DEST"/*/; do [ -d "$dir" ] || continue name=$(basename "$dir")