diff --git a/CLAUDE.md b/CLAUDE.md index 5f71c0e..4215184 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2444,14 +2444,17 @@ could otherwise be picked up silently.) Three properties worth preserving if you touch this: -- **Staged, then swapped.** The download is verified, extracted to - `.new`, checked with `executabilityProblem()` and stamped with its - `version.json` — and only then swapped in via two renames, with the old tree - moved to `.old` and deleted afterwards. It used to delete the live - install and extract over the top, which left the user with *nothing* if that - window was interrupted. If the second rename fails the first is undone. -- **`version.json` is written last**, inside the staged tree. It is the commit - marker: a tree that never completed can never look valid. +- **Staged, then swapped — but only when there is an install to protect.** An + *upgrade* is verified, extracted to `.new`, stamped with its + `version.json` and only then swapped in via two renames, with the old tree + moved to `.old` and deleted afterwards; if the second rename fails the + first is undone. It used to delete the live install and extract over the top, + which left the user with *nothing* if that window was interrupted. A **first + install** extracts straight into `` and performs no rename at all — + there is nothing to protect, and the rename is the fragile step (see below). +- **`version.json` is written last**, inside whichever tree was written. It is + the commit marker: a tree that never completed can never look valid — which + is also what makes extracting a first install in place safe. - **Direction is checked, not just equality.** Installed *newer* than expected is `newerThanExpected` — kept, with a one-time warning at startup — not `outdated`. Treating it as outdated downgraded a deliberately newer bundle, @@ -2459,6 +2462,61 @@ Three properties worth preserving if you touch this: `compareVersions`, not `!=` or a string compare: `"1.10.0"` sorts before `"1.9.0"` lexically. +> **A Windows directory rename is refused while anything holds a handle inside +> it, and that is not a permissions problem (issue #87).** Measured: an open +> read handle on one descendant file, or a child process whose working directory +> is inside the tree, is enough — both surface as +> `PathAccessException … Access is denied, errno = 5`. A *running* `.exe` inside +> the tree is **not** enough, and a destination that already exists gives +> **errno 183** instead, so the two can be told apart. Straight after writing a +> ~200 MB bundle there is routinely something holding a handle for a few hundred +> milliseconds — a scanner, the search indexer, Explorer building a thumbnail — +> and the reporter's install failed on that every time, throwing away the whole +> download at the very last step. +> +> Both renames therefore go through `retryTransientFsOperation` (~7.5s over 12 +> attempts), as does the `.new`/`.old` cleanup at the start — a leftover `.new` +> can still be held by whatever blocked the swap, and an unguarded delete there +> failed the *next* attempt with a second, different error. +> `PathExistsException`/`PathNotFoundException` are rethrown immediately: those +> will not clear, and burning the budget on them delays a fault the user can act +> on. `dependency_install_retry_test.dart` reproduces the real errno 5 on +> Windows and skips elsewhere, because POSIX renames a directory happily with +> its files open. +> +> **`executabilityProblem()` runs after the swap on Windows, before it +> everywhere else.** The quarantine case it guards (issue #50) is macOS-only, so +> on Windows all it can report is a generic "would not run" — while executing a +> freshly written, unsigned 100 MB binary is exactly what makes a scanner open +> the tree we are about to rename. A Windows failure rolls the previous bundle +> back out of `.old`, so it still cannot leave the user worse off. + +**The zip survives a failed install, and the dialog says what is happening.** +Three things about the feedback, all of which #87 exposed: + +- **The download is cached, not thrown away.** It lands in + `/vapourbox-deps-cache/` and is deleted **only after the + install succeeds**, so Retry skips a ~200 MB re-download of bytes that were + never the problem. It is reused only when the sidecar supplied a sha256 to + check it against — without one, a truncated download is indistinguishable + from a complete one and would surface as a corrupt bundle. Anything in that + directory under another name is another version's leftovers and is pruned, + which is what bounds the cache. +- **The install phase emits progress.** It used to emit nothing between the last + extraction tick and `Complete`, so the swap happened under a bar reading + "Extracting… 100%" — and the retry budget added to that would have been + indistinguishable from a hang. `_reportInstallStep` sends 0/0 events (an + indeterminate bar, deliberately: the swap has no fraction to report) and + `retryTransientFsOperation`'s `onRetry` names the wait. +- **The remedy is chosen from the failure.** `DependencyManager.remedyFor` maps + the error to advice — held handles, a full disk, a permissions fault, or the + connection line as the fallback — and `DependencyInstallException` carries its + own for messages that already say what to do (macOS quarantine ships its + `xattr` command). The dialog used one fixed line of *connection* advice under + every failure, which is why the reporter went looking at folder ACLs. The + headings were wrong for the same reason and now say **Installation Failed**, + not "Download Failed" — most of what can fail here happens after the download. + The critical-file list also includes a file that exists **only** in the R78 layout (`libvapoursynthfilters`, plus `vapoursynth/__init__.py` on Unix). `vspipe`, `plugins/` and `ffmpeg` all exist in both layouts, so without an diff --git a/app/lib/services/dependency_manager.dart b/app/lib/services/dependency_manager.dart index 2376148..de68427 100644 --- a/app/lib/services/dependency_manager.dart +++ b/app/lib/services/dependency_manager.dart @@ -62,6 +62,26 @@ class DownloadProgress { String get progressPercent => '${(progress * 100).toStringAsFixed(1)}%'; } +/// An install failure that already knows what to tell the user. +/// +/// The download dialog used to print one fixed line — "Please check your +/// internet connection and try again" — under whatever went wrong. That is +/// wrong for everything past the download, and issue #87 is the case that +/// showed it: a rename refused by a held file handle is not a network problem, +/// and the advice sent the reporter auditing folder permissions instead. +/// +/// [remedy] may be empty, meaning [message] already says what to do (the macOS +/// quarantine text carries its own `xattr` command, for instance). +class DependencyInstallException implements Exception { + final String message; + final String remedy; + + DependencyInstallException(this.message, {required this.remedy}); + + @override + String toString() => message; +} + /// Metadata about dependencies from the bundled version file. class DepsVersionInfo { final String version; @@ -534,18 +554,44 @@ class DependencyManager { final expectedSha256 = await _fetchExpectedSha256(expected.getManifestUrl(platformId)); - // Create temp file for download - final tempDir = - await TempDirectoryService.instance.createTemp('vapourbox_deps_'); - final tempFile = File(path.join(tempDir.path, filename)); + // The zip is downloaded into a stable cache path rather than a throwaway + // temp directory, and kept if anything after the download fails. Everything + // past this point — extraction, verification, the swap — can fail for + // reasons that have nothing to do with the bytes we just fetched (issue + // #87), and making the user re-fetch ~200 MB to retry a rename is a poor + // trade for the disk the zip occupies until the install succeeds. + final tempFile = File(await _cachedDownloadPath(filename)); try { - // Download with progress - await _downloadFile( - downloadUrl, - tempFile, - expectedSha256: expectedSha256, - ); + // Reuse the cached zip only when the sidecar gave us a hash to check it + // against. Without one an interrupted download is indistinguishable from + // a complete one, and extracting a truncated zip would report a corrupt + // bundle rather than the missing bytes. + var reusable = false; + if (await tempFile.exists()) { + if (expectedSha256 != null) { + _progressController.add(DownloadProgress( + bytesReceived: 0, + totalBytes: 0, + status: 'Checking the downloaded file...', + )); + reusable = await _sha256OfFile(tempFile) == expectedSha256; + print('DependencyManager: cached download ${reusable ? 'matches the ' + 'expected hash - skipping the download' : 'does not match the ' + 'expected hash - downloading again'}'); + } + if (!reusable) { + await tempFile.delete().catchError((_) => tempFile); + } + } + + if (!reusable) { + await _downloadFile( + downloadUrl, + tempFile, + expectedSha256: expectedSha256, + ); + } // Extract. _extractZip emits per-file extraction progress; this initial // event (0/0 -> indeterminate) covers the synchronous decode that precedes @@ -573,10 +619,30 @@ class DependencyManager { final staging = Directory('${depsDir.path}.new'); final retired = Directory('${depsDir.path}.old'); for (final d in [staging, retired]) { - if (await d.exists()) await d.delete(recursive: true); + // Retried: a leftover .new from a failed swap can still be held open by + // whatever blocked that swap, and an unguarded delete here would fail + // the next attempt with a second, different error. + if (await d.exists()) { + await retryTransientFsOperation(() => d.delete(recursive: true), + what: 'remove ${d.path}', onRetry: _reportInstallWait); + } } - await _extractZip(tempFile, targetOverride: staging); + // Staging only earns its cost when there is an install to protect. On a + // first install there is none, so extract straight into place and skip + // the swap entirely — which is what issue #87 needs: the rename is the + // fragile step, and on Windows it is refused outright (errno 5) while + // anything at all holds a handle on a file in the tree, which on that + // platform routinely means a scanner or the search indexer working + // through the ~200 MB we have just written. + // + // Safe to extract in place because version.json is still written last: + // an interrupted first install leaves a tree with no version file, which + // the next startup reads as missing rather than as installed. + final hadPrevious = await depsDir.exists(); + final target = hadPrevious ? staging : depsDir; + + await _extractZip(tempFile, targetOverride: target); // Prove the install is actually usable before declaring success. The // quarantine strip above can fail — silently, and it cannot succeed at all @@ -587,29 +653,83 @@ class DependencyManager { // // Checked against the staged copy, so a bundle that fails here is thrown // away with the existing install still in place. - final problem = await executabilityProblem(depsDirOverride: staging); - if (problem != null) { - await staging.delete(recursive: true).catchError((_) => staging); - throw Exception(problem); + // + // Windows runs it *after* the swap instead (below). The quarantine case + // this guards is macOS-only, so on Windows all it can report is a generic + // "would not run" — while executing a freshly written, unsigned 100 MB + // binary is exactly what makes a scanner open the tree we are about to + // rename. See the rollback below for how a genuine failure is handled. + if (!Platform.isWindows) { + _reportInstallStep('Checking the new components...'); + final problem = await executabilityProblem(depsDirOverride: target); + if (problem != null) { + await target.delete(recursive: true).catchError((_) => target); + throw DependencyInstallException(problem, remedy: ''); + } } // Write version file (per-platform version, so the next check matches). // Still the last thing written into the tree, so a staged directory that // never gets swapped in can never look complete. await _writeInstalledVersion(expected.versionFor(platformId), - depsDirOverride: staging); + depsDirOverride: target); // Swap. If the second rename fails we have already moved the old install // aside, so put it back rather than leaving the user with no deps at all. - final hadPrevious = await depsDir.exists(); - if (hadPrevious) await depsDir.rename(retired.path); - try { - await staging.rename(depsDir.path); - } catch (e) { - if (hadPrevious) { - await retired.rename(depsDir.path); + // + // Both renames are retried. A directory rename on Windows fails with + // "Access is denied" (errno 5) whenever any descendant is open — a + // transient condition, and one the user cannot do anything about, but it + // lands at the very end of a ~200 MB download, so a single attempt costs + // them the whole thing (issue #87). + if (hadPrevious) { + _reportInstallStep('Installing...'); + await retryTransientFsOperation(() => depsDir.rename(retired.path), + what: 'move the existing install aside', + onRetry: _reportInstallWait); + try { + await retryTransientFsOperation(() => staging.rename(depsDir.path), + what: 'move the new install into place', + onRetry: _reportInstallWait); + } catch (_) { + // Best-effort: if putting the old tree back also fails the user is + // left with no deps and re-downloads next launch, which is recoverable + // — reporting why the swap failed is the more useful error. + await retryTransientFsOperation(() => retired.rename(depsDir.path), + what: 'restore the previous install') + .catchError((_) => depsDir); + rethrow; + } + } + + // Windows' half of the executability check, against the live install. + // Restores the previous bundle if the new one will not run, so a blocked + // download can never leave the user worse off than before it. + if (Platform.isWindows) { + _reportInstallStep('Checking the new components...'); + final problem = await executabilityProblem(depsDirOverride: depsDir); + if (problem != null) { + // All best-effort: whatever happens to the trees, the error the user + // needs is why the new bundle would not run. + if (hadPrevious) { + try { + await retryTransientFsOperation( + () => depsDir.rename(staging.path), + what: 'move the failed install aside'); + await retryTransientFsOperation( + () => retired.rename(depsDir.path), + what: 'restore the previous install'); + unawaited( + staging.delete(recursive: true).catchError((_) => staging)); + } catch (e) { + print('DependencyManager: could not restore the previous ' + 'install after a failed one: $e'); + } + } else { + await depsDir.delete(recursive: true).catchError((_) => depsDir); + } + throw DependencyInstallException(problem, remedy: ''); } - rethrow; } // Best-effort: the install is already live, so failing to remove the old @@ -625,11 +745,162 @@ class DependencyManager { )); print('DependencyManager: Installation complete'); - } finally { - // Cleanup temp files + + // Only now is the zip dead weight. On any failure above it is deliberately + // left in place so Retry can skip the download. + await tempFile.delete().catchError((_) => tempFile); + } catch (e) { + print('DependencyManager: install failed ($e) - keeping the downloaded ' + 'zip at ${tempFile.path} so a retry can reuse it'); + rethrow; + } + } + + /// What to suggest the user try, given the error that ended the install. + /// + /// One fixed line of connection advice was actively misleading for anything + /// that failed after the download (issue #87), which is most of the install. + /// Errors carrying their own advice are honoured; everything else is + /// classified by what the filesystem actually said, and only a genuinely + /// unrecognised failure falls back to the connection line. + /// + /// Windows error codes are the discriminating ones and are not + /// interchangeable: **5** (access denied) and **32** (sharing violation) mean + /// something holds the files open, **112** means the disk is full, **183** + /// means the destination is already there. See + /// [retryTransientFsOperation] for how 5 was pinned down. + static String remedyFor(Object error) { + if (error is DependencyInstallException) return error.remedy; + + if (error is FileSystemException) { + final code = error.osError?.errorCode; + final noSpace = Platform.isWindows ? code == 112 : code == 28; + if (noSpace) { + return 'There is not enough free disk space to install the ' + 'components. Free some space and try again.'; + } + if (error is PathAccessException || code == 5 || code == 32) { + if (Platform.isWindows) { + return 'Another program is holding the downloaded files open, which ' + 'stops VapourBox from moving them into place. This is usually ' + 'antivirus or file indexing, and is not a permissions problem.\n\n' + 'Close any Explorer windows showing the VapourBox folder, then ' + 'try again. If it keeps happening, add the VapourBox folder to ' + 'your antivirus exclusions, or move VapourBox out of Downloads ' + 'and out of any synced folder (OneDrive, Dropbox).'; + } + return 'VapourBox could not write to its components folder. Check that ' + 'you have permission to write there and that the disk is not full, ' + 'then try again.'; + } + return 'VapourBox could not finish writing its components. Check the ' + 'free disk space and that the folder is writable, then try again.'; + } + + return 'Please check your internet connection and try again.'; + } + + /// Where the downloaded bundle is cached between attempts. + /// + /// A fixed name under the (user-configurable) temp directory, so a retry can + /// find it. Anything else in there is another version's leftovers and is + /// pruned, which is what stops the cache growing without bound. + Future _cachedDownloadPath(String filename) async { + final dir = Directory(path.join( + (await TempDirectoryService.instance.resolve()).path, + 'vapourbox-deps-cache')); + if (!await dir.exists()) await dir.create(recursive: true); + try { + await for (final entry in dir.list()) { + if (entry is File && path.basename(entry.path) != filename) { + await entry.delete().catchError((_) => entry); + } + } + } catch (e) { + print('DependencyManager: could not prune the download cache ($e)'); + } + return path.join(dir.path, filename); + } + + /// Lowercase hex sha256 of [file], in the same form the sidecar publishes. + Future _sha256OfFile(File file) async { + final digest = await sha256.bind(file.openRead()).first; + return digest.toString(); + } + + /// Progress event for a step of the install that has no measurable size. + /// + /// Emitted with 0/0 so the dialog shows an indeterminate bar: the swap really + /// has no fraction to report, and leaving the previous step's full bar on + /// screen made the install phase look finished when it had not started. + void _reportInstallStep(String status) { + _progressController.add( + DownloadProgress(bytesReceived: 0, totalBytes: 0, status: status)); + } + + /// Progress event for a retried filesystem step. + /// + /// Without this the retry budget is a silent stall on a dialog that still + /// reads "Extracting... 100%", which is indistinguishable from a hang — and + /// the machines that need the retries are the ones that wait longest. + void _reportInstallWait(int attempt, Object error) { + _reportInstallStep('Waiting for another program to release the new ' + 'files... (attempt $attempt)'); + } + + /// Run [operation], retrying while it fails for a reason that may clear on + /// its own. + /// + /// Exists for the install swap. Windows refuses to rename or delete a + /// directory while **any** descendant is open — measured: an open read handle + /// on one file, or a child process whose working directory is inside the + /// tree, is enough, and both surface as `PathAccessException … errno = 5`. + /// (A running .exe inside the tree is *not* enough, and a destination that + /// already exists gives errno 183 instead, so those two can be told apart + /// from a genuine permissions fault.) Immediately after extracting a ~200 MB + /// bundle there is usually something holding one — a scanner, the search + /// indexer, Explorer building a thumbnail — for a few hundred milliseconds. + /// + /// A single attempt therefore threw away the entire download, every time, for + /// the user who reported issue #87. Waiting a few seconds costs nothing on a + /// machine where the first attempt succeeds. + /// + /// `PathExistsException` and `PathNotFoundException` are not transient — the + /// destination will not stop existing, and a missing source will not appear — + /// so they are rethrown immediately rather than burning the whole budget. + /// [onRetry] is called with the attempt number that just failed and its + /// error, so the caller can tell the user something is being waited on + /// rather than leaving the dialog on a stalled bar. + @visibleForTesting + static Future retryTransientFsOperation( + Future Function() operation, { + int attempts = 12, + Duration firstDelay = const Duration(milliseconds: 50), + Duration maxDelay = const Duration(seconds: 1), + String? what, + void Function(int attempt, Object error)? onRetry, + }) async { + var delay = firstDelay; + for (var attempt = 1;; attempt++) { try { - await tempDir.delete(recursive: true); - } catch (_) {} + return await operation(); + } on PathExistsException { + rethrow; + } on PathNotFoundException { + rethrow; + } on FileSystemException catch (e) { + if (attempt >= attempts) { + print('DependencyManager: gave up after $attempt attempts to ' + '${what ?? 'complete a filesystem operation'}: $e'); + rethrow; + } + print('DependencyManager: attempt $attempt to ' + '${what ?? 'complete a filesystem operation'} failed ($e) - ' + 'retrying in ${delay.inMilliseconds}ms'); + onRetry?.call(attempt, e); + await Future.delayed(delay); + delay = delay * 2 > maxDelay ? maxDelay : delay * 2; + } } } diff --git a/app/lib/views/dependency_download_dialog.dart b/app/lib/views/dependency_download_dialog.dart index 5274741..7eaab99 100644 --- a/app/lib/views/dependency_download_dialog.dart +++ b/app/lib/views/dependency_download_dialog.dart @@ -39,6 +39,11 @@ class _DependencyDownloadDialogState extends State { bool _isDownloading = false; bool _hasError = false; String _errorMessage = ''; + + /// What to suggest, chosen from the failure rather than fixed. A rename + /// refused by a held file handle is not a network problem, and saying so sent + /// the reporter of issue #87 auditing folder permissions instead. + String _errorRemedy = ''; DownloadProgress? _progress; @override @@ -58,6 +63,11 @@ class _DependencyDownloadDialogState extends State { _isDownloading = true; _hasError = false; _errorMessage = ''; + _errorRemedy = ''; + // A retry starts from whatever the manager reports next, which may be the + // cached-zip check rather than a fresh download. Leaving the previous + // attempt's final bar up would claim progress this attempt has not made. + _progress = null; }); _progressSubscription = _manager.progressStream.listen( @@ -73,13 +83,14 @@ class _DependencyDownloadDialogState extends State { if (mounted) { Navigator.of(context).pop(true); } - }).catchError((error) { + }).catchError((Object error) { _progressSubscription?.cancel(); if (mounted) { setState(() { _isDownloading = false; _hasError = true; _errorMessage = error.toString(); + _errorRemedy = DependencyManager.remedyFor(error); }); } }); @@ -128,7 +139,10 @@ class _DependencyDownloadDialogState extends State { color: _hasError ? Colors.red : Theme.of(context).colorScheme.primary, ), const SizedBox(width: 12), - Text(_hasError ? 'Download Failed' : 'Installing Components'), + // Not "Download Failed": most of what can fail here happens after the + // download, and naming the wrong stage is what made issue #87 read as + // a network or permissions problem. + Text(_hasError ? 'Installation Failed' : 'Installing Components'), ], ), content: SizedBox( @@ -139,7 +153,7 @@ class _DependencyDownloadDialogState extends State { children: [ if (_hasError) ...[ Text( - 'Failed to download dependencies:', + 'VapourBox could not install its processing components:', style: TextStyle(color: Colors.red[700]), ), const SizedBox(height: 8), @@ -154,10 +168,10 @@ class _DependencyDownloadDialogState extends State { style: const TextStyle(fontFamily: 'monospace', fontSize: 12), ), ), - const SizedBox(height: 16), - const Text( - 'Please check your internet connection and try again.', - ), + if (_errorRemedy.isNotEmpty) ...[ + const SizedBox(height: 16), + Text(_errorRemedy), + ], ] else if (_isDownloading) ...[ Text(_getStatusMessage()), const SizedBox(height: 24), diff --git a/app/test/dependency_install_feedback_test.dart b/app/test/dependency_install_feedback_test.dart new file mode 100644 index 0000000..fa4bcfc --- /dev/null +++ b/app/test/dependency_install_feedback_test.dart @@ -0,0 +1,124 @@ +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/services/dependency_manager.dart'; + +/// What the install tells the user while it works and when it fails. +/// +/// The dialog used to print one fixed line — "Please check your internet +/// connection and try again" — under whatever went wrong, and to say nothing at +/// all between the last extraction tick and "Complete". Both were wrong for the +/// failure in issue #87: the download had finished, and the advice sent the +/// reporter auditing folder permissions on a problem that was neither. +void main() { + group('remedyFor', () { + // The reported failure, exactly: errno 5 renaming the staged tree. + final held = PathAccessException( + r'D:\VapourBox\deps\windows-x64.new', + const OSError('Access is denied', 5), + 'Rename failed', + ); + + test('a held file handle is not reported as a network problem', () { + final remedy = DependencyManager.remedyFor(held); + expect(remedy, isNot(contains('internet'))); + expect(remedy, isNotEmpty); + }); + + test('a held file handle names what actually holds it', () { + final remedy = DependencyManager.remedyFor(held).toLowerCase(); + if (Platform.isWindows) { + expect(remedy, contains('antivirus')); + // The owner's first guess, and the reporter's — worth saying outright. + expect(remedy, contains('not a permissions problem')); + } else { + expect(remedy, contains('permission')); + } + }); + + test('a full disk says so rather than blaming the connection', () { + final full = FileSystemException( + 'Cannot write file', + r'D:\VapourBox\deps\windows-x64.new', + OSError('There is not enough space on the disk', + Platform.isWindows ? 112 : 28), + ); + expect(DependencyManager.remedyFor(full), contains('disk space')); + }); + + test('an unrecognised failure still gets the connection advice', () { + expect( + DependencyManager.remedyFor(const SocketException('No route to host')), + contains('internet connection'), + ); + }); + + test('an error carrying its own advice is passed through verbatim', () { + // The macOS quarantine message already contains the xattr command; a + // second, generic suggestion under it would only muddy that. + expect( + DependencyManager.remedyFor( + DependencyInstallException('quarantined', remedy: '')), + isEmpty, + ); + expect( + DependencyManager.remedyFor( + DependencyInstallException('nope', remedy: 'do this instead')), + 'do this instead', + ); + }); + + test('the message is what the dialog shows, without an Exception prefix', + () { + expect( + DependencyInstallException('the bundled ffmpeg would not run', + remedy: '') + .toString(), + 'the bundled ffmpeg would not run', + ); + }); + }); + + group('retry reporting', () { + test('every failed attempt is reported, and a success is not', () async { + final reported = []; + var calls = 0; + + await DependencyManager.retryTransientFsOperation( + () async { + calls++; + if (calls < 4) { + throw const PathAccessException( + 'x', OSError('Access is denied.', 5), 'Rename failed'); + } + return null; + }, + attempts: 6, + firstDelay: const Duration(milliseconds: 1), + maxDelay: const Duration(milliseconds: 2), + onRetry: (attempt, _) => reported.add(attempt), + ); + + // Three failures, three notices — the fourth attempt succeeded and must + // not leave a "still waiting" message as the last thing on screen. + expect(reported, [1, 2, 3]); + }); + + test('the attempt that exhausts the budget is not reported as a wait', + () async { + final reported = []; + await expectLater( + DependencyManager.retryTransientFsOperation( + () async => throw const PathAccessException( + 'x', OSError('Access is denied.', 5), 'Rename failed'), + attempts: 3, + firstDelay: const Duration(milliseconds: 1), + onRetry: (attempt, _) => reported.add(attempt), + ), + throwsA(isA()), + ); + // The last failure becomes the error, not another "waiting" message. + expect(reported, [1, 2]); + }); + }); +} diff --git a/app/test/dependency_install_retry_test.dart b/app/test/dependency_install_retry_test.dart new file mode 100644 index 0000000..d2fa2a1 --- /dev/null +++ b/app/test/dependency_install_retry_test.dart @@ -0,0 +1,165 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/services/dependency_manager.dart'; + +/// Guards the install swap's tolerance of a transient filesystem failure +/// (issue #87). +/// +/// The reported symptom was `PathAccessException: Rename failed … (OS Error: +/// Access is denied, errno = 5)` on `deps\windows-x64.new`, after a complete +/// download and extraction — the very last step of the install. It reads like a +/// permissions problem and is not one: on Windows a directory rename is refused +/// while anything holds a handle on a descendant, which straight after writing +/// a ~200 MB bundle is routine and momentary. +void main() { + group('retryTransientFsOperation', () { + test('retries a transient failure and returns the eventual result', + () async { + var calls = 0; + final result = await DependencyManager.retryTransientFsOperation( + () async { + calls++; + if (calls < 3) { + throw const PathAccessException( + 'x', OSError('Access is denied.', 5), 'Rename failed'); + } + return 'swapped'; + }, + attempts: 5, + firstDelay: const Duration(milliseconds: 1), + maxDelay: const Duration(milliseconds: 2), + ); + + expect(result, 'swapped'); + expect(calls, 3); + }); + + test('gives up after the attempt budget and rethrows the last error', + () async { + var calls = 0; + await expectLater( + DependencyManager.retryTransientFsOperation( + () async { + calls++; + throw const PathAccessException( + 'x', OSError('Access is denied.', 5), 'Rename failed'); + }, + attempts: 4, + firstDelay: const Duration(milliseconds: 1), + maxDelay: const Duration(milliseconds: 2), + ), + throwsA(isA()), + ); + expect(calls, 4); + }); + + // errno 183 rather than 5: the destination is already there and waiting + // will not change that. Retrying it would spend the whole budget before + // reporting a fault the user can act on. + test('does not retry a destination that already exists', () async { + var calls = 0; + await expectLater( + DependencyManager.retryTransientFsOperation( + () async { + calls++; + throw const PathExistsException('x', + OSError('Cannot create a file when that file already exists.', 183)); + }, + attempts: 6, + firstDelay: const Duration(milliseconds: 1), + ), + throwsA(isA()), + ); + expect(calls, 1); + }); + + test('does not retry a missing source', () async { + var calls = 0; + await expectLater( + DependencyManager.retryTransientFsOperation( + () async { + calls++; + throw const PathNotFoundException( + 'x', OSError('The system cannot find the file specified.', 2)); + }, + attempts: 6, + firstDelay: const Duration(milliseconds: 1), + ), + throwsA(isA()), + ); + expect(calls, 1); + }); + + test('passes a non-filesystem error straight through', () async { + var calls = 0; + await expectLater( + DependencyManager.retryTransientFsOperation( + () async { + calls++; + throw StateError('not a filesystem problem'); + }, + attempts: 6, + firstDelay: const Duration(milliseconds: 1), + ), + throwsA(isA()), + ); + expect(calls, 1); + }); + }); + + // The real thing, on the platform that actually behaves this way. Skipped + // elsewhere: POSIX renames a directory happily with its files open, so there + // is nothing to reproduce. + group('Windows directory rename with an open descendant', () { + late Directory tmp; + late Directory staging; + late File held; + + setUp(() async { + tmp = await Directory.systemTemp.createTemp('vb_swap_test_'); + staging = Directory('${tmp.path}/deps.new'); + await Directory('${staging.path}/ffmpeg').create(recursive: true); + held = File('${staging.path}/ffmpeg/ffmpeg.exe'); + await held.writeAsString('not really an executable'); + }); + + tearDown(() async { + await tmp.delete(recursive: true).catchError((_) => tmp); + }); + + test('a single attempt fails with access denied, not a permissions fault', + () async { + final handle = await held.open(); + try { + await expectLater( + staging.rename('${tmp.path}/deps'), + throwsA(isA() + .having((e) => e.osError?.errorCode, 'errno', 5)), + ); + } finally { + await handle.close(); + } + }); + + test('the retry rides out a handle that is released a moment later', + () async { + final handle = await held.open(); + // Whatever holds the handle — scanner, indexer, Explorer — lets go on its + // own schedule, not ours. + unawaited(Future.delayed(const Duration(milliseconds: 250)) + .then((_) => handle.close())); + + final moved = await DependencyManager.retryTransientFsOperation( + () => staging.rename('${tmp.path}/deps'), + what: 'move the new install into place', + ); + + expect(await moved.exists(), isTrue); + expect(await staging.exists(), isFalse); + expect( + await File('${tmp.path}/deps/ffmpeg/ffmpeg.exe').exists(), isTrue); + }); + }, skip: !Platform.isWindows); +}