Apply the Lock-Probe Fixes to cleanup() Too, Fix a Stale Comment by ptr727 · Pull Request #1106 · ptr727/ProjectTemplate

ptr727 · 2026-08-29T23:42:23Z

CodeRabbit's review of the develop -> main promotion PR (#1098) found that cleanup()'s own separate, pre-existing lock probe had the same stderr-suppression and missing-exec-check gaps this PR chain already fixed in hub_read_lock_acquire, plus a stale comment claiming hub_read_lock_acquire "runs regardless of --dry-run" when it actually no-ops entirely under --dry-run.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved dry-run diagnostics when fetching is unavailable.
- Enhanced cleanup lock handling with clearer error messages.
- Distinguished lock contention from other file-locking errors for more reliable troubleshooting.

CodeRabbit's review of the develop -> main promotion PR (#1098) found two more real issues, both matching gaps this same PR chain already fixed in hub_read_lock_acquire but left unaddressed in cleanup()'s own separate, pre-existing lock probe: - cleanup() suppressed flock's own stderr on its non-blocking probe and treated any failure as ordinary contention, the same masking bug fixed for hub_read_lock_acquire's own probe. Applied the identical fix: captured stderr distinguishes a real error (surfaced via fail, failing immediately) from ordinary contention (the existing wait message), confirmed live for both paths. - cleanup()'s own exec {lock_fd}>"$DIR/hub.lock" had no explicit success check, the same gap fixed for hub_read_lock_acquire's own open. Checked explicitly now, confirmed live. Also fixed: fetch_hub's own comment claimed hub_read_lock_acquire "runs regardless of --dry-run", but it actually no-ops entirely under --dry-run (never opening a lock at all). Corrected to describe why fetch_hub's own dry-run check exists: without it, the generic "no reader lock held" internal-error message would fire instead of a clear, task-specific one. `shellcheck` and `prose_lint.py` are clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

qodo-code-review · 2026-08-29T23:42:58Z

PR Summary by Qodo

Harden cleanup lock probing and correct dry-run documentation

🐞 Bug fix📝 Documentation🕐 10-20 Minutes

AI Description

• Validate cleanup’s hub lock file opens before probing or deleting the checkout.
• Distinguish flock execution errors from contention, preserving waits only for busy locks.
• Correct fetch_hub dry-run comments to match reader-lock behavior.

Diagram

graph TD
A["cleanup"] --> B["Open lock file"] --> C{"Open succeeds?"}
C -- No --> H["Report failure"]
C -- Yes --> D["Probe exclusive lock"] --> E{"Probe result"}
E -- Error --> H
E -- Contended --> F["Wait for lock"] --> G["Delete hub tree"]
E -- Acquired --> G

High-Level Assessment

The current approach is appropriate because it applies the already-established lock-probe safeguards directly to cleanup while preserving cleanup’s exclusive lock and local file-descriptor lifecycle. Extracting a shared helper was considered, but the reader and cleanup paths use different lock modes, descriptor ownership, and failure cleanup, so abstraction would add complexity without improving this focused fix.

Files changed (1) +15 / -3

Bug fix (1) +15 / -3

menu.shHarden cleanup locking and clarify fetch dry-run behavior+15/-3

Harden cleanup locking and clarify fetch dry-run behavior

• Checks that cleanup can open the hub lock file before invoking flock. Captures non-blocking probe stderr to fail immediately on real flock errors while retaining the existing contention wait, and corrects the fetch_hub comment describing dry-run lock behavior.

host-setup/menu.sh

coderabbitai · 2026-08-29T23:43:11Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 189a7618-ae94-489a-8c71-d149f1bdfa4b

📥 Commits

Reviewing files that changed from the base of the PR and between 873d002 and 4ca8ded.

📒 Files selected for processing (1)

host-setup/menu.sh

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

📝 Walkthrough

Walkthrough

Changes

Lock handling

Layer / File(s)	Summary
Dry-run fetch diagnostics `host-setup/menu.sh`	Dry-run output now explains why fetch operations are rejected before it reports a missing reader lock.
Cleanup lock validation `host-setup/menu.sh`	Cleanup now checks lock-file opening and distinguishes flock errors from lock contention. Real errors stop cleanup after descriptor closure.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to 4ca8d

This localized cleanup behavior fix is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main changes: applying lock-probe fixes to cleanup() and correcting a stale comment.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix-cleanup-lock-probe

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

🟢 Approval recommended

The changes are narrowly scoped, consistent with existing lock-probe patterns in the same script, and improve diagnostics without altering the lock’s intended semantics.

Pull request overview

This PR aligns cleanup()’s lock handling with the hardened behavior already applied to hub_read_lock_acquire, and updates a stale fetch_hub() comment about --dry-run behavior so the diagnostics match the actual control flow.

Changes:

