fix: harden archive install with staging directory and network resilience#1013
fix: harden archive install with staging directory and network resilience#1013leoafarias wants to merge 38 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Adds --archive flag to install and use commands, enabling faster installations from pre-compiled archives instead of git clones. Key features: - Downloads Flutter SDK from archives (2-3x faster than git) - SHA256 checksum verification for security - Supports stable, beta, and dev channels - Works with FLUTTER_STORAGE_BASE_URL for corporate mirrors - Cross-platform extraction (zip/tar.xz) Implementation: - New ArchiveService handles download, verification, and extraction - Commands accept --archive flag - EnsureCacheWorkflow passes flag through to FlutterService - Validates unsupported scenarios (forks, commits, custom versions) Testing: - Unit tests for archive installation flow - Tests for unsupported version validation - Mock FlutterService for test isolation Resolves #688
- Fix content-length check in archive_service.dart (use != -1 instead of > 0) - Fix undefined log_message function in uninstall.sh - Remove plan.md development artifact (842 lines)
- Add cleanupAndRethrow() helper in _downloadArchive() to eliminate duplicate cleanup pattern across three catch blocks - Create comprehensive test suite (25 tests) for ArchiveService covering version validation, SHA256 checksum, directory handling, archive format detection, and FlutterSdkRelease model properties
The FileLocker mechanism was intentionally removed in d694ec5, but references were accidentally reintroduced in ff03ba6 when adding bare git cache handling. This removes the orphaned imports while preserving the valuable bare repository detection logic. Changes: - Remove import of non-existent file_lock.dart - Remove FileLocker field and createLock usage - Remove lock acquisition/release from updateLocalMirror - Keep all bare repo detection and handling logic intact
…ence - Use staging directory pattern to protect existing cached SDKs from data loss when archive download, checksum, or extraction fails - Add HTTP connection timeout (30s) to prevent stalled downloads - Add TLS HandshakeException handling with guidance for corporate mirrors - Add 12 new tests: safe install behavior, structure flattening, extraction validation, @dev qualifier, platform extraction, network errors, and archive URL construction - Fix MockFlutterService to validate archive version types
leoafarias
force-pushed
the
issue-688-relink
branch
from
d4d0cb1 to
69f3e08
Compare
leoafarias
force-pushed
the
issue-688-relink
branch
from
d5184c3 to
891c044
Compare
There was a problem hiding this comment.
Pull request overview
Hardens the archive-based Flutter SDK installation path to be more failure-tolerant (staging installs, better network behavior/error guidance) and expands regression coverage to ensure the --archive flag propagates through workflows and CI.
Changes:
- Add
ArchiveServicewith staging-directory installs, checksum verification, extraction, and improved network/TLS error handling. - Propagate
useArchivethroughEnsureCacheWorkflow,install, andusecommands; disable git-cache behavior when archive mode is enabled. - Add extensive archive regression/unit test coverage and a CI “archive-regression” job that runs a scripted local regression agent and uploads artifacts.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testing_utils.dart | Extends MockFlutterService.install to capture and simulate archive installs in tests. |
| test/src/workflows/ensure_cache_ci_test.dart | Adds coverage ensuring useArchive propagates through corrupted-cache reinstalls and workflow calls. |
| test/services/git_service_test.dart | Adds tests asserting correct git invocation for bare vs non-bare git-cache repos. |
| test/services/archive_service_test.dart | Adds comprehensive tests for archive install validation, download/extract failures, staging safety, and network errors. |
| test/commands/use_command_test.dart | Adds fvm use --archive behavioral tests (success + unsupported version cases). |
| test/commands/install_command_test.dart | Adds fvm install --archive behavioral tests (success + unsupported version cases). |
| scripts/local_regression_agent.sh | Introduces a local regression “agent” script used both locally and in CI for deterministic archive-focused checks. |
| pubspec.yaml | Moves crypto to main dependencies for runtime archive checksum verification. |
| pubspec.lock | Reflects crypto dependency change from dev to main. |
| lib/src/workflows/ensure_cache.workflow.dart | Adds useArchive parameter propagation; skips git validations and git-cache updates in archive mode. |
| lib/src/utils/context.dart | Registers ArchiveService in default DI generators. |
| lib/src/services/git_service.dart | Updates local mirror update logic to support bare repos (--git-dir) and adds stronger “recreate mirror” paths. |
| lib/src/services/flutter_service.dart | Adds useArchive install path, delegating to ArchiveService. |
| lib/src/services/archive_service.dart | New service implementing resilient archive download/verify/extract with staging/finalize logic and better network/TLS errors. |
| lib/src/commands/use_command.dart | Adds --archive flag and forwards it to EnsureCacheWorkflow. |
| lib/src/commands/install_command.dart | Adds --archive flag and forwards it to EnsureCacheWorkflow. |
| agents/local-regression-testing-agent.md | Documents the regression agent workflow, stages, and reporting contract. |
| .github/workflows/test.yml | Adds archive-regression CI job; conditionally uploads coverage only when Codecov token is present; switches Dart version for release tool steps. |
| .github/workflows/README.md | Documents the new archive-regression job in CI. |
| .fvmrc | Adds repository-level FVM config for local/dev usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…omments Merge open+acquire into a single _acquireCacheMutationLock that returns the handle and cleans up on failure, removing the separate open helper and the lockAcquired tracking flag. Remove redundant comments in ForkCommand.
…aults - Filter archive operation directories (.archive_staging, .archive_backup, .archive_lock) from cache version listing to prevent transient dirs from appearing as installed SDKs - Reject Windows drive paths in git URL validation - Simplify gitCache logic: mirror is always disabled on CI regardless of explicit config, matching established CI install flow - Remove fork-to-git-reference fallback for unknown refs with slashes; unconfigured fork aliases now consistently error
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Whether to use the local git mirror cache. | ||
| /// | ||
| /// Explicit config/ENV opt-in/opt-out is always honoured. | ||
| /// Default: enabled locally, disabled on CI. | ||
| /// The mirror stays disabled on CI to match the established install flow. | ||
| @MappableField() | ||
| bool get gitCache { | ||
| final explicit = config.useGitCache; | ||
| if (explicit != null) return explicit; | ||
| return !isCI; | ||
| final useGitCache = config.useGitCache ?? true; | ||
|
|
||
| return useGitCache && !isCI; | ||
| } |
There was a problem hiding this comment.
gitCache now forces the mirror off on CI even when config.useGitCache == true. That’s a behavioral change from the previous “explicit opt-in is always honoured” contract (and can affect CI users relying on an enabled mirror). If this is intentional, consider updating the in-code doc comment and user-facing documentation/CHANGELOG so the override-on-CI behavior is clear.
| Never cleanupAndRethrow(Object error, StackTrace stackTrace) { | ||
| progress.fail('Failed to download Flutter SDK archive'); | ||
| try { | ||
| tempDir.deleteSync(recursive: true); | ||
| } catch (_) { | ||
| // Best-effort cleanup; don't mask the original error | ||
| } |
There was a problem hiding this comment.
In _downloadArchive, cleanupAndRethrow deletes tempDir synchronously. When it’s invoked from the read-timeout handler, the archive file sink may still be open (the finally that closes sink runs after the throw), so deletion can fail (notably on Windows) and leak fvm_archive_* temp directories. Consider closing/cancelling the sink/response before attempting deletion, and/or performing tempDir cleanup after the sink is closed (e.g., in the surrounding finally) using deleteDirectoryWithRetry for best-effort cleanup.
|
|
||
| if (version.isUnknownRef) { | ||
| throw const AppException( | ||
| 'Archive installation is not supported for commit references. ' |
There was a problem hiding this comment.
validateArchiveInstallVersion rejects version.isUnknownRef but the error message says “commit references”. FlutterVersion.parse classifies any non-semver ref (e.g., branch names like feature/foo, tags, commits) as unknownRef, so this message can mislead users. Suggest updating the wording to “git references (branches/tags/commits)” (or similar) to match what’s actually rejected.
| 'Archive installation is not supported for commit references. ' | |
| 'Archive installation is not supported for git references ' | |
| '(branches, tags, or commits). ' |
| List<String> _listArchiveTempDirs() { | ||
| return Directory.systemTemp | ||
| .listSync() | ||
| .whereType<Directory>() | ||
| .where((dir) => path.basename(dir.path).startsWith('fvm_archive_')) | ||
| .map((dir) => dir.path) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
_listArchiveTempDirs() scans the global system temp directory for fvm_archive_* folders and several tests assert that the set difference is empty. With concurrent test execution (especially across files), another test/process could create/delete a matching temp dir during this window and make these assertions flaky. Prefer tracking the specific temp directory created for the install under test (e.g., by injecting a temp-dir factory into ArchiveService for tests) instead of asserting against global Directory.systemTemp state.
| if (fork == null) { | ||
| // For unknownRef types (e.g., feature/my-branch), the slash is likely | ||
| // part of a git branch name, not a fork alias. Treat the whole input | ||
| // as a git reference. | ||
| if (flutterVersion.isUnknownRef) { | ||
| logger.debug( | ||
| 'No fork alias "${flutterVersion.fork}" configured, ' | ||
| 'treating "$version" as git reference', | ||
| ); | ||
|
|
||
| return FlutterVersion.gitReference(version); | ||
| } | ||
|
|
||
| // For channel/release forms (e.g., myfork/stable, myfork/3.24.0), | ||
| // the user clearly intended a configured fork alias. | ||
| throw AppDetailedException( | ||
| 'Fork "${flutterVersion.fork}" has not been configured', | ||
| 'Add the fork to your configuration first: fvm config', |
There was a problem hiding this comment.
This change makes unconfigured fork/... inputs always error, removing the prior fallback that treated unknownRef forms like feature/my-branch as a git reference when the fork alias isn’t configured. That’s a user-facing behavior change (and can break installs/uses of branch names containing /) but it isn’t mentioned in the PR description. If intentional, please update the PR description/changelog (or add a targeted error message) to make the breaking change explicit; otherwise consider restoring the fallback for isUnknownRef inputs.
| try { | ||
| await super.install(version, useArchive: useArchive); | ||
| final cacheService = get<CacheService>(); | ||
| lastInstallDirectory = cacheService.getVersionCacheDir(version); | ||
| } finally {} | ||
| } | ||
| } |
There was a problem hiding this comment.
The try { ... } finally {} block here has an empty finally clause, which is redundant and makes the intent unclear. Consider removing the try/finally entirely, or using try { ... } catch { ... } / finally only if there’s cleanup to perform.
| try { | |
| await super.install(version, useArchive: useArchive); | |
| final cacheService = get<CacheService>(); | |
| lastInstallDirectory = cacheService.getVersionCacheDir(version); | |
| } finally {} | |
| } | |
| } | |
| await super.install(version, useArchive: useArchive); | |
| final cacheService = get<CacheService>(); | |
| lastInstallDirectory = cacheService.getVersionCacheDir(version); | |
| } | |
| } | |
| } | |
| } |
test-setup now runs with FVM_USE_GIT_CACHE=true so it creates a bare mirror during setup. The mirror is then cloned locally to the path tests expect (~/fvm_test_cache/gitcache), so integration-style tests that call the real FlutterService.install() clone from local mirror instead of GitHub.
…ndant tests The pre-seed git cache fix added ~5 min of mirror creation overhead, making CI slower. Revert it and bump test-os timeout from 30 to 45 minutes to handle normal Windows/macOS CI variance. Remove 6 command-level archive validation tests that duplicate unit tests in archive_service_test.dart (same static method, same errors).
_targetDirFromInvocation now parses -DestinationPath from PowerShell Expand-Archive commands, not just Unix -d/-C flags. Also relax extraction failure error assertion to be platform-agnostic.
20-minute timeouts were too tight — all three jobs timed out during test/analysis steps because setup (checkout, SDK install, pub get, git cache creation) consumes significant time before tests even start.
This PR mirrors #966 on a fresh head branch so tooling can re-associate the PR correctly.\n\nIncludes:\n- Staging-directory archive install to prevent cache loss on failures\n- HTTP connection timeout for archive downloads\n- Improved TLS handshake error guidance\n- Expanded archive service test coverage