fix: prevent system backups from swallowing backup_dir recursively (+ split config mounts) #128

Merged

fastie81 merged 2 commits from fix/backup-dir-collision into main 2026-09-05 20:46:33 +12:00

Conversation 1 Commits 2 Files changed 6 +157 -106

fastie81 commented 2026-09-05 20:35:54 +12:00

Owner

Copy link

Root cause

On prod (Tower 192.168.10.10) the running container binds BOTH /app/backups and /app/app/data/content_images to the SAME host folder (/mnt/user/appdata/honbu-manager/backup). In create_system_backup() the content walk (os.walk(self.content_dir)) then embeds every prior system_backup_.zip inside the new archive - runaway GB growth. The job dies mid-write (OOM on the giant archive), leaving a truncated, unrestorable zip (no EOCD). All 3 existing system_backup_.zip on prod are corrupt (verified: 87M/126M/145M, all rejected by zipfile/unzip). The scheduled daily backup has silently failed since Jul 8.

Fixes (2 commits)

1. backup_service.py - code

• Scratch dir: database.dump and club_data.json now write to tempfile.mkdtemp() OUTSIDE backup_dir.
• Recursion guard: the content walk _is_within-filters anything under backup_dir and scratch, so an archive can never contain another archive regardless of how dirs are bound.
• Atomic publish: zip written to .part then os.replace() to final name - a killed process can no longer leave a partial zip (.part cleaned in except handler).
• Integrity check: zipfile.testzip() after close; fails loudly (and deletes) rather than silently accepting a corrupt archive.

2. docker-compose.yml + env templates - config

The compose fed both mounts from the SAME ${HOST_DATA_PATH} variable - the exact mis-config live on prod. Split into BACKUP_PATH (-> /app/backups) and CONTENT_PATH (-> /app/app/data/content_images) with distinct defaults so they can never collapse. Documented in .env.example, .env.prod, .env.test.

Test

• Existing tests/unit/test_backup_service.py: 17 passed.
• NEW regression test TestSystemBackupRecursionGuard reproduces the prod mis-config (content_dir == backup_dir), asserts the result is a valid archive, contains database.dump, contains NO embedded .zip, leaves no .part leftover and no stray database.dump. 18 passed total.

Follow-on (separate)

Deploy to prod requires setting BACKUP_PATH/CONTENT_PATH to distinct real paths in .env.prod and restarting the compose stack. Registry-based CI prod-deploy migration tracked separately.

## Root cause On prod (Tower 192.168.10.10) the running container binds BOTH /app/backups and /app/app/data/content_images to the SAME host folder (/mnt/user/appdata/honbu-manager/backup). In create_system_backup() the content walk (os.walk(self.content_dir)) then embeds every prior system_backup_*.zip inside the new archive - runaway GB growth. The job dies mid-write (OOM on the giant archive), leaving a truncated, unrestorable zip (no EOCD). All 3 existing system_backup_*.zip on prod are corrupt (verified: 87M/126M/145M, all rejected by zipfile/unzip). The scheduled daily backup has silently failed since Jul 8. ## Fixes (2 commits) ### 1. backup_service.py - code - Scratch dir: database.dump and club_data.json now write to tempfile.mkdtemp() OUTSIDE backup_dir. - Recursion guard: the content walk _is_within-filters anything under backup_dir and scratch, so an archive can never contain another archive regardless of how dirs are bound. - Atomic publish: zip written to <file>.part then os.replace() to final name - a killed process can no longer leave a partial zip (.part cleaned in except handler). - Integrity check: zipfile.testzip() after close; fails loudly (and deletes) rather than silently accepting a corrupt archive. ### 2. docker-compose.yml + env templates - config The compose fed both mounts from the SAME ${HOST_DATA_PATH} variable - the exact mis-config live on prod. Split into BACKUP_PATH (-> /app/backups) and CONTENT_PATH (-> /app/app/data/content_images) with distinct defaults so they can never collapse. Documented in .env.example, .env.prod, .env.test. ## Test - Existing tests/unit/test_backup_service.py: 17 passed. - NEW regression test TestSystemBackupRecursionGuard reproduces the prod mis-config (content_dir == backup_dir), asserts the result is a valid archive, contains database.dump, contains NO embedded .zip, leaves no .part leftover and no stray database.dump. 18 passed total. ## Follow-on (separate) Deploy to prod requires setting BACKUP_PATH/CONTENT_PATH to distinct real paths in .env.prod and restarting the compose stack. Registry-based CI prod-deploy migration tracked separately.

