fix(userdata): resolve profile deactivation file conflict - #393
fix(userdata): resolve profile deactivation file conflict #393undead2146 wants to merge 3 commits into
Conversation
undead2146
changed the title
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
80bbe4f to
afb9fa5
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (index.FileToInstallationMap.TryGetValue(normalizedPath, out var installationKey)) | ||
| { | ||
| return OperationResult<string?>.CreateSuccess(installationKey); | ||
| var manifest = await LoadUserDataManifestByKeyAsync(installationKey, cancellationToken); |
There was a problem hiding this comment.
[WARNING]: An unreadable manifest is treated the same as a missing one, so a transient read failure can strip an active installation's ownership.
LoadUserDataManifestFromFileAsync (line 1568) returns null both when the manifest file is absent and when reading or deserializing throws (sharing violation, AV/OneDrive lock, corrupt JSON). The new manifest != null && manifest.IsActive guard therefore prunes the FileToInstallationMap entry of a still-active installation on any transient I/O error, and the public CheckFileConflictAsync twin (lines 573-581) persists that prune to disk. Another installation can then claim the path, and a later uninstall of the original manifest removes the new owner's mapping via the blind FileToInstallationMap.Remove in UpdateIndexUnlockedAsync (line 1727) — the same corruption shape the earlier review finding described, now reachable through a locked or corrupt manifest read. DeleteAllUserDataAsync (lines 709-713) already distinguishes these cases with File.Exists(GetManifestFilePath(key)); these conflict paths should do the same and keep the mapping (or return a failure) when the manifest file exists but cannot be read.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| string absolutePath, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| await IndexLock.WaitAsync(cancellationToken); |
There was a problem hiding this comment.
[WARNING]: OperationCanceledException is swallowed and converted into a failure result.
The rewritten CheckFileConflictAsync now performs cancellable awaits inside its try (lock acquisition here, index read at 568, manifest read at 573, index write at 581) but keeps the generic catch (Exception ex) at line 586, so cooperative cancellation surfaces as CreateFailure with a cancelled message. Every other public method in this file rethrows OCE before its generic catch (lines 171, 227, 280, 446, 480, 503 — line 505 even documents why cancellation must escape), and this same PR adds catch (OperationCanceledException) when (...) { throw; } four times in ProfileContentLinkerService.cs. The same gap applies to the new manifest-load await at line 1646 inside CheckFileConflictUnlockedAsync's generic catch, so a cancelled install returns a plain failure (plus a full rollback) from InstallUserDataAsync instead of propagating OCE as its own catch at lines 171-174 promises.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| // Installation is inactive or manifest no longer exists; clean up stale in-memory mapping | ||
| index.FileToInstallationMap.Remove(normalizedPath); |
There was a problem hiding this comment.
[SUGGESTION]: The in-memory prune of the shared cached index is not transactionally scoped.
LoadIndexUnlockedAsync returns the _cachedIndex instance itself, so this Remove mutates shared state. If the enclosing install fails before UpdateIndexUnlockedAsync persists (materialization failure, per-file abort, cancellation, or a throw while persisting), nothing rolls the prune back: memory and disk diverge for the rest of the process, and the next unrelated SaveIndexAsync silently makes the prune durable. The public variant (line 581) instead rewrites the entire index file on every stale check through the pre-existing non-atomic File.WriteAllTextAsync path (no temp+rename like SaveUserDataManifestAsync at lines 1538-1543), so a crash mid-write corrupts the index for all future operations. Consider snapshotting prune decisions and persisting them only on successful install, and making SaveIndexAsync atomic.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .Where(file => file.InstallTarget != ContentInstallTarget.System && | ||
| (file.InstallTarget != ContentInstallTarget.Workspace || | ||
| manifest.ContentType is ContentType.Map or ContentType.MapPack)) | ||
| .Select(file => (manifest.ContentType is ContentType.Map or ContentType.MapPack) && |
There was a problem hiding this comment.
[WARNING]: Blanket remap of every Workspace file in Map/MapPack manifests into the user Maps directory.
The Select remaps all Workspace-targeted files to UserMapsDirectory, but manifest providers deliberately route non-map payloads to Workspace even for map content — CommunityOutpostManifestFactory sends .big/.ini/.exe/.dll/data/ files to Workspace. Those files are still materialized into the workspace by reconciliation and now also get copied into Documents/.../Maps/; ResolveUserDataTargetPath only strips a leading Maps/ segment (UserDataTrackerService.cs:802), so Data/x.big lands at Maps/Data/x.big and setup.exe at Maps/setup.exe, duplicating bytes (user-writable targets always take full copies) and polluting the folder the game scans for maps. Additionally, same-named Workspace files across two installed packs (for example readme.txt) both resolve to Maps/readme.txt, so the second pack's install hard-fails the entire profile prepare through the conflict abort — a failure mode that did not exist before this remap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| var userDataFiles = manifest.Files | ||
| .Where(f => f.InstallTarget != ContentInstallTarget.Workspace && | ||
| f.InstallTarget != ContentInstallTarget.System) | ||
| .Where(file => file.InstallTarget != ContentInstallTarget.System && |
There was a problem hiding this comment.
[SUGGESTION]: Duplicated user-data predicate will drift.
This inline Where predicate is a verbatim copy of HasProfileUserData (lines 377-380). If one copy changes (for example adding ContentType.Mission or filtering by file extension), filtering and installation silently disagree: a manifest can be classified as user data yet install zero files (hitting the empty-success branch at line 424), or files can be installed for manifests the flow skipped. Extract a shared per-file predicate such as IsProfileUserDataFile(manifest, file) and use it in both places.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Assert: No conflict reported and stale mapping is pruned | ||
| Assert.True(conflictResult.Success); | ||
| Assert.Null(conflictResult.Data); |
There was a problem hiding this comment.
[WARNING]: The pruning this test claims to verify is never actually checked.
The test name and comment say the stale mapping is pruned, but only Assert.Null(conflictResult.Data) is asserted — deleting the Remove + SaveIndexAsync lines added at UserDataTrackerService.cs:580-581 (while still returning null) would pass all three new tests, leaving stale mappings to accumulate on disk. Read the persisted index JSON under the fixture's app-data directory and assert the path key is gone.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| // Arrange | ||
| var gameDataDir = Path.Combine(_zeroHourDataDir, "Maps", "Arabia v2"); | ||
| Directory.CreateDirectory(gameDataDir); |
There was a problem hiding this comment.
[SUGGESTION]: Dead arrangement — the pre-created directory is never used.
Directory.CreateDirectory(gameDataDir) creates an empty Maps/Arabia v2 folder that no file is placed in before install A; the install path creates directories itself and no pre-existing-content or backup scenario is set up, so the lines only suggest an arrangement the test never exercises. Remove them, or seed a pre-existing file there if the overwrite path was meant to be covered.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| $"Expected exit failure message, but got: {errors}"); | ||
| Assert.True( | ||
| stopwatch.Elapsed < TimeSpan.FromSeconds(5), | ||
| stopwatch.Elapsed < config.ExpectedChildDiscoveryTimeout, |
There was a problem hiding this comment.
[WARNING]: Self-referential timing bound no longer verifies fast failure.
The assertion changed from Elapsed < 5s against a 10s timeout to Elapsed < config.ExpectedChildDiscoveryTimeout while that same timeout was raised to 30s (line 159), so any failure taking up to ~30s now passes while the message still claims Expected a fast failure. The test's documented purpose (FailsWithoutWaitingOutTheTimeoutAsync) is to prove failure is meaningfully faster than the timeout; a bound such as half the timeout keeps that property with CI headroom. Note 30s also no longer mirrors the production default (10s in ProcessConstants).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| File.Delete(tempExe); | ||
| } | ||
| } | ||
| catch (IOException) |
There was a problem hiding this comment.
[SUGGESTION]: Best-effort cleanup can still fail the test.
File.Delete on a just-terminated process's temp script can also throw UnauthorizedAccessException (read-only attribute, ACL or Controlled-Folder-Access interference), which escapes catch (IOException) and fails an otherwise-passing test from its own finally block. The repo's best-effort I/O convention catches both (see GameProcessManager.HasExecutePermission); use a filter like catch (Exception ex) when (ex is IOException or UnauthorizedAccessException).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| !.gitignore | ||
| !.agents/ | ||
| !.claude/ | ||
| !.gitattributes |
There was a problem hiding this comment.
[SUGGESTION]: Whitelisted .gitattributes does not exist in the repo.
!.gitattributes un-ignores the file, but no .gitattributes exists anywhere in the repository and none is added in this PR, which suggests a companion file (line-ending or linguist rules) was intended but forgotten. Harmless if this is deliberate future-proofing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 13 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by glm-5.3 · Input: 106.1K · Output: 39.3K · Cached: 2.2M |
1 participant
Summary
Resolves user data file conflicts when a previously active profile is deactivated during profile switching, refactors
CasPoolManagerpath checking helper, and introduces thescripts/build-check.ps1build mutex script.Note
This is a self-contained side PR carved out of #265 and must be merged ahead of #265.
Motivation & Problem
Changes
ProfileContentLinkerServiceandUserDataTrackerServiceto handle prior owner deactivation without file collision.CasPoolManagerwith staticIsInsideApplicationDirectorymethod.scripts/build-check.ps1with cross-process mutex and Visual Studio debugger detection.UserDataTrackerServiceTests.cs, cleaned upGameProcessManagerTests.csandGameInstallationValidatorTests.cs.Verification