Skip to content

refactor(backup): convert sync fs operations to async promises#237

Merged
BillChirico merged 4 commits intomainfrom
fix/backup-async-clean
Mar 3, 2026
Merged

refactor(backup): convert sync fs operations to async promises#237
BillChirico merged 4 commits intomainfrom
fix/backup-async-clean

Conversation

@BillChirico
Copy link
Collaborator

Summary

Converts src/modules/backup.js from synchronous file system operations to async fs.promises for better performance and non-blocking I/O.

Changes

src/modules/backup.js

  • Convert getBackupDir() to async using access() and mkdir()
  • Convert createBackup() to async using writeFile() and stat()
  • Convert parseBackupMeta() to async using stat()
  • Convert listBackups() to async
  • Convert readBackup() to async
  • Convert pruneBackups() to async

src/api/routes/backup.js

  • Add async/await to all route handlers that call backup functions
  • Ensures proper handling of Promise returns from async backup operations

Verification

  • All backup API endpoints properly await async operations
  • No behavioral changes - same functionality
  • Addresses TODO comment in original backup.js

Closes code quality improvement from CODE_IMPROVEMENTS.md analysis

Bill added 2 commits March 3, 2026 10:50
Addresses TODO in backup.js to switch from sync fs operations to
fs.promises for better performance and non-blocking I/O.

- Convert getBackupDir, createBackup, parseBackupMeta to async
- Replace existsSync/mkdirSync with access/mkdir promises
- Replace writeFileSync/statSync with async equivalents
Copilot AI review requested due to automatic review settings March 3, 2026 15:53
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 3, 2026

Warning

Rate limit exceeded

@BillChirico has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 14f145c and 38e0829.

📒 Files selected for processing (2)
  • src/api/routes/backup.js
  • src/modules/backup.js
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/backup-async-clean

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Mar 3, 2026

Greptile Summary

Successfully converts synchronous file system operations to async promises throughout the backup module. The refactor eliminates blocking I/O operations and removes the TODO comment about performance improvements.

Key changes:

  • All fs operations now use node:fs/promises (access, mkdir, readdir, readFile, stat, unlink, writeFile)
  • getBackupDir() now uses mkdir with recursive option, eliminating TOCTOU race condition
  • listBackups() efficiently uses Promise.all to parse backup metadata in parallel
  • setInterval callback in startScheduledBackups() is async and properly awaits operations
  • All API route handlers correctly await async backup functions

The changes maintain identical functionality while improving performance and following async best practices. Express 5 automatically handles any async errors from route handlers.

Confidence Score: 5/5

  • Safe to merge - clean refactor with no behavioral changes
  • This is a well-executed async refactor that addresses previous review feedback. All sync operations properly converted to async/await, no logic changes, proper error handling maintained, and follows project conventions (ESM, Winston logging, node: prefix).
  • No files require special attention

Important Files Changed

Filename Overview
src/modules/backup.js Converted all sync fs operations to node:fs/promises with proper await usage throughout
src/api/routes/backup.js Added async/await to all route handlers that call async backup functions

Last reviewed commit: 5db6669

Copy link

@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.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the backup subsystem to use async node:fs/promises APIs and updates the backup HTTP routes to await the backup module operations, aiming to avoid blocking the event loop during filesystem work.

Changes:

  • Reworks src/modules/backup.js to use async filesystem operations for backup create/read/list/prune and config import/restore.
  • Adds/updates src/api/routes/backup.js to use async/await for backup endpoints.
  • Introduces scheduled-backup helpers (startScheduledBackups / stopScheduledBackups) in the backup module.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File Description
src/modules/backup.js Async backup module implementation (create/list/read/restore/prune + scheduler).
src/api/routes/backup.js Backup API endpoints wired to the async module functions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Greptile review found that setInterval callback was not awaiting
the async createBackup() and pruneBackups() calls, which would
cause unhandled promise rejections instead of being caught by
the try/catch block.
coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 3, 2026
@BillChirico BillChirico merged commit 7d78202 into main Mar 3, 2026
7 of 9 checks passed
@BillChirico BillChirico deleted the fix/backup-async-clean branch March 3, 2026 16:14
@BillChirico BillChirico mentioned this pull request Mar 3, 2026
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.

2 participants