fastie81 added 2 commits 2026-09-05 20:35:55 +12:00

fix: prevent system backups from swallowing backup_dir recursively ... 4ddeccb321

Root cause: when CONTENT_UPLOAD_DIR is bound to the same host folder as the
backup directory (as on prod), create_system_backup() walks and embeds prior
system_backup_*.zip files inside the new archive, causing runaway GB growth and
truncated/corrupt archives (killed mid-write, no EOCD).

Changes:
- Dump database and club JSON into a scratch dir outside backup_dir
- Skip backup_dir during the content walk (recursion guard via _is_within)
- Publish final zip with atomic .part -> rename (no partial zips)
- Verify integrity with zipfile.testzip(); fail loudly on corruption
- Regression test: content_dir == backup_dir still yields a valid archive
  that never contains another .zip

config: split backup_dir and content_dir mounts into separate vars ... b686a4ac26

The compose file fed both /app/backups and /app/app/data/content_images from
the same ${HOST_DATA_PATH} variable. If set, both collapses to one host folder;
that is the exact mis-config live on prod that made system backups recursively
embed themselves. Split into BACKUP_PATH and CONTENT_PATH with distinct
defaults, and document both in the env templates.

fastie81 force-pushed fix/backup-dir-collision from b686a4ac26 to 1fe881643c 2026-09-05 20:38:48 +12:00 Compare

fastie81 commented 2026-09-05 20:46:21 +12:00

Author

Owner

Copy link

CI verified green on this PR (post-rebase onto main after #127 merged):

• test ✅ (2m13s push / 2m12s PR)
• lint-frontend ✅ (57s push / 1m1s PR)
• build-test / build-release non-gating; deploy-test / deploy-prod correctly blocked (gated to protected branch/version tags)

Note: the push-run failure earlier was the pre-existing DATABASE_URL placeholder bug (#127), not this branch — resolved by merging #127 and rebasing. ~200 lines total (code + regression test + config split), mergeable clean against main.

CI verified green on this PR (post-rebase onto main after #127 merged): - `test` ✅ (2m13s push / 2m12s PR) - `lint-frontend` ✅ (57s push / 1m1s PR) - `build-test` / `build-release` non-gating; `deploy-test` / `deploy-prod` correctly blocked (gated to protected branch/version tags) Note: the push-run failure earlier was the pre-existing `DATABASE_URL` placeholder bug (#127), not this branch — resolved by merging #127 and rebasing. ~200 lines total (code + regression test + config split), mergeable clean against `main`.

fastie81 merged commit 657aa8b5ae into main 2026-09-05 20:46:33 +12:00

fastie81 referenced this pull request from a commit 2026-09-05 20:46:41 +12:00

Merge pull request 'fix: prevent system backups from swallowing backup_dir recursively (+ split config mounts)' (#128) from fix/backup-dir-collision into main

fastie81 referenced this pull request 2026-09-05 21:13:30 +12:00

ci: push v<semver> tag so deploy-prod can pull it; point .env.prod at real prod paths #129

fastie81 referenced this pull request 2026-09-05 21:18:15 +12:00

ci: push v<semver> tag so deploy-prod can pull it; point .env.prod at real prod paths #129

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

architecture

Architecture change

  

backend

Backend (Python/Flask)

  

bug

Something is not working

  

ci

CI/CD pipeline

  

config

Configuration issue

  

database

Database related

  

docker

Docker/container related

  

documentation

Documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

frontend

Frontend (React/TypeScript)

  

help wanted

Need some help

  

invalid

Something is wrong

  

issue-25

Related to issue #25

  

issue-26

Related to issue #26

  

issue-42

Related to issue #42

  

issue-65

Related to issue #65

  

issue-67

Related to issue #67

  

issue-82

Related to issue #82

  

migrations

Database migrations

  

p0-critical

Critical priority - immediate fix needed

  

p1-high

High priority

  

p2-medium

Medium priority

  

p3-low

Low priority

  

performance

Performance issue

  

quality

Code quality

  

question

More information is needed

  

security

Security issue

  

state-management

Frontend state management

  

testing

Testing related

  

timezone

Timezone related

  

wontfix

This won't be fixed

No labels

architecture

backend

bug

ci

config

database

docker

documentation

duplicate

enhancement

frontend

help wanted

invalid

issue-25

issue-26

issue-42

issue-65

issue-67

issue-82

migrations

p0-critical

p1-high

p2-medium

p3-low

performance

quality

question

security

state-management

testing

timezone

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

fastie81/honbu-manager!128

No description provided.