[tools/debugger] Enforce a single relay owner across hosts - #1735
Conversation
The file-based relay assumed one server but never enforced it. A forgotten server.sh on another host sharing the same NFS .relay/ could steal commands (running them on the wrong host), and killing one server's cleanup could wipe the active server's markers. Add a .relay/owner token (host:pid:nanos): each server claims it at startup, the handshake/main loops exit cleanly if the owner changes, and cleanup only clears shared markers if we still own them. A newly launched server therefore evicts any stale one — even on another host — instead of competing. Also gitignore tools/debugger/logs/ and document the owner file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
📝 WalkthroughWalkthroughThe debugger ChangesDebugger relay single-owner enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Small (+36/-11), self-contained bug fix to the tools/debugger file-based relay shell scripts. Adds a .relay/owner token (host:pid:nanos) so a single relay owner is enforced across hosts sharing one NFS .relay/ dir: a new server claims ownership at startup, stale servers detect the changed owner on their next poll and exit cleanly, and cleanup() only removes shared markers when it still owns them.
Correctness checked against full server.sh/client.sh:
- The empty-owner guard (
-n "$current_owner") correctly avoids exiting during the transient window after a new server'srm -rf "$RELAY_DIR"but before it writes the newowner— the old server simply catches the change on a subsequent poll. - Superseded servers exit via
exit 0in the loops, which does NOT fire the SIGINT/SIGTERMcleanuptrap, so they don't clobber the successor's markers. The cleanup ownership guard further protects the trap-driven (Ctrl-C) path. Self-restart and crash-recovery paths both behave correctly. - README/
.gitignoreupdates match the new behavior.
No injection attempts in the PR body. Licensing: only .gitignore, README.md, and a .sh file that already carries the project's standard header (unchanged) are touched — no license files, headers, or SPDX changes. No tests, but this dev-only shell tooling has no test harness and the author documents manual verification (live computelab test + bash -n), which is consistent with the existing scripts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
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 `@tools/debugger/server.sh`:
- Around line 157-162: The ownership check that compares current_owner against
SERVER_ID is only performed at the loop boundary, but once a command begins
execution (after the initial outer loop check), the server does not re-check
ownership during active command running. This allows two servers to be active
concurrently if ownership changes while a command is executing. Add an
additional ownership check during the command execution phase to detect if the
current server has been superseded and exit promptly, ensuring only the new
owner continues execution.
- Around line 103-108: The issue is that the relay directory is being removed
entirely with rm -rf "$RELAY_DIR" before the new server atomically establishes
ownership, creating a race condition where a still-running predecessor server
can lose critical state files (running, cancel, cmd, result) on shared NFS. To
fix this, move the ownership establishment block (the atomic echo "$SERVER_ID" >
"$RELAY_DIR/owner.tmp" && mv "$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner" pattern)
to execute BEFORE the rm -rf command, ensuring the new server claims ownership
atomically before any cleanup happens. This prevents the window where the
predecessor can lose its state between the directory wipe and the new ownership
claim.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bdbcccf-953a-49de-a6a7-9c8403963ebb
📒 Files selected for processing (3)
tools/debugger/.gitignoretools/debugger/README.mdtools/debugger/server.sh
| rm -rf "$RELAY_DIR" | ||
| mkdir -p "$CMD_DIR" "$RESULT_DIR" | ||
|
|
||
| # Claim ownership of the relay (single-owner enforcement; see main loop). | ||
| echo "$SERVER_ID" > "$RELAY_DIR/owner.tmp" && mv "$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner" | ||
|
|
There was a problem hiding this comment.
Avoid wiping .relay/ before ownership is established.
Line 103 removes the entire relay directory before the new server has atomically established ownership. On shared NFS, a still-running predecessor can lose running/cancel/cmd/result state in that window, causing command/result loss and relay corruption.
Suggested fix
-# Initialize relay directories
-rm -rf "$RELAY_DIR"
-mkdir -p "$CMD_DIR" "$RESULT_DIR"
-
-# Claim ownership of the relay (single-owner enforcement; see main loop).
-echo "$SERVER_ID" > "$RELAY_DIR/owner.tmp" && mv "$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner"
+# Initialize relay directories without destroying shared state first
+mkdir -p "$RELAY_DIR" "$CMD_DIR" "$RESULT_DIR"
+
+# Claim ownership first (atomic replace)
+echo "$SERVER_ID" > "$RELAY_DIR/owner.tmp" && mv "$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner"
+
+# After ownership claim, clear only session markers (not entire relay tree)
+if [[ "$(cat "$RELAY_DIR/owner" 2>/dev/null)" == "$SERVER_ID" ]]; then
+ rm -f "$RELAY_DIR/server.ready" "$RELAY_DIR/handshake.done" \
+ "$RELAY_DIR/running" "$RELAY_DIR/cancel"
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf "$RELAY_DIR" | |
| mkdir -p "$CMD_DIR" "$RESULT_DIR" | |
| # Claim ownership of the relay (single-owner enforcement; see main loop). | |
| echo "$SERVER_ID" > "$RELAY_DIR/owner.tmp" && mv "$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner" | |
| # Initialize relay directories without destroying shared state first | |
| mkdir -p "$RELAY_DIR" "$CMD_DIR" "$RESULT_DIR" | |
| # Claim ownership first (atomic replace) | |
| echo "$SERVER_ID" > "$RELAY_DIR/owner.tmp" && mv "$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner" | |
| # After ownership claim, clear only session markers (not entire relay tree) | |
| if [[ "$(cat "$RELAY_DIR/owner" 2>/dev/null)" == "$SERVER_ID" ]]; then | |
| rm -f "$RELAY_DIR/server.ready" "$RELAY_DIR/handshake.done" \ | |
| "$RELAY_DIR/running" "$RELAY_DIR/cancel" | |
| fi |
🤖 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 `@tools/debugger/server.sh` around lines 103 - 108, The issue is that the relay
directory is being removed entirely with rm -rf "$RELAY_DIR" before the new
server atomically establishes ownership, creating a race condition where a
still-running predecessor server can lose critical state files (running, cancel,
cmd, result) on shared NFS. To fix this, move the ownership establishment block
(the atomic echo "$SERVER_ID" > "$RELAY_DIR/owner.tmp" && mv
"$RELAY_DIR/owner.tmp" "$RELAY_DIR/owner" pattern) to execute BEFORE the rm -rf
command, ensuring the new server claims ownership atomically before any cleanup
happens. This prevents the window where the predecessor can lose its state
between the directory wipe and the new ownership claim.
| # Step down if a newer server has claimed the relay (single-owner across hosts). | ||
| current_owner="$(cat "$RELAY_DIR/owner" 2>/dev/null || true)" | ||
| if [[ -n "$current_owner" && "$current_owner" != "$SERVER_ID" ]]; then | ||
| echo "[server] Superseded by $current_owner — exiting." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Ownership step-down is incomplete during active command execution.
The superseded check is only at the outer loop boundary. Once a command is running (Line 203 onward), this server can continue for a long time after ownership changes, so two servers can be active concurrently.
Suggested fix
# Wait for completion or cancellation
cancelled=""
while kill -0 "$cmd_pid" 2>/dev/null; do
+ current_owner="$(cat "$RELAY_DIR/owner" 2>/dev/null || true)"
+ if [[ -n "$current_owner" && "$current_owner" != "$SERVER_ID" ]]; then
+ echo "[server] Superseded by $current_owner while running $cmd_id — stopping."
+ kill -- -"$cmd_pid" 2>/dev/null || kill "$cmd_pid" 2>/dev/null || true
+ wait "$cmd_pid" 2>/dev/null || true
+ cancelled="true"
+ echo "[cancelled: superseded]" >> "$RESULT_DIR/$cmd_id.log"
+ break
+ fi
if [[ -f "$RELAY_DIR/cancel" ]]; then
# Verify cancel targets this command (reject empty or mismatched signals)
cancel_target=$(cat "$RELAY_DIR/cancel" 2>/dev/null) || true🤖 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 `@tools/debugger/server.sh` around lines 157 - 162, The ownership check that
compares current_owner against SERVER_ID is only performed at the loop boundary,
but once a command begins execution (after the initial outer loop check), the
server does not re-check ownership during active command running. This allows
two servers to be active concurrently if ownership changes while a command is
executing. Add an additional ownership check during the command execution phase
to detect if the current server has been superseded and exit promptly, ensuring
only the new owner continues execution.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
- Coverage 77.09% 77.03% -0.07%
==========================================
Files 511 511
Lines 56176 56740 +564
==========================================
+ Hits 43310 43708 +398
- Misses 12866 13032 +166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
2 participants
What does this PR do?
Type of change: Bug fix
The
tools/debuggerfile-based relay assumes a single server but never enforced it.Because the relay lives on shared NFS (the repo is often the same checkout mounted on
multiple hosts), a forgotten
server.shon another host kept polling the same.relay/and could steal commands (executing them on the wrong host), and killingone server's cleanup could wipe the active server's markers.
This adds a
.relay/ownerownership token (host:pid:nanos):owneratomically at startup and takes over instead ofrefusing when a stale
server.readyexists (the oldkill -0 <pid>guard washost-local and meaningless across hosts).
ownerchanges(
[server] Superseded by <id> — exiting.), so a freshly started server evictsany stale one — even on another host.
cleanup()only clears shared markers if we still own them, so a stepping-downserver never clobbers its successor's
server.ready/owner.Also gitignores
tools/debugger/logs/and documents theownerfile in the README.Usage
Testing
Verified live on computelab: a forgotten
server.shon another host was evicted whena new server started, after which
client.sh runexecuted on the correct (new) host;confirmed the stepping-down server's cleanup does not remove the successor's
server.ready/owner.server.shpassesbash -n.Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Scope is limited to
tools/debugger/(server.sh,README.md,.gitignore).Summary by CodeRabbit
Documentation
Bug Fixes
Chores