feat(cliproxy): back up manager analytics to S3 - #2233
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesCPA Manager Plus backup flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BackupService
participant backup.sh
participant cliproxy_backup_manager_data
participant sqlite3
participant S3
BackupService->>backup.sh: Run hourly
backup.sh->>cliproxy_backup_manager_data: Back up CPA Manager Plus data
cliproxy_backup_manager_data->>sqlite3: Create and validate SQLite snapshot
cliproxy_backup_manager_data->>S3: Upload archive with data.key
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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 `@home-manager/services/cliproxyapi/scripts/backup.sh`:
- Line 24: Update the cp invocation in the backup script to include the force
flag, changing the existing options to use non-interactive cp -f behavior while
preserving the current source glob, destination, error suppression, and
fallback.
In `@spec/cliproxyapi_backup_spec.sh`:
- Line 164: Update the backup invocation in the test command to capture the exit
status of bash running $__BACKUP_SCRIPT before executing cat on $MOCK_LOG. Read
the mock log afterward, then exit using the captured backup status so
log-reading success cannot mask backup failures.
🪄 Autofix (Beta)
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: 0a4b00f9-b73f-4319-8a9f-47df6817c29c
📒 Files selected for processing (5)
home-manager/services/cliproxyapi/README.mdhome-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/common.shspec/cliproxyapi_backup_spec.sh
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/services/cpa-manager-plus/start.sh">
<violation number="1" location="home-manager/services/cpa-manager-plus/start.sh:55">
P3: The fallback wrapper can make this capture the Docker group's GID instead of the service user's primary GID, causing the data tree and container process to run as `uid:docker_gid`. Using the real group ID keeps ownership and container identity consistent across both direct and `sg` launches.</violation>
</file>
<file name="home-manager/services/cliproxyapi/scripts/common.sh">
<violation number="1" location="home-manager/services/cliproxyapi/scripts/common.sh:141">
P2: The new hourly-rollback upload runs before the primary "latest" upload, and neither `aws s3 cp` is protected by `|| true`. Under `set -euo pipefail`, a transient S3 failure on the auxiliary hourly slot upload aborts the function before the main `analytics-backup.tar.gz` (latest) upload is attempted, so the primary backup is lost for that run even though the snapshot/archive are valid and the latest object is the more important one. Recommend uploading the latest archive first (primary before convenience slot), or at minimum tolerating a failure of the hourly upload so it can't prevent the latest upload.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | ||
| --only-show-errors \ | ||
| "$archive_path" \ | ||
| "$(cliproxy_manager_backup_s3_uri "$hourly_object")" |
Contributor
There was a problem hiding this comment.
P2: The new hourly-rollback upload runs before the primary "latest" upload, and neither aws s3 cp is protected by || true. Under set -euo pipefail, a transient S3 failure on the auxiliary hourly slot upload aborts the function before the main analytics-backup.tar.gz (latest) upload is attempted, so the primary backup is lost for that run even though the snapshot/archive are valid and the latest object is the more important one. Recommend uploading the latest archive first (primary before convenience slot), or at minimum tolerating a failure of the hourly upload so it can't prevent the latest upload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/cliproxyapi/scripts/common.sh, line 141:
<comment>The new hourly-rollback upload runs before the primary "latest" upload, and neither `aws s3 cp` is protected by `|| true`. Under `set -euo pipefail`, a transient S3 failure on the auxiliary hourly slot upload aborts the function before the main `analytics-backup.tar.gz` (latest) upload is attempted, so the primary backup is lost for that run even though the snapshot/archive are valid and the latest object is the more important one. Recommend uploading the latest archive first (primary before convenience slot), or at minimum tolerating a failure of the hourly upload so it can't prevent the latest upload.</comment>
<file context>
@@ -116,18 +117,29 @@ cliproxy_backup_manager_data() (
+ --endpoint-url="$OBJECTSTORE_ENDPOINT" \
+ --only-show-errors \
+ "$archive_path" \
+ "$(cliproxy_manager_backup_s3_uri "$hourly_object")"
+
AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
</file context>
| fi | ||
|
|
||
| host_uid="$(id -u)" | ||
| host_gid="$(id -g)" |
Contributor
There was a problem hiding this comment.
P3: The fallback wrapper can make this capture the Docker group's GID instead of the service user's primary GID, causing the data tree and container process to run as uid:docker_gid. Using the real group ID keeps ownership and container identity consistent across both direct and sg launches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/cpa-manager-plus/start.sh, line 55:
<comment>The fallback wrapper can make this capture the Docker group's GID instead of the service user's primary GID, causing the data tree and container process to run as `uid:docker_gid`. Using the real group ID keeps ownership and container identity consistent across both direct and `sg` launches.</comment>
<file context>
@@ -51,10 +51,23 @@ if ! ensure_container_removed; then
fi
+host_uid="$(id -u)"
+host_gid="$(id -g)"
+
+# Older launches ran as root inside the container. Migrate the bind-mounted data
</file context>
Suggested change
| host_gid="$(id -g)" | |
| host_gid="$(id -rg)" |
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
1 participant
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.
Summary
cliproxyapi-backup.serviceto snapshot CPA Manager Plus analytics hourlyusage.sqlitewith its matchingdata.keyVerification
shellspec spec/cliproxyapi_backup_spec.sh(8 examples, 0 failures)shellcheck home-manager/services/cliproxyapi/scripts/*.sh spec/cliproxyapi_backup_spec.shmake shell-inline-checkmake nix-format-checkhomeConfigurations.kyberbackup timer and service configurationdata.keyBaseline limitation
The full
make nix-flake-checkreaches the Kyber Home Manager evaluation successfully, then fails on the existing host-platform mismatch when the aarch64-darwin machine is asked to build x86_64-linux Docker derivations.Summary by cubic
Adds wall-clock hourly backups of CPA Manager Plus analytics via
cliproxyapi-backup, creating a consistent SQLite snapshot and uploading bothlatestand per-hour archives to S3. Also improves resilience and runs the CPA Manager Plus container as the service user with automatic data ownership migration.New Features
StartCalendarInterval+ usersystemdtimer withOnCalendar=hourly,Persistent=true); analytics backup runs independently of auth files..backup+PRAGMA integrity_check), archivesusage.sqlite+data.key, and uploads tos3://cliproxyapi/cpa-manager-plus/analytics-backup.tar.gzandanalytics-backup-HH.tar.gz(24 hourly slots).sqlite,gnutar, andgzip; setsUMask=0077; ShellSpec covers snapshot and hourly uploads.Bug Fixes
Written for commit 0211b44. Summary will update on new commits.