improve error handling around disk partitioning#430
Conversation
WalkthroughThis PR adds retry and polling logic to the Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@platform/windows/disk/partitioner_test.go`:
- Line 283: The failing line uses goimports-incompatible formatting; run
goimports on platform/windows/disk/partitioner_test.go (or the whole repo) and
commit the formatted file so the line with cmdRunner.AddCmdResult(getKey,
fakes.FakeCmdResult{Stdout: "E\n"}) conforms to goimports rules—this will fix
import grouping/formatting and unblock CI; locate the call by searching for
AddCmdResult, getKey, or fakes.FakeCmdResult to confirm the change.
In `@platform/windows/disk/partitioner.go`:
- Around line 174-193: The retry loop in GetDriveLetter (the for attempt := 0;
attempt < 10; attempt++ block using p.ps with driveScript) always calls
time.Sleep(pollInterval) even after the final attempt; modify the loop so it
only sleeps between attempts (e.g., compute maxAttempts := 10 or use attempt <
9) and call time.Sleep(pollInterval) only when attempt < maxAttempts-1, leaving
the error return path (which uses partitionNumber and diskNumber) unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9a1c89b-c08a-454b-b674-b14e6c7a4136
📒 Files selected for processing (4)
integration/windows/environment_test.gointegration/windows/ephemeral_disk_test.goplatform/windows/disk/partitioner.goplatform/windows/disk/partitioner_test.go
|
|
||
| cmdRunner.AddCmdResult(addKey, fakes.FakeCmdResult{}) | ||
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "\x00\n"}) // null char — unassigned | ||
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll |
There was a problem hiding this comment.
Fix goimports formatting at this line to unblock lint.
CI is currently failing goimports on this line; please run goimports on the file and commit the formatted result.
Likely formatting-only adjustment
- cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll
+ cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll📝 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.
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll | |
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll |
🧰 Tools
🪛 GitHub Check: lint (macos-latest)
[failure] 283-283:
File is not properly formatted (goimports)
🪛 GitHub Check: lint (ubuntu-latest)
[failure] 283-283:
File is not properly formatted (goimports)
🤖 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 `@platform/windows/disk/partitioner_test.go` at line 283, The failing line uses
goimports-incompatible formatting; run goimports on
platform/windows/disk/partitioner_test.go (or the whole repo) and commit the
formatted file so the line with cmdRunner.AddCmdResult(getKey,
fakes.FakeCmdResult{Stdout: "E\n"}) conforms to goimports rules—this will fix
import grouping/formatting and unblock CI; locate the call by searching for
AddCmdResult, getKey, or fakes.FakeCmdResult to confirm the change.
| for attempt := 0; attempt < 10; attempt++ { | ||
| stdout, _, _, err := p.ps(driveScript) | ||
| if err != nil { | ||
| return "", fmt.Errorf( | ||
| "failed to find drive letter for partition %s on disk %s: %s", | ||
| partitionNumber, | ||
| diskNumber, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| // The DriveLetter property is a char; an unassigned partition returns the null | ||
| // character (\x00). Strip it along with whitespace before checking. | ||
| letter := strings.Trim(strings.TrimSpace(stdout), "\x00") | ||
| if letter != "" { | ||
| return letter, nil | ||
| } | ||
|
|
||
| time.Sleep(pollInterval) | ||
| } |
There was a problem hiding this comment.
Avoid sleeping after the last retry attempt.
The loop sleeps even on the final failed attempt, adding an unnecessary delay before returning the error.
Proposed fix
for attempt := 0; attempt < 10; attempt++ {
stdout, _, _, err := p.ps(driveScript)
if err != nil {
return "", fmt.Errorf(
"failed to find drive letter for partition %s on disk %s: %s",
partitionNumber,
diskNumber,
err,
)
}
// The DriveLetter property is a char; an unassigned partition returns the null
// character (\x00). Strip it along with whitespace before checking.
letter := strings.Trim(strings.TrimSpace(stdout), "\x00")
if letter != "" {
return letter, nil
}
- time.Sleep(pollInterval)
+ if attempt < 9 {
+ time.Sleep(pollInterval)
+ }
}📝 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.
| for attempt := 0; attempt < 10; attempt++ { | |
| stdout, _, _, err := p.ps(driveScript) | |
| if err != nil { | |
| return "", fmt.Errorf( | |
| "failed to find drive letter for partition %s on disk %s: %s", | |
| partitionNumber, | |
| diskNumber, | |
| err, | |
| ) | |
| } | |
| // The DriveLetter property is a char; an unassigned partition returns the null | |
| // character (\x00). Strip it along with whitespace before checking. | |
| letter := strings.Trim(strings.TrimSpace(stdout), "\x00") | |
| if letter != "" { | |
| return letter, nil | |
| } | |
| time.Sleep(pollInterval) | |
| } | |
| for attempt := 0; attempt < 10; attempt++ { | |
| stdout, _, _, err := p.ps(driveScript) | |
| if err != nil { | |
| return "", fmt.Errorf( | |
| "failed to find drive letter for partition %s on disk %s: %s", | |
| partitionNumber, | |
| diskNumber, | |
| err, | |
| ) | |
| } | |
| // The DriveLetter property is a char; an unassigned partition returns the null | |
| // character (\x00). Strip it along with whitespace before checking. | |
| letter := strings.Trim(strings.TrimSpace(stdout), "\x00") | |
| if letter != "" { | |
| return letter, nil | |
| } | |
| if attempt < 9 { | |
| time.Sleep(pollInterval) | |
| } | |
| } |
🤖 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 `@platform/windows/disk/partitioner.go` around lines 174 - 193, The retry loop
in GetDriveLetter (the for attempt := 0; attempt < 10; attempt++ block using
p.ps with driveScript) always calls time.Sleep(pollInterval) even after the
final attempt; modify the loop so it only sleeps between attempts (e.g., compute
maxAttempts := 10 or use attempt < 9) and call time.Sleep(pollInterval) only
when attempt < maxAttempts-1, leaving the error return path (which uses
partitionNumber and diskNumber) unchanged.
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
beyhan
requested review from
a team,
a-hassanin and
neddp
and removed request for
a team
beyhan
moved this from Waiting for Changes | Open for Contribution
to Pending Review | Discussion
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Pull request overview
Improves error handling around Windows disk partitioning so that an unassigned drive letter (PowerShell's char(0)/null result from Get-Partition.DriveLetter) no longer produces a junction pointing at bare ":" (effectively C:\). The partitioner now polls for a real drive letter and surfaces an explicit error on persistent failure, and integration tests fail loudly rather than silently cleaning up the system partition.
Changes:
- Add retry/polling loop and null-char handling to
Partitioner.AssignDriveLetter, with a configurableDriveLetterPollInterval. - Add unit tests covering transient
\x00recovery and persistent-failure error message. - Add integration guards:
AfterEachassertsdiskLetter != "C"beforeRemove-Partition, andShrinkRootPartitionasserts success after exhausting retries.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| platform/windows/disk/partitioner.go | Adds polling loop, null-character stripping, and explicit failure error in AssignDriveLetter; introduces DriveLetterPollInterval field. |
| platform/windows/disk/partitioner_test.go | Adds tests for transient \x00 followed by valid letter and for sticky failure exhausting all attempts. |
| integration/windows/ephemeral_disk_test.go | Guards Remove-Partition cleanup against accidentally targeting C:. |
| integration/windows/environment_test.go | Tracks shrunkSuccessfully and asserts true after the retry loop so silent retry exhaustion fails the test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return letter, nil | ||
| } | ||
|
|
||
| time.Sleep(pollInterval) |
|
Was this not superceded by #432 ? |
The test cleanup code was trying to unmount c:, this should catch the actual failure instead of falling through to the cleanup code.