fix(steam/download): close race that hangs Update/Verify at 0%#1434
fix(steam/download): close race that hangs Update/Verify at 0%#1434jeremybernstein wants to merge 1 commit into
Conversation
downloadJobs[appId] = info was assigned AFTER scope.launch{} kicked off
the work. on a fast verify/update (game already on disk, DepotDownloader
has no chunks to fetch) the launched coroutine could complete and call
removeDownloadJob(appId) BEFORE the outer assignment ran. removeDownloadJob
found no entry, skipped notifyDownloadStopped, then the outer code
populated the map with a stale completed-job DownloadInfo and emitted
notifyDownloadStarted. UI hung at 0% with no stop event ever coming.
cancel was a no-op too — DownloadInfo.downloadJob may still have been
null (setDownloadJob hadn't run), and even when set the job was already
complete. retry returned the same stale DownloadInfo from downloadJobs
via the line ~1666 short-circuit.
fix:
- register downloadJobs[appId] + notifyDownloadStarted BEFORE launching
the coroutine, so the inline removeDownloadJob inside the launch body
always finds the entry and emits DownloadStatusChanged(false).
- invokeOnCompletion now unconditionally calls removeDownloadJob as a
safety net for paths the inline call doesn't cover (early return on
empty licenses, exceptions before the catch handlers, cancellations
out of suspension points). second call is a no-op.
event ordering for the success path is preserved: inline
removeDownloadJob fires first (DownloadStatusChanged false), then
LibraryInstallStatusChanged, then the invokeOnCompletion no-op.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDownload Job Lifecycle
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Please upload a recording - I have not seen this issue before |
I'm not sure why I would make this up, but here. I left it at a minute or so, but it's still hung at 0% while I upload this: screen-20260517-171532.mp4 |
Description
Fixes a long-standing race in
SteamService.downloadApp(...)that hangs Update and Verify at 0% on games whose depot work completes synchronously inside the launched coroutine.downloadJobs[appId] = infowas assigned afterinstance!!.scope.launch { ... }had already kicked off the download body. For a fast verify/update (game already on disk, DepotDownloader has no chunks to fetch) the launched coroutine can complete and reach its inlineremoveDownloadJob(appId)before the outer assignment runs.removeDownloadJobfinds no entry, skipsnotifyDownloadStopped, and then the outer code populates the map with a now-stale completed-jobDownloadInfoand emitsnotifyDownloadStarted. The UI sits at 0% with no stop event ever coming.Cancel can't recover either:
DownloadInfo.downloadJobmay still benull(thesetDownloadJobline hadn't run yet), and even if set, the job is already complete — socancel()is a no-op. The retry path returns the same staleDownloadInfovia theif (downloadJobs.contains(appId)) return getAppDownloadInfo(appId)short-circuit at line ~1666, so the second attempt hangs identically.Two surgical changes:
downloadJobs[appId]+ emitnotifyDownloadStartedbefore launching the coroutine. The inlineremoveDownloadJobinside the launch body now always finds the entry and emitsDownloadStatusChanged(false).invokeOnCompletionnow unconditionally callsremoveDownloadJob(appId)as a safety net for paths the inline call doesn't cover: the earlyreturn@launchon empty licenses, exceptions thrown before thecatchhandlers, and cancellations out of suspension points. The second call is a no-op when the inline path already removed the entry, so the existing event ordering is preserved on the success path (inlineremoveDownloadJob→LibraryInstallStatusChanged→ safety-net no-op).Closes #1433
Recording
N/A — fix is for a hang. Before: progress bar parks at 0% forever on Update/Verify of an up-to-date title. After: completes and Play button returns. Happy to attach a screen recording if needed.
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Verified on device
Built
:app:assembleDebug, installed on a physical device, ran Update and Verify on an already-installed title repeatedly. No more hang at 0%; Play button returns reliably.Summary by cubic
Fixes a race in
SteamService.downloadAppthat caused Update/Verify to hang at 0% on up-to-date games (fixes #1433). Ensures start/stop events emit correctly and cancel/retry work reliably.downloadJobs[appId]and callnotifyDownloadStartedbefore launching the coroutine soremoveDownloadJobalways finds the entry and the UI receives the stop event.removeDownloadJob(appId)unconditionally ininvokeOnCompletionto cover early returns, exceptions, and cancellations without leaving stale state.Written for commit 9c0188b. Summary will update on new commits.
Summary by CodeRabbit