hotfix(installer): normalise tree ownership before re-run update (urgent, unblocks fresh installs) - #1840
Conversation
A re-run of install-server.sh over an existing checkout drops to the repo-owning user (the 'taos' service user) for the git fetch + reset, to avoid running git as root inside a user-writable tree. But it only reads the TOP-LEVEL dir owner. If a prior install was interrupted mid-chown (or a root step wrote a few paths back), the tree has MIXED ownership: the owning user then cannot unlink the still-root-owned paths, so the reset fails with 'unable to unlink old ...: Permission denied' -> 'Could not reset index file to revision origin/master', bricking every subsequent re-run. Normalise ownership to the owning user (chown -R, run by root) right before the update so the reset can rewrite the whole tree. Safe: root does the chown and git still runs unprivileged. Reported on #2 (fresh Orange Pi 5 Plus, retry after a partial first run). Failure class reproduced locally: an unwritable path in the tree yields the identical unlink-EACCES; normalising the tree makes the reset apply cleanly.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
👋 Thanks for the PR! This one targets See CONTRIBUTING.md for the branch model. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe installer now recursively assigns existing checkout contents to the detected non-root repository owner before running ChangesInstaller checkout update
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # "Could not reset index file"), bricking every re-run. Normalise | ||
| # ownership to the owning user first so the reset can rewrite the whole | ||
| # tree. Safe: root performs the chown and git still runs unprivileged. | ||
| chown -R "$_repo_owner" "$INSTALL_DIR" |
There was a problem hiding this comment.
WARNING: Unguarded chown -R aborts the whole install under set -euo pipefail
This script sets set -euo pipefail (line 35), and this chown line has no || fallback. If chown fails for any reason (e.g. _repo_owner resolves to an unusable name, a path chown can't traverse, disk/permission hiccup), the non-zero exit aborts the entire installer — a worse, harder-to-recover failure than the reset failure this PR is meant to fix. The subsequent sudo -u "$_repo_owner" git ... chain is also unguarded, but the reset there is a deliberate intentional failure path; a mid-flight chown abort is not.
Consider guarding it so a chown failure degrades to the previous behaviour instead of bricking the run, e.g.:
| chown -R "$_repo_owner" "$INSTALL_DIR" | |
| chown -R "$_repo_owner" "$INSTALL_DIR" || warn "could not normalise tree ownership -- update may fail if ownership is mixed" |
(or || true if you'd rather always attempt the update). This mirrors the || warn ...; continue pattern already used elsewhere in the file for best-effort steps.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 34.3K · Output: 2.3K · Cached: 101.5K |
1 participant
Urgent install-blocker. The installer is fetched from
master, so this fix must land on master to help anyone installing now.Cherry-pick of the dev fix (#1839) so only this one-file change reaches master, without pulling unreleased dev work into a release.
Problem
Reported on #2 (fresh Orange Pi 5 Plus). A re-run of
install-server.shover an existing checkout dies withunable to unlink old ...: Permission denied->Could not reset index file to revision origin/master, bricking every subsequent attempt.Root cause
The update path drops to the repo-owning user (
taos) for thegit reset --hard(a real privilege-escalation guard), but reads only the top-level dir owner. A prior interrupted install leaves the tree with mixed ownership, so the owning user cannot unlink the still-root-owned paths and the reset aborts.Fix
chown -Rthe tree to the owning user (as root) immediately before the update, so the reset can rewrite the whole tree. git still runs unprivileged.Validation
bash -nclean. Failure class reproduced locally (unwritable tree path -> identical unlink-EACCES; normalising the tree makes the reset apply cleanly). Identical change already on the dev PR #1839.Summary by CodeRabbit