fix(web): hold a wake lock so a sleeping machine can't kill an upload - #245
Conversation
ravirajsinh45
left a comment
ravirajsinh45
left a comment
There was a problem hiding this comment.
Thanks @Lennart-Pingpong. The approach is right and the three details you called out in #244 are the correct three. We did find a race in the reference counting, so rather than send it back we pushed a fix to your branch.
Concurrent uploads requested a sentinel each. acquireWakeLock guarded on if (wakeLock) return, but that is only set after the await resolves. handleStartUpload runs pendingFiles.forEach(f => startUpload(f)), and each startUpload runs synchronously as far as retainWakeLock() before its first await. So dropping three files produced three concurrent navigator.wakeLock.request('screen') calls, only the last of which was stored. The other two were never released and held the screen awake for the life of the page, which is the opposite of what the change is for.
Worth flagging why the suite did not catch it: requests the lock only once for concurrent uploads awaits between the two retainWakeLock() calls, so by the second call the first has already resolved and the guard works. That await is exactly what hides the race.
A lock arriving after the last upload finished was held forever. If an upload completes while request() is still in flight, releaseWakeLock() runs while wakeLock is still null, so it releases nothing; the sentinel then arrives and is stored with the holder count already at zero. Short files on a fast connection hit this.
The fix guards on a pending promise so only one request is ever in flight, and re-checks the holder count after awaiting so a sentinel that arrives too late is released immediately. We also moved retainWakeLock() above the try, since it sat inside it while the release sits in the finally, meaning a throw before the retain would have released a concurrent upload's lock. And the same CHANGELOG ordering nit as #243.
Two regression tests came with it, both failing before the change and passing after. Full suite is 271 green.
Please give it a sanity read when you get a chance. Merging after that and once CI re-runs.
An upload lives entirely in the browser tab. When the machine suspends mid-transfer, the loop pushing chunks dies with it - and unlike a failed part, nothing runs afterwards: the catch that calls /upload/abort never executes. The version is left at processing_status='uploading', which in the UI is indistinguishable from a stalled transcode, and the multipart upload stays open until reap_stale_uploads gets to it. The browser is now asked for a Screen Wake Lock while an upload runs. Reference-counted, since several uploads can be in flight; the lock is only released when the last one finishes, and re-acquired on visibilitychange because browsers drop it whenever the tab is hidden. Best-effort on purpose: without HTTPS, in low-power mode, or in a browser without the API there is simply no lock. None of that is a reason to refuse the upload, so every failure path is swallowed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects in the reference-counted lock, both on paths users hit.
acquireWakeLock had no in-flight guard. Dropping several files calls
startUpload once per file, and each runs synchronously as far as
retainWakeLock() before its first await, so N files meant N concurrent
navigator.wakeLock.request('screen') calls. Only the last sentinel was
stored in `wakeLock`; the others were never released and held the screen
awake for the life of the page. The existing "requests the lock only once"
test awaited between the two retains, which is exactly what hid the race.
Second, an upload finishing before the request resolved left the lock held
forever: releaseWakeLock ran while `wakeLock` was still null, then the
sentinel arrived and was stored with the holder count already at zero. The
acquire path now re-checks holders after awaiting and releases immediately
if everything has finished.
Also moves retainWakeLock() above the try, so a throw before it cannot run
the finally's release against a concurrent upload's lock, and orders the
CHANGELOG sections per Keep a Changelog.
Two regression tests added, both failing before this change.
713dd00 to
f252ae9
Compare
2 participants
Summary
An upload lives entirely in the browser tab, so a machine suspending mid-transfer
kills the loop pushing chunks. Unlike a failed part, nothing runs afterwards —
the
catchcalling/upload/abortnever executes — so the version is left atprocessing_status = 'uploading', indistinguishable in the UI from a stalledtranscode, and the multipart upload stays open until
reap_stale_uploadsgets toit.
The browser is now asked for a Screen Wake Lock while an upload runs.
Closes #244
Changes
retainWakeLock()/releaseWakeLock(), reference-counted so the lock is onlydropped when the last concurrent upload finishes
finallythat already cleans up the abort controller, so italso covers the cancel and error paths
visibilitychange, because browsers drop the lock when the tabis hidden and the dropped sentinel cannot be reused
without the API simply means no lock, never a failed upload
request-only-once case, the missing-API case and a refused lock
Testing
pnpm test→ 269 tests passing,pnpm exec tsc --noEmitclean,pnpm buildsucceeds)stub; the real-world effect (a laptop lid closing) is by nature awkward to
assert in CI. Happy to run a manual pass if you'd like one before merging.
Checklist
CHANGELOG.mdentry added under## [Unreleased]→### FixedScreenshots
n/a — no UI changes.