fix(steam): roll back failed proxy preparation - #304
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rollback = new PreparationRollback( | ||
| targetExePath, | ||
| backupPath, | ||
| proxySourcePath, | ||
| filesToCapture); |
There was a problem hiding this comment.
When two profiles sharing a Steam installation launch concurrently, their separate per-profile locks allow both preparations to snapshot and mutate the same executable, backup, config, and App ID files. One rollback can then overwrite the surviving profile's configuration or delete the backup it requires, leaving the wrong launch configuration or an unrestorable proxy executable.
Knowledge Base Used: Game Profiles and Launching
Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/Launching/SteamLauncher.cs
Line: 207-211
Comment:
**Installation mutations are not serialized**
When two profiles sharing a Steam installation launch concurrently, their separate per-profile locks allow both preparations to snapshot and mutate the same executable, backup, config, and App ID files. One rollback can then overwrite the surviving profile's configuration or delete the backup it requires, leaving the wrong launch configuration or an unrestorable proxy executable.
**Knowledge Base Used:** [Game Profiles and Launching](https://app.greptile.com/genhub/-/custom-context/knowledge-base/community-outpost/genhub/-/docs/game-profiles-launching.md)
How can I resolve this? If you propose a fix, please make it concise.| if (!File.Exists(_backupPath)) | ||
| { | ||
| var backupStagingPath = CreateTemporaryPath(_backupPath); | ||
| _temporaryFiles.Add(backupStagingPath); | ||
| File.Copy(_targetExePath, backupStagingPath, overwrite: false); | ||
| File.Move(backupStagingPath, _backupPath, overwrite: false); | ||
| _temporaryFiles.Remove(backupStagingPath); | ||
| _backupCreated = true; | ||
| } |
There was a problem hiding this comment.
When an unrelated .ghbak already exists beside a valid executable, successful preparation preserves it without validation. The next launch's cleanup unconditionally replaces the current executable with that stale backup, after which preparation backs up the stale contents and the valid original is no longer recoverable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/Launching/SteamLauncher.cs
Line: 540-548
Comment:
**Stale backups become authoritative**
When an unrelated `.ghbak` already exists beside a valid executable, successful preparation preserves it without validation. The next launch's cleanup unconditionally replaces the current executable with that stale backup, after which preparation backs up the stale contents and the valid original is no longer recoverable.
How can I resolve this? If you propose a fix, please make it concise.![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| { | ||
| var backupStagingPath = CreateTemporaryPath(_backupPath); | ||
| _temporaryFiles.Add(backupStagingPath); | ||
| File.Copy(_targetExePath, backupStagingPath, overwrite: false); |
There was a problem hiding this comment.
CRITICAL: Missing file existence check before copying target executable
The code attempts to copy _targetExePath without checking if it exists. This can fail when the target executable doesn't exist but the backup file does (e.g., after previous proxy deployment).
The validation in PrepareForProfileAsync (line 129) allows this scenario, so File.Copy(_targetExePath, backupStagingPath, overwrite: false) at line 544 will throw FileNotFoundException if _targetExePath doesn't exist.
| File.Copy(_targetExePath, backupStagingPath, overwrite: false); | |
| if (!File.Exists(_backupPath)) | |
| { | |
| var backupStagingPath = CreateTemporaryPath(_backupPath); | |
| _temporaryFiles.Add(backupStagingPath); | |
| if (_targetInitiallyExisted) | |
| { | |
| File.Copy(_targetExePath, backupStagingPath, overwrite: false); | |
| File.Move(backupStagingPath, _backupPath, overwrite: false); | |
| _temporaryFiles.Remove(backupStagingPath); | |
| _backupCreated = true; | |
| } | |
| } |
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)CRITICAL
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by glm-4.7 · Input: 63.2K · Output: 9.6K · Cached: 534.4K |
1 participant
Summary
Prevent failed Steam proxy preparation from leaving the proxy installed.
Changes
Testing
Risks and rollback
Process termination can bypass managed rollback. File locks or concurrent changes may prevent restoration; recovery files are retained and reported.
Related issues
Fixes #303
Greptile Summary
The PR adds transactional filesystem handling for Steam proxy preparation.
Confidence Score: 2/5
This PR should not merge until preparation is serialized per installation and preserved unrelated backups cannot be restored on a later launch.
Concurrent profiles can interleave mutations and invalidate each other's rollback state, while an unrelated backup preserved by a successful attempt is unconditionally restored during the next pre-launch cleanup.
Files Needing Attention: GenHub/GenHub/Features/Launching/SteamLauncher.cs
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Launch Steam profile] --> B[Pre-launch cleanup] B --> C[Validate prerequisites] C --> D[Snapshot executable and files] D --> E[Create or preserve backup] E --> F[Deploy proxy] F --> G[Write config and App ID] G --> H{Preparation completed?} H -->|Yes| I[Commit and launch] H -->|Failure or cancellation| J[Rollback mutations] J --> K{Executable unchanged?} K -->|Yes| L[Restore original and clean artifacts] K -->|No| M[Retain recovery files and report conflict]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(steam): roll back failed proxy prepa..." | Re-trigger Greptile
Context used: