Skip to content

fix: publish database backups atomically#9756

Open
jodylarsen wants to merge 2 commits into
paperclipai:masterfrom
jodylarsen:fix/atomic-backup-publication
Open

fix: publish database backups atomically#9756
jodylarsen wants to merge 2 commits into
paperclipai:masterfrom
jodylarsen:fix/atomic-backup-publication

Conversation

@jodylarsen

@jodylarsen jodylarsen commented Jul 17, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source control plane people use to manage AI-agent companies
  • Reliable logical database backups protect the control-plane state those companies depend on
  • Backup generation currently writes directly to the final .sql.gz path while compression is still running
  • A failed or interrupted producer can therefore leave an incomplete archive that looks published
  • Health checks also treat a valid gzip containing zero uncompressed bytes as healthy when it is fresh
  • This pull request stages both backup engines in unique same-directory files, atomically renames only completed output, and adds a dedicated empty-backup warning
  • The benefit is that operators and retention logic only see completed archives and can distinguish empty backup artifacts immediately

Linked Issues or Issue Description

What happened?

Interrupted pg_dump or JavaScript compression can expose a partial final archive; a valid empty gzip is not identified as unusable.

Expected behavior

Final backup filenames appear only after successful compression and process exit, temporary output is removed on handled failures, and empty valid gzip archives produce a dedicated warning.

Steps to reproduce

Start a backup with a dump producer that writes partial stdout and exits non-zero, then inspect the backup directory; separately place gzipSync(\"\") at the newest .sql.gz path and query health.

Paperclip version or commit

master at 5d42382d.

Deployment mode

All modes using logical database backups.

  • GitHub duplicate search found no related open issue or pull request.

What Changed

  • Stage gzip output for both pg_dump and JavaScript backup engines in unique files beside the destination, then rename after successful completion.
  • Wait for both the pg_dump child and compression pipeline to settle and clean staged output on failure without deleting an existing final archive.
  • Detect syntactically valid gzip archives with zero uncompressed bytes and expose database_backup_empty, including redacted health output.
  • Add regressions for failed pg_dump publication and valid empty-gzip health classification.

Verification

  • pnpm exec vitest run packages/db/src/backup-lib.test.ts server/src/__tests__/health.test.ts
  • Result: 2 test files passed; 19 tests passed.

Risks

  • Low risk. Final filenames, retention matching, and restore format remain unchanged. Atomic rename depends on staging in the destination directory, which this change guarantees.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See CONTRIBUTING.md.

Model Used

  • OpenAI GPT-5.6 Sol with tool-assisted reasoning and code execution; context-window size is not exposed by the runtime.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have not referenced internal/instance-local Paperclip issues or links (only public GitHub #NNN / github.com/paperclipai/paperclip URLs)
  • My branch name describes the change (e.g. docs/..., fix/...) and contains no internal Paperclip ticket id or instance-derived details
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

Made with Cursor

Keep incomplete gzip output hidden until backup generation succeeds and surface valid empty archives through health warnings.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-authored-by: Cursor <cursoragent@cursor.com>
@commitperclip

commitperclip Bot commented Jul 17, 2026

Copy link
Copy Markdown

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two related backup reliability gaps: incomplete archives could be observed at the final path during a failed write, and a valid gzip with zero uncompressed bytes passed health checks without any specific signal.

  • Both backup engines (pg_dump and the JS pipeline) now stage output to a UUID-suffixed temp file in the same directory, then atomically rename to the final .sql.gz path only after the write and process exit both succeed; cleanup on failure targets the temp file rather than the final archive.
  • Promise.all in runPgDumpBackup is replaced with Promise.allSettled so both pipeline and child-exit failures are collected, and an AggregateError is thrown when both fail simultaneously.
  • isValidEmptyGzip uses the gzip ISIZE trailer field as a cheap early-exit check, then confirms zero uncompressed bytes via gunzipSync with maxOutputLength: 1, emitting a database_backup_empty warning and redacted health message through the existing warning pipeline.

Confidence Score: 5/5

Safe to merge — the atomic rename pattern is correct, both temp paths are in the same directory as the destination ensuring POSIX rename atomicity, cleanup is properly guarded by existsSync, and the empty-gzip detector cannot false-positive on normal backups.

The core change is a well-scoped staged-rename pattern with no changes to final filenames, retention matching, or restore format. The ISIZE trailer check is implemented correctly, the gunzipSync short-circuit via maxOutputLength prevents decompressing large files, and both new behaviors are covered by dedicated regression tests.

No files require special attention.

Important Files Changed

Filename Overview
packages/db/src/backup-lib.ts Introduces UUID-based temporary files for both backup engines, renames atomically to the final path only on success, and uses Promise.allSettled to surface pg_dump exit-code errors alongside pipeline errors. Cleanup targets the staged path rather than the final archive, which is the core correctness fix.
server/src/services/database-backup-health.ts Adds isValidEmptyGzip, which reads the ISIZE trailer field (last 4 bytes of a gzip file) as a cheap pre-check before calling gunzipSync with maxOutputLength:1 to confirm zero uncompressed bytes; correctly avoids false positives on normal backups.
server/src/routes/health.ts Adds the redacted message for the new database_backup_empty warning code, keeping the exhaustive Record type constraint intact.
packages/db/src/backup-lib.test.ts Adds a regression test that stubs pg_dump with a script that writes partial output and exits 17, then asserts that the backup directory remains empty — directly verifying the atomic publication guarantee.
server/src/tests/health.test.ts Adds a test that places a valid empty gzip at the expected backup path and asserts the health endpoint surfaces database_backup_empty at warning status with the correct message.

Reviews (2): Last reviewed commit: "fix: preserve concurrent backup failures" | Re-trigger Greptile

Comment thread packages/db/src/backup-lib.ts Outdated
Comment thread server/src/services/database-backup-health.ts
@jodylarsen

Copy link
Copy Markdown
Author

CTO review: changes requested before approval.

  • Preserve both failure causes when the gzip pipeline and pg_dump exit fail together (for example, throw an AggregateError). Operational excellence requires retaining the actionable child exit code rather than silently dropping it.
  • Add the required upstream issue reference or inline issue-template description to the PR body so the commitperclip PR Review gate passes.
  • Re-run the focused backup-library and health tests after the code change. The current unrelated heartbeat-process-recovery.test.ts CI failure does not touch this diff; rerun the failed job after the new commit.

The atomic same-directory staging/rename behavior and dedicated empty-gzip warning otherwise match the requested blast-radius constraints. The second Greptile note (double read for the rare ISIZE-zero path) is non-blocking.

Retain both compression and pg_dump diagnostics when they fail together so operators can act on every underlying cause.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-authored-by: Cursor <cursoragent@cursor.com>
@jodylarsen

Copy link
Copy Markdown
Author

CTO re-review approved at ee65c56. The concurrent-failure path now preserves both causes with AggregateError; policy, Greptile, build, typecheck, server/workspace tests, serialized suites, canary, and security checks pass. Focused backup/health verification is 19/19. GitHub does not allow this authenticated PR author to submit a formal approval. The remaining e2e job is still running and remains a merge gate; do not merge until it passes.

@jodylarsen

Copy link
Copy Markdown
Author

All required CI is now complete and green at ee65c56e, including e2e. The change has passed CTO re-review and the focused backup/health verification is 19/19.

@cryppadotta, please perform the upstream maintainer merge review. The authenticated PR author cannot merge, enable auto-merge, request a formal review, or submit a self-approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant