Fix daily backup via BrainBar VACUUM INTO#294
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@coderabbitai review |
|
@codex review |
|
@greptileai review |
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc51176ae8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| <key>ExitTimeOut</key> |
There was a problem hiding this comment.
For runs that hang while the process is active, such as during Drive upload after the snapshot is made, this key does not impose a 5-minute runtime limit. I checked the launchd.plist(5) semantics: ExitTimeOut is the grace period between launchd sending SIGTERM and then SIGKILL when it is already stopping a job, not a maximum wall-clock duration. Since this agent only uses StartCalendarInterval and the wrapper has no whole-job watchdog, a stuck backup can still remain alive indefinitely; the timeout needs to be enforced in backup-daily.sh or backup_daily.py if the daily job must be bounded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The launchd wrapper now exports BRAINLAYER_BACKUP_TIMEOUT_SECONDS=300 by default, and backup_daily.main() enforces that wall-clock timeout with SIGALRM and exits 124 on timeout. Added a regression test for the timeout path.
EtanHey
force-pushed
the
fix/backup-daily-vacuum-into
branch
from
fc51176 to
7034ddc
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7034ddce0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raw_snapshot = Path(tmp) / f"{date_stamp}.db" | ||
| source = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True, timeout=60) | ||
| target = sqlite3.connect(raw_snapshot) | ||
| request_brainbar_vacuum_into(raw_snapshot, socket_path=socket_path) |
There was a problem hiding this comment.
When db_path is non-canonical or comes from the documented BRAINLAYER_DB override, this now only checks/sizes/reports that path but asks BrainBar to VACUUM INTO whatever database its own process has open (BRAINBAR_DB_PATH or the canonical default). In that configuration the scheduled/manual backup can upload a valid snapshot of the wrong database while reporting the requested db_path, which makes restores lose the intended data; the request needs to verify the BrainBar DB matches db_path or pass the expected source through the tool contract.
Useful? React with 👍 / 👎.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| finally: | ||
| if timeout_seconds is not None: | ||
| signal.setitimer(signal.ITIMER_REAL, 0) | ||
| signal.signal(signal.SIGALRM, previous_alarm_handler) |
There was a problem hiding this comment.
🟢 Low brainlayer/backup_daily.py:432
When signal.getsignal(signal.SIGALRM) returns None (e.g., if the handler was set by C code or a library), passing that None to signal.signal(signal.SIGALRM, previous_alarm_handler) in the finally block raises TypeError: signal handler must be signal.SIG_IGN, signal.SIG_DFL, or a callable. This crashes the cleanup after a successful backup or timeout.
- finally:
- if timeout_seconds is not None:
- signal.setitimer(signal.ITIMER_REAL, 0)
- signal.signal(signal.SIGALRM, previous_alarm_handler)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/brainlayer/backup_daily.py around lines 432-435:
When `signal.getsignal(signal.SIGALRM)` returns `None` (e.g., if the handler was set by C code or a library), passing that `None` to `signal.signal(signal.SIGALRM, previous_alarm_handler)` in the `finally` block raises `TypeError: signal handler must be signal.SIG_IGN, signal.SIG_DFL, or a callable`. This crashes the cleanup after a successful backup or timeout.
Evidence trail:
src/brainlayer/backup_daily.py lines 410-437 (REVIEWED_COMMIT): line 414 calls `signal.getsignal(signal.SIGALRM)` which can return `None` per Python docs; line 435 in the `finally` block passes that value to `signal.signal(signal.SIGALRM, previous_alarm_handler)` which does not accept `None`. Python docs: https://docs.python.org/3/library/signal.html confirms `getsignal()` may return `None` and `signal.signal()` requires SIG_IGN, SIG_DFL, or a callable.
| let targetURL = URL(fileURLWithPath: trimmedTarget).standardizedFileURL | ||
| let sourceURL = URL(fileURLWithPath: path).standardizedFileURL |
There was a problem hiding this comment.
🟡 Medium BrainBar/BrainDatabase.swift:479
standardizedFileURL does not resolve symlinks, so the safety check at line 481 can be bypassed when paths differ by symlink. For example, on macOS /tmp is a symlink to /private/tmp, so /tmp/db.sqlite and /private/tmp/db.sqlite compare as different but refer to the same file, allowing VACUUM INTO to overwrite the live database. Consider using resolvingSymlinksInPath() before comparing.
- let targetURL = URL(fileURLWithPath: trimmedTarget).standardizedFileURL
- let sourceURL = URL(fileURLWithPath: path).standardizedFileURL
+ let targetURL = URL(fileURLWithPath: trimmedTarget).standardizedFileURL.resolvingSymlinksInPath()
+ let sourceURL = URL(fileURLWithPath: path).standardizedFileURL.resolvingSymlinksInPath()🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file brain-bar/Sources/BrainBar/BrainDatabase.swift around lines 479-480:
`standardizedFileURL` does not resolve symlinks, so the safety check at line 481 can be bypassed when paths differ by symlink. For example, on macOS `/tmp` is a symlink to `/private/tmp`, so `/tmp/db.sqlite` and `/private/tmp/db.sqlite` compare as different but refer to the same file, allowing `VACUUM INTO` to overwrite the live database. Consider using `resolvingSymlinksInPath()` before comparing.
Evidence trail:
brain-bar/Sources/BrainBar/BrainDatabase.swift lines 472-498 (viewed at REVIEWED_COMMIT): `vacuumInto` uses `standardizedFileURL` at lines 479-480, compares paths at line 481. Apple documentation at https://developer.apple.com/documentation/foundation/url/standardizedfileurl confirms standardizedFileURL does not resolve symlinks. Apple documentation at https://developer.apple.com/documentation/foundation/nsurl/resolvingsymlinksinpath confirms resolvingSymlinksInPath does resolve symlinks. macOS `/tmp` → `/private/tmp` is a well-known symlink example.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7034ddc. Configure here.
| [ | ||
| "name": "brain_backup_vacuum_into", | ||
| "description": "Create a SQLite backup snapshot using VACUUM INTO on BrainBar's single-writer connection.", | ||
| "annotations": MCPRouter.writeIdempotentAnnotations, |
There was a problem hiding this comment.
Non-idempotent backup tool annotated as idempotent
Low Severity
The brain_backup_vacuum_into tool uses writeIdempotentAnnotations which sets idempotentHint: true, but vacuumInto explicitly rejects repeat calls with the same target_path because it guards against the target file already existing. MCP clients seeing idempotentHint: true may automatically retry failed or timed-out calls, which will always fail on the second attempt with "backup target already exists."
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7034ddc. Configure here.
1 participant
Summary
backup_dailysnapshot creation through BrainBar's existing Unix socket with a newbrain_backup_vacuum_intoMCP tool.VACUUM INTOon BrainBar's database connection, preserving the single-writer queue instead of opening a competingsqlite3.backup()reader.ExitTimeOut=300to the daily backup LaunchAgent while retaining the calendar schedule and noKeepAlive.Fixes the 6h57m runaway via single-writer VACUUM INTO over the existing UDS. launchd swap eliminates self-perpetuation.
Tests
PYTHONPATH=src pytest tests/test_backup_daily.py -qswift test --package-path brain-bar --filter SocketIntegrationTests/testBrainBackupVacuumIntoOverSocketCreatesRestorableSnapshotswift test --package-path brain-barruff check src/ tests/ && ruff format --check src/ tests/plutil -lint scripts/launchd/com.brainlayer.backup-daily.plistlaunchctl print gui/$(id -u)/com.brainlayer.backup-dailyshowed calendar interval trigger andkeepalive = 0for the installed jobNotes
PYTHONPATH=src pytest tests -q --ignore=tests/regression/test_drift_detection.py --ignore=tests/test_eval_framework.pyis blocked locally by existing live production DB locks plus unrelated eval/hook timing assertions: 2034 passed, 3 failed, 62 errors.Note
Medium Risk
Changes the production backup path to rely on a new BrainBar MCP tool and SQLite
VACUUM INTO, plus adds a hard wall-clock timeout; failures could break scheduled backups if the socket/tool is unavailable or path validation rejects the target.Overview
Routes daily BrainLayer backup creation through BrainBar’s Unix-socket MCP interface by adding a new
brain_backup_vacuum_intotool that runs SQLiteVACUUM INTOon BrainBar’s single-writer connection and returns snapshot metadata.Updates
backup_daily.pyto request the snapshot over the socket (instead of usingsqlite3’s backup API) and adds a configurable wall-clock timeout (BRAINLAYER_BACKUP_TIMEOUT_SECONDS) with alaunchdExitTimeOut=300default. Tests are expanded to cover the new tool listing, end-to-end socket backup + restore validation, and the Python timeout/socket behavior.Reviewed by Cursor Bugbot for commit 7034ddc. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix daily backup by delegating SQLite snapshot creation to BrainBar via VACUUM INTO
backup_daily.pywith a Unix socket call to BrainBar's newbrain_backup_vacuum_intoMCP tool, ensuring the snapshot is created through BrainBar's single-writer connection.BrainDatabase.vacuumInto(targetPath:)inBrainDatabase.swiftand the correspondingbrain_backup_vacuum_intotool routing inMCPRouter.swift.BRAINLAYER_BACKUP_TIMEOUT_SECONDS, default 300s) enforced viaSIGALRMinmain(); exits with code 124 on timeout.ExitTimeOut 300in the launchd plist and exports the timeout default in the shell wrapper.📊 Macroscope summarized 7034ddc. 5 files reviewed, 2 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues