fix(cas): fall back from unwritable installation pools (backport of #349) - #350
Conversation
* fix(cas): fall back from an unwritable installation CAS pool * fix(cas): preserve fallback pool state safely * fix(cas): harden writable pool fallback * fix(cas): honor pool selection cancellation * fix(cas): normalize legacy pool roots * fix(cas): avoid duplicate primary legacy storage * test(cas): tolerate cleanup failures in installation pool tests * refactor(cas): drop a redundant installation-path check * fix(cas): retain every previous installation pool root for lookup (cherry picked from commit b3f5c4a)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| private bool IsInsideApplicationDirectory(string rootPath) | ||
| { | ||
| var appBaseDirectory = Path.TrimEndingDirectorySeparator(Path.GetFullPath(AppContext.BaseDirectory)); | ||
| var normalizedRootPath = Path.TrimEndingDirectorySeparator(Path.GetFullPath(rootPath)); |
There was a problem hiding this comment.
WARNING: Path.GetFullPath can throw unhandled from the DI constructor.
NormalizeRoot and IsInsideApplicationDirectory are reached via RefreshInstallationPools() in the CasPoolManager constructor, and RefreshLegacyInstallationPool calls them once per entry in the user-supplied LegacyInstallationPoolRootPaths. A malformed or illegal-character path in settings raises ArgumentException/PathTooLongException/SecurityException out of constructor resolution, preventing the app from starting. The previous check used Path.TrimEndingDirectorySeparator on the raw value and only checked a single root; wrap these calls in try/catch (treat as "skip/invalid root") or validate first.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (!saved) | ||
| { | ||
| logger.LogError("Failed to save installation CAS pool settings"); | ||
| return false; |
There was a problem hiding this comment.
WARNING: Save-failure path leaves in-memory settings and the pool manager inconsistent.
UserSettingsService.TryUpdateAndSaveAsync applies the mutation to the in-memory settings before persisting and returns false if SaveAsync throws (UserSettingsService.cs:113-139). On that branch this code returns false and skips casPoolManager.ReinitializeInstallationPool(), so in-memory settings now point at the new pool while the pool manager still routes to the previous configuration. Additionally, aborting the entire GameClient acquisition for a transient settings-persistence error contradicts the graceful primary-pool fallback intent of this change. Consider rolling back the in-memory mutation, or returning true to continue with the primary pool when persistence fails.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| return await installationCasPoolService.EnsurePoolPathAsync(installations, cancellationToken); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
WARNING: Cancellation is swallowed and surfaced as a hard acquisition failure.
IInstallationCasPoolService.EnsurePoolPathAsync calls cancellationToken.ThrowIfCancellationRequested(), but this blanket catch (Exception ex) catches the resulting OperationCanceledException, and the newly-added return false then turns a user cancellation into CreateFailure("Could not ensure storage for GameClient content.") at the call site instead of letting it propagate. Re-throw OperationCanceledException (and TaskCanceledException) before the generic handler so cancellation flows correctly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| var poolPathReady = await EnsureInstallationPoolPathAsync(cancellationToken); | ||
| if (!poolPathReady) | ||
| { | ||
| return OperationResult<ContentManifest>.CreateFailure( |
There was a problem hiding this comment.
WARNING: New early-return path leaks the downloaded archive and extract directory.
By the time this guard runs, archivePath (downloaded archive) and extractPath (extracted content) already exist, but this return executes before CleanupTemporaryFilesAsync(archivePath, extractPath) at the end of the method, and DeliverContentAsync has no finally. Failed GameClient acquisitions will therefore leave those temp files behind on disk. Run the cleanup (or move the pool-ensure and cleanup into a try/finally) before returning.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| try | ||
| { | ||
| return Path.GetFullPath(path); |
There was a problem hiding this comment.
SUGGESTION: NormalizePath diverges from CasPoolManager.NormalizeRoot.
This helper calls Path.GetFullPath without Path.TrimEndingDirectorySeparator, while CasPoolManager.NormalizeRoot trims. A user-configured InstallationPoolRootPath that ends in a separator keeps it, so currentPath never equals effectivePath/candidatePath, making settingsAlreadyMatch perpetually false and triggering a settings rewrite on every run. Align the two normalizers.
| return Path.GetFullPath(path); | |
| return Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| CasRootPath = CasRootPath, | ||
| InstallationPoolRootPath = InstallationPoolRootPath, | ||
| IsInstallationPoolRootPathAutoDerived = IsInstallationPoolRootPathAutoDerived, | ||
| LegacyInstallationPoolRootPaths = [.. LegacyInstallationPoolRootPaths], |
There was a problem hiding this comment.
SUGGESTION: Clone throws NullReferenceException if the list is null.
If a persisted CasConfiguration deserializes LegacyInstallationPoolRootPaths as null (e.g. hand-edited or corrupted settings), the spread [.. LegacyInstallationPoolRootPaths] throws here, and the equivalent [.. userCasConfig.LegacyInstallationPoolRootPaths] in CasModule.cs runs during startup options binding. Guard the spread.
| LegacyInstallationPoolRootPaths = [.. LegacyInstallationPoolRootPaths], | |
| LegacyInstallationPoolRootPaths = [.. (LegacyInstallationPoolRootPaths ?? [])], |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (22 files)
Fix these issues in Kilo Cloud Reviewed by glm-5.2 · Input: 143.5K · Output: 37.8K · Cached: 1.4M |
1 participant
Summary
Backport of #349 into
release/alpha-4.Prevents installation-adjacent CAS storage from breaking content acquisition when
the game is installed in a protected location such as Program Files. GenHub
verifies that it can create and write the actual installation-pool directory
before routing content there. When the location is unavailable, new content uses
the primary user-writable pool while existing readable objects remain
discoverable through read-only legacy pools.
See #349 for the full description of the change.
Backport notes
-x; no conflicts and no manual edits.release/alpha-4and the mergebase of fix(cas): fall back from unwritable installation pools #349, and both auto-merged. The executable-classifier call in
CommunityOutpostDeliverercorrectly kept this branch's single-argumentsignature, since fix(content): classify executables by magic bytes instead of the extensionless heuristic #339 and fix(tests): pass the absolute path to the executable classifier #345 are not on
release/alpha-4.the cross-volume copy path used when materialization falls back to the primary
pool, and fix: disable unsafe CAS garbage collection #312 keeps automatic CAS garbage collection disabled so retained
legacy pools are never mutated.
Testing
dotnet test GenHub/GenHub.Tests/GenHub.Tests.Core/GenHub.Tests.Core.csproj -c Releasedotnet build GenHub/GenHub.Linux/GenHub.Linux.csproj -c Releasegit diff --checkprobing, and legacy-content lookup all run and pass on this branch.
Risks and rollback
are retained for lookup only, while new writes use the effective writable pool.
prior behavior when absent.
protected-path acquisition failures.
Manual validation
Packaged non-administrator Windows validation was completed for #349: GameClient
content acquired with the game under Program Files, GenHub restarted, profile
created, and launched.
That run covers this backport too. The CAS code here is identical to #349, and
the only production difference against
release/alpha-4is theexecutable-classifier call in
CommunityOutpostDeliverer, which is outside thestorage-routing path.
Backport of #349
Fixes #347
Greptile Summary
This backport makes installation-adjacent CAS routing conditional on actual directory writability and preserves previous installation pools for read-only object lookup.
Confidence Score: 5/5
The PR appears safe to merge, with no concrete changed-code-triggered failures identified.
The updated routing probes the actual target directory before enabling installation-pool writes, falls back to primary storage when unavailable, and retains prior readable pools for object lookup.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Acquire installation-related content] --> B[Detect game installations] B --> C[Derive installation-adjacent CAS path] C --> D{Directory accepts writes?} D -->|Yes| E[Persist active installation pool] D -->|No| F[Route new objects to primary pool] E --> G[Reinitialize CAS pool routing] F --> G G --> H[Store new content] I[Previous installation pools] --> J[Retain as read-only legacy pools] J --> K[Lookup active, primary, and legacy pools] H --> KReviews (1): Last reviewed commit: "fix(cas): fall back from unwritable inst..." | Re-trigger Greptile
Context used (4)