fix(deploy): harden legacy cutover rollback - #409
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds legacy cutover and rollback scripts, updates deploy behavior for legacy versus shared-release instances, changes rollback backup output handling, and revises related docs and tests. ChangesLegacy cutover and rollback flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
corrin
marked this pull request as ready for review
June 26, 2026 07:58
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@scripts/legacy_rollback.sh`:
- Around line 144-166: The sync_sequences step is currently picking the first
complete shared release venv, which may not match the restored snapshot
environment. Update the legacy_rollback.sh flow around the SHARED_VENV lookup
and the manage.py sync_sequences invocation so it uses the restored legacy
environment associated with the rollback target, or explicitly validates that
the selected venv matches the restored runtime before proceeding. Keep the
change localized to the shared venv selection logic and the sync_sequences
execution block.
- Line 141: The ownership change in the rollback script is too broad and
recurses into the backups area, which should remain root-owned. Update the
ownership logic around the chown in legacy_rollback.sh so it only applies to the
restored checkout/runtime paths used by the rollback flow, and explicitly
exclude the backups directory (including snapshots, saved unit files, nginx
backups, and dumps) from any recursive ownership changes. Keep the
backup-related paths owned by root while still assigning the restored app files
to the instance user.
In `@scripts/server/cutover_legacy_instance.sh`:
- Around line 73-109: The snapshot creation flow in cutover_legacy_instance.sh
currently writes the tarball in the create snapshot step before the required
systemd unit files and nginx config are validated, which can leave behind an
incomplete legacy_<sha> snapshot. Move the asset validation ahead of the tarball
write, or stage the archive and backup outputs in temporary locations and only
finalize them after all checks pass; use the existing log, unit_path loop, and
NGINX_CONF checks to gate the snapshot creation.
- Line 86: The rollback snapshot ownership is being changed to the instance
user, which allows the payload consumed by the root-executed rollback path to be
tampered with. In the cutover flow in cutover_legacy_instance.sh, keep the
$SNAPSHOT archive root-owned instead of chowning it to $INST_USER, or relocate
the snapshot to a root-owned location outside the instance-owned tree so
legacy_rollback.sh can safely extract the trusted archive.
In `@scripts/server/deploy.sh`:
- Around line 331-334: The deploy flow is using a different legacy-checkout test
in the previous-sha handling than the shared predicate used earlier, so it can
misclassify instances that still have .git alongside current after cutover.
Update the condition in the previous shared-release path to reuse the same
legacy-layout predicate already used around the earlier branch, and apply the
same fix anywhere else the legacy rollback/build decision is made so both paths
make the same .git and current check.
In `@scripts/server/README.md`:
- Line 118: The deployment step description in the README only mentions the
explicit rollback path for normal deploys, but it also needs to document the
legacy checkout rollback flow. Update the text around the deploy/rollback
instructions to mention both `predeploy_rollback.sh` for standard cases and
`legacy_rollback.sh` for legacy checkouts, keeping the wording aligned with the
operator flow described by the deploy script.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c981e2fa-a5b3-43c9-8b6c-adac20e585ae
📒 Files selected for processing (7)
apps/workflow/tests/test_xero_instance_templates.pydocs/updating.mdscripts/cleanup_backups.pyscripts/legacy_rollback.shscripts/server/README.mdscripts/server/cutover_legacy_instance.shscripts/server/deploy.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/server/cutover_legacy_instance.sh`:
- Around line 65-66: The rollback dump placement in cutover_legacy_instance.sh
is unsafe because the predeploy_*.sql.gz file is being kept under the
app-writable backups directory and later consumed by legacy_rollback.sh as
postgres. Update the cutover flow to store the rollback-consumed dump in the
root-owned rollback directory (or make the dump file/directory root-owned and
immutable to the instance user), and ensure the restore path in
legacy_rollback.sh only reads from that protected location after validating the
expected file ownership and contents. Use the existing
cutover_legacy_instance.sh backup setup and legacy_rollback.sh restore logic to
keep the predeploy dump inaccessible to $INST_USER.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f395058f-30e7-413c-b172-8655ce84d483
📒 Files selected for processing (5)
apps/workflow/tests/test_xero_instance_templates.pyscripts/legacy_rollback.shscripts/server/README.mdscripts/server/cutover_legacy_instance.shscripts/server/deploy.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/server/deploy.sh
Comment on lines
+65
to
+66
| chown "$INST_USER:$INST_USER" "$BACKUP_DIR" | ||
| chmod 700 "$BACKUP_DIR" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep rollback-consumed DB dumps out of the app-writable backup directory.
legacy_rollback.sh later restores predeploy_*_${OLD_SHORT}.sql.gz from $INSTANCE_DIR/backups, but this script makes that directory writable by $INST_USER. A compromised instance user can replace the gzip with arbitrary psql input that rollback runs as postgres. Store the paired predeploy dump in the root-owned $ROLLBACK_DIR, or make the dump directory/file root-owned and validate both immediately before restore.
🤖 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 `@scripts/server/cutover_legacy_instance.sh` around lines 65 - 66, The rollback
dump placement in cutover_legacy_instance.sh is unsafe because the
predeploy_*.sql.gz file is being kept under the app-writable backups directory
and later consumed by legacy_rollback.sh as postgres. Update the cutover flow to
store the rollback-consumed dump in the root-owned rollback directory (or make
the dump file/directory root-owned and immutable to the instance user), and
ensure the restore path in legacy_rollback.sh only reads from that protected
location after validating the expected file ownership and contents. Use the
existing cutover_legacy_instance.sh backup setup and legacy_rollback.sh restore
logic to keep the predeploy dump inaccessible to $INST_USER.
This was referenced Jun 27, 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
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
Hardens the remaining legacy-to-shared-release cutover path for KAN-197.
legacy_rollback.shscripts/cleanup_backups.pyWhy
The shared immutable release model superseded the original per-instance
frontend/node_modulessymlink approach. The remaining risk is the one-timemsm-prodcutover from a legacy checkout, where the old SHA cannot be rebuilt as a shared release and rollback must use a pre-cutover snapshot.Validation
shellcheck scripts/server/deploy.sh scripts/server/cutover_legacy_instance.sh scripts/legacy_rollback.sh scripts/backup_db.sh scripts/predeploy_backup.sh scripts/predeploy_rollback.shbash -n scripts/server/deploy.sh scripts/server/cutover_legacy_instance.sh scripts/legacy_rollback.shgit diff --checkpre-commit run --files scripts/server/deploy.sh scripts/server/cutover_legacy_instance.sh scripts/legacy_rollback.sh scripts/cleanup_backups.py docs/updating.md scripts/server/README.mdSummary by CodeRabbit