fix(backups): Every server has its own backup dir - #202
Merged
Conversation
📝 WalkthroughWalkthroughBackup adapters and router operations now carry the server UUID. Local backup paths now include a server-specific directory. Tests update constructor calls and local path coverage. ChangesServer-scoped backup handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/backup/backup_test.go (1)
46-46: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert server-specific storage, not only containment.
The current assertion passes if
Path()ignoresServerUuidand stores the archive directly underbackupDir. Assert the expected server directory and verify that two server UUIDs produce different paths.Suggested test assertion
+ const serverUuid = "ce6ee345-6729-4aed-8fed-c866c535a69d" for _, identifier := range []string{ "11111111-1111-1111-1111-111111111111", "../target/archive", "nested/archive", } { - b := NewLocal(nil, identifier, "ce6ee345-6729-4aed-8fed-c866c535a69d", "") + b := NewLocal(nil, identifier, serverUuid, "") + if got, want := filepath.Dir(b.Path()), filepath.Join(backupDir, serverUuid); got != want { + t.Fatalf("expected backup directory %q, got %q", want, got) + } + other := NewLocal(nil, identifier, "22222222-2222-2222-2222-222222222222", "") + if b.Path() == other.Path() { + t.Fatalf("expected server-scoped backup paths to differ") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/backup/backup_test.go` at line 46, Strengthen the test around NewLocal and Path so it asserts the archive path includes the expected server-specific directory, rather than merely checking containment under backupDir. Create local backups with two different server UUIDs and verify their resulting paths differ, while preserving the existing path expectations for the same server.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/backup/backup.go`:
- Line 126: Make directory creation in the backup generation paths race-safe for
concurrent calls from postServerBackup. Update the LocalBackup.Generate and
S3Backup.Generate flows to create filepath.Dir(b.Path()) and
filepath.Dir(s.Path()) with os.MkdirAll, or explicitly tolerate os.IsExist,
while preserving existing error handling for other failures.
- Line 126: Update the Path() construction to validate and canonicalize
b.ServerId() as a UUID path component before passing it to path.Join, rather
than using the raw server ID. Preserve normalizedIdentifier() validation and
ensure filesystem paths, download tokens, and restoration targets cannot
interpret .. or path separators as directory traversal.
- Line 126: Update the backup path/lookup flow around Path() and LocateLocal()
to retain access to legacy archives stored directly under BackupDirectory.
Prefer the new server-UUID subdirectory path, then fall back to the legacy
backup-UUID path when the new archive is absent, so download, restore, and
delete continue working for existing archives.
---
Nitpick comments:
In `@server/backup/backup_test.go`:
- Line 46: Strengthen the test around NewLocal and Path so it asserts the
archive path includes the expected server-specific directory, rather than merely
checking containment under backupDir. Create local backups with two different
server UUIDs and verify their resulting paths differ, while preserving the
existing path expectations for the same server.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5de633c9-fe98-4c81-949a-fdd88232f1e8
📒 Files selected for processing (6)
router/router_download.gorouter/router_server_backup.goserver/backup/backup.goserver/backup/backup_local.goserver/backup/backup_s3.goserver/backup/backup_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.5, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.12, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.12, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.5, linux, amd64)
- GitHub Check: Test macOS (1.25.12)
- GitHub Check: Test macOS (1.26.5)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/backup/backup_test.goserver/backup/backup_local.goserver/backup/backup.goserver/backup/backup_s3.go
🪛 ast-grep (0.45.0)
server/backup/backup.go
[warning] 146-146: SHA-1 is a cryptographically broken hash function vulnerable to collision attacks and is unsuitable for security purposes such as signatures, integrity checks, or password hashing. Use a SHA-2 family function (e.g. sha256.New() / sha256.Sum256()) or SHA-3 instead.
Context: sha1.New
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(weak-hash-sha1-go)
🔇 Additional comments (6)
server/backup/backup.go (1)
131-133: LGTM!Also applies to: 146-146
server/backup/backup_local.go (1)
24-39: LGTM!Also applies to: 59-59, 87-87, 117-117
server/backup/backup_s3.go (1)
29-36: LGTM!Also applies to: 45-45, 73-73
router/router_server_backup.go (1)
59-61: LGTM!Also applies to: 143-143, 184-192, 204-204, 226-226, 369-369
router/router_download.go (1)
46-46: LGTM!server/backup/backup_test.go (1)
18-21: LGTM!Also applies to: 103-103
QuintenQVD0
marked this pull request as draft
August 7, 2026 10:28
QuintenQVD0
marked this pull request as ready for review
August 8, 2026 08:16
parkervcp
approved these changes
Aug 8, 2026
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
2 participants
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.
Changes
Summary by CodeRabbit