Skip to content

fix(backups): Fix deleting all backups instead of keeping n latest#3856

Open
jirkavrba wants to merge 1 commit intoDokploy:canaryfrom
jirkavrba:fix/keep-latest-n-backups
Open

fix(backups): Fix deleting all backups instead of keeping n latest#3856
jirkavrba wants to merge 1 commit intoDokploy:canaryfrom
jirkavrba:fix/keep-latest-n-backups

Conversation

@jirkavrba
Copy link
Contributor

@jirkavrba jirkavrba commented Mar 1, 2026

What is this PR about?

Fix backup deleting all existing backups, not just the x latest.

Described in the issue #3855

Checklist

Before submitting this PR, please make sure that:

  • You created a dedicated branch based on the canary branch.
  • You have read the suggestions in the CONTRIBUTING.md file https://github.com/Dokploy/dokploy/blob/canary/CONTRIBUTING.md#pull-request
  • You have tested this PR in your local instance. If you have not tested it yet, please do so before submitting. This helps avoid wasting maintainers' time reviewing code that has not been verified by you.

Issues related (if applicable)

closes #3855

Screenshots (if applicable)

Greptile Summary

This PR fixes a critical bug where path.join() was used to construct S3 paths in the backup cleanup logic. The Node.js path.join() function is designed for filesystem paths and causes issues with S3 URLs: if the prefix contains a leading slash, path.join() treats it as an absolute path and discards the bucket portion, resulting in an invalid S3 path. This could cause rclone to fail or behave incorrectly when deleting old backups.

The fix:

  • Imports and uses normalizeS3Path() which properly handles S3 path semantics (removes leading/trailing slashes, adds trailing slash)
  • Constructs the S3 path with template literals instead of path.join()
  • Ensures cross-platform compatibility and correct S3 path formatting

No issues found. The implementation is clean and the normalizeS3Path() function properly handles edge cases.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - it's a targeted bug fix that replaces filesystem path logic with proper S3 path handling
  • The fix directly addresses a specific bug by replacing path.join() (designed for filesystem paths) with normalizeS3Path() (designed for S3 paths). The implementation is straightforward, uses existing utility functions, and doesn't introduce new complexity or side effects. The change is well-scoped to the problematic code path.
  • No files require special attention

Last reviewed commit: bf9d019

(5/5) You can turn off certain types of comments like style here!

@jirkavrba jirkavrba requested a review from Siumauricio as a code owner March 1, 2026 18:51
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Mar 1, 2026
@dosubot
Copy link

dosubot bot commented Mar 1, 2026

Related Documentation

Checked 7 published document(s) in 1 knowledge base(s). No updates required.

How did I do? Any feedback?  Join Discord

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

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

Labels

bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instead of keeping x latest backups, all database + dokploy web server backups are deleted

1 participant