fix: stop ops-post-update-migrate self-rsync load storm on SessionStart#659
Conversation
refresh_current_directory() runs unconditionally on every SessionStart (before the sentinel check). When the plugin already runs FROM current/, PLUGIN_ROOT resolves to current/ and current_dir = dirname(PLUGIN_ROOT)/current == current/ — so `rsync -a --delete SRC/ SRC/` copies the dir onto itself. With ~10 background Claude sessions each firing SessionStart (plus ops-daemon-manager ensure-current), these self-rsyncs stack and never converge. The 60s timeout only wraps run_migrations, not refresh_current_directory, so each ran unbounded (observed 14-17 min alive), hammering the disk and pinning fseventsd — driving host load to 130+. Fix: - Skip the rsync/cp entirely when realpath(PLUGIN_ROOT) == realpath(current_dir) (the common case — no work to do, you ARE current/). - Single-flight mkdir lock so concurrent sessions can't stack refreshes, with a 2-min stale-lock sweep for killed runs. - Same guard applied to the duplicate block in run_migrations (#4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| [[ -d "$lock" ]] && [[ -n "$(find "$lock" -maxdepth 0 -mmin +2 2>/dev/null)" ]] \ | ||
| && rmdir "$lock" 2>/dev/null || true # clear stale lock from a killed run | ||
| if mkdir "$lock" 2>/dev/null; then | ||
| trap 'rmdir "'"$lock"'" 2>/dev/null || true' RETURN |
There was a problem hiding this comment.
Bug: The trap ... RETURN on line 51 will not clean up the lock directory if the script is terminated by a signal, causing subsequent operations to be skipped.
Severity: HIGH
Suggested Fix
Replace the trap ... RETURN inside the function with a script-wide trap ... EXIT handler. An EXIT trap executes on any script exit, including normal termination and termination via most signals (like SIGTERM), ensuring the lock directory is reliably removed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: claude-ops/bin/ops-post-update-migrate#L51
Potential issue: The `refresh_current_directory` function in `ops-post-update-migrate`
uses `trap ... RETURN` to clean up a lock directory. This script is executed as an
asynchronous `SessionStart` hook, which can be terminated with `SIGTERM` by the parent
process if it runs too long. A `RETURN` trap only fires on a normal function return, not
when the script is terminated by a signal. Consequently, if the script is killed while
the lock is held, the lock directory will persist. Subsequent script executions within
the next 2 minutes will detect the stale lock and skip the directory refresh, leading to
the `current/` directory becoming out of sync with the active plugin version.
Did we get this right? 👍 / 👎 to inform future reviews.
📝 WalkthroughWalkthroughThis PR adds a real-path comparison guard to skip redundant rsync/cp refreshes when PLUGIN_ROOT already points to the current/ directory, and introduces a single-flight filesystem lock around the refresh logic in refresh_current_directory to prevent concurrent SessionStart hooks from stacking operations. ChangesSelf-refresh Guard and Locking
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Aurora Capital (auroracapital)
deleted the
fix/ops-post-update-migrate-self-rsync-load-storm
branch
![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 using default effort and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit f8b8623. Configure here.
| [[ -d "$lock" ]] && [[ -n "$(find "$lock" -maxdepth 0 -mmin +2 2>/dev/null)" ]] \ | ||
| && rmdir "$lock" 2>/dev/null || true # clear stale lock from a killed run | ||
| if mkdir "$lock" 2>/dev/null; then | ||
| trap 'rmdir "'"$lock"'" 2>/dev/null || true' RETURN |
There was a problem hiding this comment.
RETURN trap clears peer lock
Medium Severity
The two-minute stale sweep can remove .refresh-current.lock while another session is still refreshing. A later session may mkdir the same lock path and run rsync. When the original session’s refresh_current_directory returns, its RETURN trap always rmdirs that path, dropping the active holder’s lock and weakening single-flight protection under concurrent SessionStart hooks.
Reviewed by Cursor Bugbot for commit f8b8623. Configure here.
There was a problem hiding this comment.
Not approving: Cursor Bugbot reported 1 unresolved medium-severity finding (RETURN trap may clear a peer refresh lock), and the Bugbot check finished as skipped. Human review is needed before merge; no reviewers were assigned because the only repo collaborator is the PR author.
Sent by Cursor Approval Agent: Pull Request Router and Approver
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
claude-ops/bin/ops-post-update-migrate (1)
149-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winclaude-ops/bin/ops-post-update-migrate:149-167 — Reuse the locked refresh helper here
This duplicated
current/refresh still runs without the single-flight lock, so version-bump sessions can stack refresh I/O and keep the load-storm fix half-applied. Callrefresh_current_directory()here instead of maintaining a second copy.🤖 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 `@claude-ops/bin/ops-post-update-migrate` around lines 149 - 167, The duplicated current/ refresh logic in ops-post-update-migrate is bypassing the single-flight lock, which can still allow concurrent refresh I/O. Replace the inline PLUGIN_ROOT-to-current copy/sync block with a call to refresh_current_directory() so this path uses the same locked helper as the version-bump flow and avoids maintaining a second refresh implementation.
🤖 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.
Outside diff comments:
In `@claude-ops/bin/ops-post-update-migrate`:
- Around line 149-167: The duplicated current/ refresh logic in
ops-post-update-migrate is bypassing the single-flight lock, which can still
allow concurrent refresh I/O. Replace the inline PLUGIN_ROOT-to-current
copy/sync block with a call to refresh_current_directory() so this path uses the
same locked helper as the version-bump flow and avoids maintaining a second
refresh implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 493abd62-3184-471a-be09-a331f5e45c63
📒 Files selected for processing (1)
claude-ops/bin/ops-post-update-migrate
1 participant
Problem
Host load spiked to 130+ on a 16-core Mac. Root cause traced to a self-referential rsync storm from this plugin's
bin/ops-post-update-migrate.refresh_current_directory()runs unconditionally on every SessionStart (line 68, before the sentinel check). It computescurrent_dir = dirname(PLUGIN_ROOT)/current. But the SessionStart hook invokes the script from.../ops/current/, soPLUGIN_ROOTiscurrent/→ the command becomes:Copying a directory onto itself with
--delete. With ~10 background Claude sessions each firing SessionStart (plusops-daemon-manager.sh ensure-currentalso spawning it), the self-rsyncs stack and never converge. The 60s watchdog only wrapsrun_migrations, notrefresh_current_directory, so each ran unbounded — observed 14–17 minutes alive — hammering the disk and pinningfseventsdat ~55%, cascading intosecd/trustd/cloudd.Observed live: 8+ concurrent
rsync -a --delete .../current/ .../current/pairs.Fix
realpath(PLUGIN_ROOT) == realpath(current_dir)— the common case, where you already arecurrent/and there is nothing to copy.mkdir-based lock so concurrent sessions can't stack refreshes, with a 2-minute stale-lock sweep for killed runs.run_migrations(feat(slack): scan Slack Desktop, Firefox, Safari for cookies #4).Verification
bash -nclean.CLAUDE_PLUGIN_ROOT=.../ops/current) now logsskip: PLUGIN_ROOT already == current/ — no self-rsyncand spawns zero rsyncs.🤖 Generated with Claude Code
Note
Low Risk
Targeted guard and locking around an existing refresh path; behavior when plugin root differs from
current/is unchanged aside from serialized refreshes.Overview
Fixes a host load spike caused by
refresh_current_directory()running on every SessionStart while the hook’sPLUGIN_ROOTis already.../ops/current/, sorsync -a --deletecopied that directory onto itself and stacked across many sessions.refresh_current_directory()now compares canonical paths (pwd -P) and skips rsync/cp when source and destination are the same. When a real refresh is needed, amkdir-based single-flight lock (with a 2-minute stale lock cleanup) ensures only one rsync runs at a time; other processes log and skip.The duplicate migration step #4
current/sync gets the same-path skip only (no lock in that block), so the common “already oncurrent/” case no longer spawns rsync there either.Reviewed by Cursor Bugbot for commit f8b8623. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit