diff --git a/backend/app/services/backup_service.py b/backend/app/services/backup_service.py index caa7cfd..ac59b93 100644 --- a/backend/app/services/backup_service.py +++ b/backend/app/services/backup_service.py @@ -24,11 +24,17 @@ from pathlib import Path _BACKUPS_DIRNAME = "_backups" -# Subprocess-level guardrails BEYOND the Celery soft_time_limit. The -# Celery soft limit signals the Python process; subprocess.Popen in a -# blocking syscall ignores that signal. These bound the worst case. -_DB_SUBPROCESS_TIMEOUT_S = 12 * 60 # 12 min (Celery soft is 10 min) -_IMAGES_SUBPROCESS_TIMEOUT_S = 7 * 60 * 60 # 7 hr (Celery soft is 6 hr) +# Subprocess-level guardrails BEYOND the Celery soft_time_limit. The Celery +# soft limit signals the Python process; subprocess.Popen in a blocking syscall +# ignores that signal, so these bound the worst case directly. Each sits just +# UNDER its task's Celery soft_time_limit so the bounded-kill (_run_bounded) is +# the primary guard and fires cleanly before Celery's soft/hard limits — which +# matters because an NFS D-state hang defeats even Celery's SIGKILL (the failure +# that wedged the maintenance lane for hours, #739). +# backup_db_task: soft=1800s / hard=2100s → 1700s +# backup_images_task: soft=21600s / hard=23400s → 21000s +_DB_SUBPROCESS_TIMEOUT_S = 1700 # ~28 min, under the 30-min DB soft limit +_IMAGES_SUBPROCESS_TIMEOUT_S = 21000 # ~5.8 hr, under the 6-hr images soft limit # Grace after SIGKILL to reap the child. If it can't be reaped in this window # (an uninterruptible NFS D-state — the failure mode that wedged the # concurrency-1 maintenance lane for hours, operator-flagged 2026-06-07), we @@ -115,18 +121,21 @@ def backup_db( to persist into BackupRun. Raises on subprocess failure.""" ts = _now_ts() out_dir = _backups_dir(images_root) - sql_path = out_dir / f"fc_db_{ts}.sql" + # Custom format (-Fc): compressed (much smaller on NFS) and restored with + # pg_restore. The .dump extension marks it as non-SQL. The BackupRun field + # is still named sql_path — it's just "the db artifact path". + sql_path = out_dir / f"fc_db_{ts}.dump" # Dump to LOCAL disk first, then move the finished file to the (NFS) backups # dir. pg_dump's long phase is then a DB-socket wait + local writes — both # killable — instead of an NFS write that can hang uninterruptibly. Only the # final move touches NFS, and it's a bounded single-file step. - fd, tmp_name = tempfile.mkstemp(prefix="fc_db_", suffix=".sql") + fd, tmp_name = tempfile.mkstemp(prefix="fc_db_", suffix=".dump") os.close(fd) tmp_path = Path(tmp_name) try: _run_bounded( [ - "pg_dump", "--no-owner", "--no-acl", + "pg_dump", "--no-owner", "--no-acl", "-Fc", "-f", str(tmp_path), _libpq_url(db_url), ], _DB_SUBPROCESS_TIMEOUT_S, @@ -184,8 +193,8 @@ def backup_images( def restore_db(*, db_url: str, sql_path: Path) -> None: - """Wipe public schema, then load from .sql. Raises on subprocess - failure; partial-restore state is the caller's concern.""" + """Wipe public schema, then load from the custom-format dump. Raises on + subprocess failure; partial-restore state is the caller's concern.""" libpq = _libpq_url(db_url) subprocess.run( [ @@ -194,8 +203,9 @@ def restore_db(*, db_url: str, sql_path: Path) -> None: ], capture_output=True, check=True, timeout=120, ) + # Custom-format (-Fc) dumps are restored with pg_restore, not psql. subprocess.run( - ["psql", libpq, "-f", str(sql_path)], + ["pg_restore", "--no-owner", "--no-acl", "-d", libpq, str(sql_path)], capture_output=True, check=True, timeout=_DB_SUBPROCESS_TIMEOUT_S, ) diff --git a/tests/test_backup_service.py b/tests/test_backup_service.py index 46b1ffc..cf45fb2 100644 --- a/tests/test_backup_service.py +++ b/tests/test_backup_service.py @@ -69,7 +69,7 @@ def test_backup_db_writes_sql_and_manifest(tmp_path, fake_subprocess): ) sql = Path(result["sql_path"]) manifest = Path(result["manifest_path"]) - assert sql.is_file() and sql.suffix == ".sql" + assert sql.is_file() and sql.suffix == ".dump" assert manifest.is_file() and manifest.suffix == ".json" assert result["kind"] == "db" assert result["tar_path"] is None @@ -92,6 +92,12 @@ def test_backup_db_strips_sqlalchemy_psycopg_driver(tmp_path, fake_subprocess): assert "+psycopg" not in cmd[-1] +def test_backup_db_uses_compressed_custom_format(tmp_path, fake_subprocess): + # -Fc → pg_restore-loadable, compressed dump (#739 backup polish). + backup_service.backup_db(db_url="postgresql://u@h/d", images_root=tmp_path) + assert "-Fc" in fake_subprocess[0] + + def test_backup_db_strips_asyncpg_driver(tmp_path, fake_subprocess): backup_service.backup_db( db_url="postgresql+asyncpg://u:p@h/d", images_root=tmp_path, @@ -132,17 +138,19 @@ def test_backup_images_excludes_backups_and_quarantine(tmp_path, fake_subprocess def test_restore_db_drops_schema_then_loads(tmp_path, fake_subprocess): - sql_path = tmp_path / "fake.sql" - sql_path.write_text("SELECT 1;") + dump_path = tmp_path / "fake.dump" + dump_path.write_bytes(b"\x00fake custom dump") backup_service.restore_db( - db_url="postgresql://u@h/d", sql_path=sql_path, + db_url="postgresql://u@h/d", sql_path=dump_path, ) - # Two psql calls: one with -c (DROP SCHEMA), one with -f (load). + # Two calls: psql -c (DROP SCHEMA), then pg_restore -d (load the dump). assert len(fake_subprocess) == 2 + assert fake_subprocess[0][0] == "psql" assert "-c" in fake_subprocess[0] assert "DROP SCHEMA IF EXISTS public CASCADE" in fake_subprocess[0][-1] - assert "-f" in fake_subprocess[1] - assert str(sql_path) in fake_subprocess[1] + assert fake_subprocess[1][0] == "pg_restore" + assert "-d" in fake_subprocess[1] + assert str(dump_path) in fake_subprocess[1] # --- restore_images -------------------------------------------------- diff --git a/tests/test_tasks_backup.py b/tests/test_tasks_backup.py index 4c285e3..583a202 100644 --- a/tests/test_tasks_backup.py +++ b/tests/test_tasks_backup.py @@ -93,7 +93,7 @@ async def test_backup_db_task_creates_backup_run_row_status_ok(db_sync): ).one() assert row.kind == "db" assert row.status == "ok" - assert row.sql_path and row.sql_path.endswith(".sql") + assert row.sql_path and row.sql_path.endswith(".dump") assert row.size_bytes is not None and row.size_bytes > 0 assert row.finished_at is not None assert row.error is None