Update fetch_hub() comment to reflect that hub_read_lock_acquire no-ops under --dry-run, and explain the purpose of the early --dry-run failure.
Harden cleanup()’s exclusive lock acquisition by explicitly checking the lock-file exec open and by distinguishing lock contention from real flock errors (no stderr suppression).

File summaries

File	Description
host-setup/menu.sh	Improves `cleanup()` lock probing/error reporting and fixes stale `--dry-run` comment to match current behavior.

Review details

Files reviewed: 1/1 changed files
Comments generated: 0
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

qodo-code-review · 2026-08-29T23:45:50Z

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

1. Probe drops cleanup lock 🐞 Bug≡ Correctness

Description

cleanup() runs the nonblocking flock inside command substitution, so an uncontended lock is
acquired only by the subshell and released when that subshell exits. The parent then deletes
$DIR/hub without an exclusive lock, allowing cleanup to race with readers or a concurrent fetch.

Code

host-setup/menu.sh[275]
+ if ! probe_err=$(flock -n "$lock_fd" 2>&1); then

Relevance

●●● Strong
Recent menu.sh precedents accept cleanup and lock-safety correctness fixes; this directly exposes an
exclusive-lock race.
PR-#1041
PR-#1098
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cleanup parent opens the fd, but the changed probe executes flock in a command-substitution
child. Only the contention branch subsequently runs flock in the parent; on probe success
execution proceeds directly to rm -rf, violating the file's documented requirement that cleanup
hold the exclusive writer lock while readers use the tree.

host-setup/menu.sh[267-275]
host-setup/menu.sh[282-292]
host-setup/menu.sh[91-94]
host-setup/menu.sh[299-304]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The command-substitution probe acquires the lock in a subshell, which releases it before the parent reaches deletion.
## Issue Context
Preserve stderr/error classification without moving the successful `flock` acquisition out of the cleanup process. For example, run `flock -n` directly in the parent and distinguish contention with a dedicated conflict exit code, or capture diagnostics through a mechanism that does not execute `flock` in a subshell.
## Fix Focus Areas
- host-setup/menu.sh[274-288]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

✅ Compliance rules (platform): 72 rules

✅ Skills: 5 invoked

comment-and-doc-style

dotnet-codestyle

python-codestyle

shell-codestyle

workflow-ci-contract

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

qodo-code-review · 2026-08-29T23:45:51Z

+ return 1
+ fi
+ local probe_err
+ if ! probe_err=$(flock -n "$lock_fd" 2>&1); then


1. Probe drops cleanup lock 🐞 Bug≡ Correctness
cleanup() runs the nonblocking flock inside command substitution, so an uncontended lock is acquired only by the subshell and released when that subshell exits. The parent then deletes $DIR/hub without an exclusive lock, allowing cleanup to race with readers or a concurrent fetch.
Agent Prompt
## Issue description The command-substitution probe acquires the lock in a subshell, which releases it before the parent reaches deletion. ## Issue Context Preserve stderr/error classification without moving the successful `flock` acquisition out of the cleanup process. For example, run `flock -n` directly in the parent and distinguish contention with a dedicated conflict exit code, or capture diagnostics through a mechanism that does not execute `flock` in a subshell. ## Fix Focus Areas - host-setup/menu.sh[274-288]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

CopilotAI lite review requested due to automatic review settings August 29, 2026 23:42

ptr727 mentioned this pull request Aug 29, 2026
Promote develop to main #1098
Merged

Copilotstarted reviewing on behalf of ptr727 August 29, 2026 23:42 View session

CopilotAI reviewed Aug 29, 2026
View reviewed changes

ptr727 merged commit 333897b into developAug 29, 2026
9 checks passed

ptr727 deleted the fix-cleanup-lock-probe branch August 29, 2026 23:45

qodo-code-reviewBot reviewed Aug 29, 2026
View reviewed changes

Navigation Menu

Apply the Lock-Probe Fixes to cleanup() Too, Fix a Stale Comment - #1106

Apply the Lock-Probe Fixes to cleanup() Too, Fix a Stale Comment#1106
ptr727 merged 1 commit into
developfrom
fix-cleanup-lock-probe

ptr727 commented Aug 29, 2026•
edited by coderabbitai Bot
Loading

Uh oh!

qodo-code-reviewBot commented Aug 29, 2026

Uh oh!

coderabbitaiBot commented Aug 29, 2026•
edited
Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

qodo-code-reviewBot commented Aug 29, 2026

Uh oh!

qodo-code-reviewBotAug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ptr727 commented Aug 29, 2026• edited by coderabbitai BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

qodo-code-reviewBot commented Aug 29, 2026

PR Summary by Qodo

Uh oh!

coderabbitaiBot commented Aug 29, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

qodo-code-reviewBot commented Aug 29, 2026

Code Review by Qodo

Uh oh!

qodo-code-reviewBotAug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ptr727 commented Aug 29, 2026•
edited by coderabbitai Bot
Loading

coderabbitaiBot commented Aug 29, 2026•
edited
Loading