From f6d8fd404d7439022044a2b11d9ec9b751624378 Mon Sep 17 00:00:00 2001 From: Manfred Endres Date: Thu, 5 Mar 2026 07:49:54 +0100 Subject: [PATCH] Fix modules.json installation after editor failure ## Description Improve error handling in the module installation pipeline to properly distinguish between Editor and module failures, and clean up partial installations when the Editor itself fails during a fresh install. ## Changes * [ADD] `EditorInstallationFailed` and `ModuleInstallationsFailed` variants to `InstallError` * [FIX] Clean up installation directory when Editor installation fails on fresh install * [FIX] Return early on Editor failure without attempting module installs * [UPDATE] `install_modules_with_installer()` and `install_module_and_dependencies()` return `Result<()>` instead of `Vec` * [ADD] Tests covering editor failure cleanup, module failure collection, and success scenarios * [ADD] `editor-failure-cleanup` spec * [UPDATE] `incremental-modules-sync` spec with Editor failure exception clause --- .../.openspec.yaml | 2 + .../design.md | 267 ++++++++++++++++++ .../proposal.md | 30 ++ .../specs/editor-failure-cleanup/spec.md | 79 ++++++ .../specs/incremental-modules-sync/spec.md | 35 +++ .../tasks.md | 69 +++++ openspec/specs/editor-failure-cleanup/spec.md | 85 ++++++ .../specs/incremental-modules-sync/spec.md | 17 +- uvm_install/src/error.rs | 25 ++ uvm_install/src/lib.rs | 186 ++++++++++-- 10 files changed, 772 insertions(+), 23 deletions(-) create mode 100644 openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/.openspec.yaml create mode 100644 openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/design.md create mode 100644 openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/proposal.md create mode 100644 openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/editor-failure-cleanup/spec.md create mode 100644 openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/incremental-modules-sync/spec.md create mode 100644 openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/tasks.md create mode 100644 openspec/specs/editor-failure-cleanup/spec.md diff --git a/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/.openspec.yaml b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/.openspec.yaml new file mode 100644 index 00000000..5aae5cfa --- /dev/null +++ b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-04 diff --git a/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/design.md b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/design.md new file mode 100644 index 00000000..c4068446 --- /dev/null +++ b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/design.md @@ -0,0 +1,267 @@ +## Context + +The current installation flow in `uvm_install/src/lib.rs` iterates through components in topological order (Editor first, then modules) and continues past failures, collecting errors for final reporting. This resilient behavior is valuable when adding modules to an existing Editor, but problematic for fresh installs: if the Editor fails, modules install anyway, leaving a broken partial installation that breaks version detection and requires manual cleanup. + +The recent change (commit 6d4c7b5) to write `modules.json` incrementally after each module exacerbated this issue by persisting state even when the Editor never successfully installed. + +**Current flow:** +1. Create base directory (`/path/to/Unity-X.Y.Z/`) +2. Check if installation exists, mark installed components in graph +3. Keep only requested components in graph (line 348: `graph.keep(&all_components)`) +4. Install loop: iterate graph in topological order, continue on failures +5. Write final `modules.json` +6. Create `UnityInstallation` (reads `Info.plist` - fails if Editor missing) + +**Key insight:** The graph already encodes whether Editor installation is needed: +- **Fresh install**: Editor not installed → "Unity" is in the graph → will be attempted +- **Adding modules**: Editor already installed → "Unity" marked installed (line 255), filtered by `keep()` → not in graph + +## Goals / Non-Goals + +**Goals:** +- Fail-fast when Editor installation fails (instead of collecting error and continuing) +- Cleanup entire installation directory when Editor installation fails +- Preserve resilient module installation behavior (continue on module failures) +- Prevent broken partial installations from appearing in `uvm list` + +**Non-Goals:** +- Detecting whether Editor exists before installation (graph already knows) +- Changing the topological order of installation +- Rolling back individual module installations +- Changing the incremental `modules.json` writing behavior +- Adding complex state tracking (let the graph tell us what's needed) + +## Decisions + +### Decision 1: Fail-fast on Editor installation failure + +**Decision:** In the installation loop (`install_modules_with_installer()` at line 555), check if the failed component is the Editor (`module_id == "Unity"`). If so, immediately cleanup and return error instead of collecting and continuing. + +**Rationale:** +- If "Unity" is being installed, it means it's required (not already installed) +- Topological order guarantees Editor installs first, so failing here prevents module installations +- No need to track additional state - the graph already tells us if Editor is needed +- Simple check: `if module_id == "Unity" && install_result.is_err()` + +**Alternatives considered:** +- Add boolean flag to track "fresh install" → unnecessary, graph already knows +- Check in the caller → misses opportunity to skip module installations +- Continue and check at the end → too late, modules already installed + +**Implementation approach:** +```rust +// In install_modules_with_installer, line ~546 +fn install_modules_with_installer<'a, P: AsRef, I: ModuleInstaller>( + graph: &'a InstallGraph<'a>, + base_dir: P, + modules: &mut Vec, + installer: &I, +) -> Result<()> { // Changed from Vec + let base_dir = base_dir.as_ref(); + let mut errors = Vec::new(); + + for node in graph.topo().iter(graph.context()) { + if let Some(InstallStatus::Missing) = graph.install_status(node) { + let component = graph.component(node).unwrap(); + let module_id = match component { + UnityComponent::Editor(_) => "Unity".to_string(), + UnityComponent::Module(m) => m.id().to_string(), + }; + + info!("install {}", module_id); + let install_result = installer.install_module(&module_id, base_dir); + + match install_result { + Err(err) if module_id == "Unity" => { + // Editor installation failed - cleanup and abort + log::error!("Editor installation failed, cleaning up"); + if base_dir.exists() { + if let Err(cleanup_err) = std::fs::remove_dir_all(base_dir) { + log::warn!("Cleanup failed: {}", cleanup_err); + } + } + return Err(InstallError::EditorInstallationFailed(Box::new(err))); + } + Err(err) => { + // Module failure - collect and continue + log::warn!("Failed to install module {}: {}", module_id, err); + errors.push(err); + } + Ok(()) => { + // Success - mark installed + if let Some(m) = modules.iter_mut().find(|m| m.id() == module_id) { + m.is_installed = true; + trace!("module {} installed successfully", module_id); + } + } + } + + // Write modules.json after each attempt (unless we returned early) + write_modules_json(base_dir, modules); + } + } + + // Return appropriate result + if errors.is_empty() { + Ok(()) + } else { + Err(InstallError::ModuleInstallationsFailed(errors)) + } +} +``` + +### Decision 2: Perform cleanup inline in the installer loop + +**Decision:** Cleanup the installation directory immediately when Editor fails, inside `install_modules_with_installer()`. + +**Rationale:** +- Simpler control flow - single location for error handling +- Prevents any subsequent module installations from running +- No need to signal "cleanup needed" to caller +- Cleanup failure is non-fatal (best-effort) + +**Alternatives considered:** +- Cleanup in caller → requires returning signal, more complex +- Create separate cleanup function → overengineered for `fs::remove_dir_all()` +- Don't cleanup on error → leaves broken partial installation (current problem) + +**Error handling:** If cleanup fails, log warning but still return the Editor installation error. The cleanup failure is secondary to the primary error (Editor didn't install). + +### Decision 3: Use Result<()> return type with semantic error variants + +**Decision:** Change `install_modules_with_installer()` signature from returning `Vec` to returning `Result<()>`, using error variants to distinguish Editor vs. module failures. + +**Rationale:** +- Error type encodes what happened, not the caller's responsibility to interpret +- `EditorInstallationFailed` = cleanup happened, Editor was required and failed +- `ModuleInstallationsFailed` = Editor succeeded (or not needed), one or more modules failed +- No vec scanning needed - the type tells you everything +- Idiomatic Rust: `Result<()>` for operations that succeed or fail + +**Type change:** +```rust +// Before +fn install_modules_with_installer(...) -> Vec + +// After +fn install_modules_with_installer(...) -> Result<()> +``` + +**Control flow at end of install loop:** +```rust +// After the loop completes +if errors.is_empty() { + Ok(()) +} else { + Err(InstallError::ModuleInstallationsFailed(errors)) +} +``` + +**Caller adjustment** (line 365): +```rust +// Before +let errors = install_module_and_dependencies(&graph, &base_dir, &mut modules); +if !errors.is_empty() { + // ... check and return MultipleInstallFailures +} + +// After +install_module_and_dependencies(&graph, &base_dir, &mut modules)?; +// Done - error handling is in the error type itself +``` + +### Decision 4: Add semantic error variants for Editor and module failures + +**Decision:** Add two error variants to distinguish failure types: +- `InstallError::EditorInstallationFailed(Box)` - Editor failed, cleanup happened +- `InstallError::ModuleInstallationsFailed(Vec)` - One or more modules failed + +**Rationale:** +- Error type itself communicates what failed and what action was taken +- Caller doesn't need to inspect error contents to know if cleanup happened +- Clear contract: EditorInstallationFailed means cleanup already done +- ModuleInstallationsFailed can contain multiple module errors (preserves current behavior) +- Replaces the current `MultipleInstallFailures` pattern with more semantic variants + +**Implementation in `uvm_install/src/error.rs`:** +```rust +#[derive(Debug)] +pub enum InstallError { + // ... existing variants + EditorInstallationFailed(Box), + ModuleInstallationsFailed(Vec), +} + +impl Display for InstallError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + // ... existing arms + Self::EditorInstallationFailed(err) => { + write!(f, "Unity Editor installation failed: {}", err) + } + Self::ModuleInstallationsFailed(errs) => { + write!(f, "Module installation failed ({} errors)", errs.len()) + } + } + } +} +``` + +### Decision 5: Don't write modules.json after Editor failure + +**Decision:** Only write `modules.json` after successful or module-only failures, not after Editor failure with cleanup. + +**Rationale:** +- If we're removing the entire directory, no point writing modules.json +- Cleanup will delete it anyway +- Avoids momentary inconsistent state + +**Implementation:** Move `write_modules_json()` call to after the match expression, so it only runs if we didn't return early. + +```rust +match install_result { + Err(err) if module_id == "Unity" => { + // cleanup and return - DON'T write modules.json + ... + return Err(...); + } + Err(err) => { errors.push(err); } + Ok(()) => { /* mark installed */ } +} + +// Only reached if we didn't return early +write_modules_json(base_dir, modules); +``` + +## Risks / Trade-offs + +**[Risk] Cleanup might fail due to file locks or permissions** +→ **Mitigation:** Best-effort cleanup - log warning if it fails, but still return the Editor error. User will need manual cleanup, but at least they'll know Editor failed (better than current state). + +**[Trade-off] Breaking behavior change: installations now abort on Editor failure** +→ This is intentional and improves UX. Current behavior leaves broken partial installations. + +**[Risk] Architecture mismatch reinstall path might be affected** +→ **Mitigation:** Architecture mismatch code (line 258-282) already deletes the directory and marks all as missing. If Editor reinstall fails, cleanup is appropriate behavior. + +**[Risk] Cleanup removes base_dir even if user specified custom destination** +→ This is correct behavior - if user specified a destination and Editor failed, that destination should be cleaned up. + +## Migration Plan + +**Deployment:** +1. No database or external system changes required +2. Change is self-contained within `uvm_install` crate +3. Existing installations are unaffected (only impacts new installation attempts) +4. Error messages will be clearer (explicit "Editor installation failed") + +**Rollback:** +- Revert to previous behavior where all errors are collected +- No data migration needed + +**Testing approach:** +- Extend existing `MockModuleInstaller` tests to simulate Editor failure +- Test: Editor fails → cleanup triggered, modules not attempted +- Test: Module fails → continue with other modules, no cleanup +- Test: Cleanup failure doesn't mask Editor error +- Manual testing with network failures during Editor download diff --git a/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/proposal.md b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/proposal.md new file mode 100644 index 00000000..ef2eccb0 --- /dev/null +++ b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/proposal.md @@ -0,0 +1,30 @@ +## Why + +When the Unity Editor installation fails during a fresh install, the current implementation continues installing modules and writes `modules.json`, leaving a broken partial installation on disk. This partial state causes version detection to fail (can't read `Info.plist` or find Unity executable), breaks `uvm list`, and leaves orphaned module files without a working Editor. Users must manually clean up these broken installations. + +## What Changes + +- Add detection to distinguish "fresh install" (Editor required) from "adding modules" (Editor already exists) +- When Editor installation fails during a fresh install, immediately cleanup the installation directory and abort +- Preserve current behavior for module-only installations: continue past module failures and collect errors +- Ensure no partial installations are left on disk when Editor installation fails + +## Capabilities + +### New Capabilities +- `editor-failure-cleanup`: Handle Editor installation failures differently from module failures, with cleanup for fresh installs + +### Modified Capabilities +- `incremental-modules-sync`: Modify the installation flow to check Editor existence before installing and cleanup on Editor failure + +## Impact + +**Affected code:** +- `uvm_install/src/lib.rs`: Main installation flow, needs to track whether Editor pre-existed +- `uvm_install/src/lib.rs` (`install_module_and_dependencies`, `install_modules_with_installer`): Add Editor failure detection and cleanup logic +- `uvm_install/src/error.rs`: May need new error variant for "Editor installation required but failed" + +**Behavior changes:** +- Fresh installations will abort and cleanup if Editor fails (breaking change in behavior, but better UX) +- Module-only installations continue with current resilient behavior +- No more broken partial installations left on disk diff --git a/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/editor-failure-cleanup/spec.md b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/editor-failure-cleanup/spec.md new file mode 100644 index 00000000..ba777d58 --- /dev/null +++ b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/editor-failure-cleanup/spec.md @@ -0,0 +1,79 @@ +## ADDED Requirements + +### Requirement: System detects whether Editor pre-exists before installation + +The system SHALL detect whether a Unity Editor installation already exists at the target path before beginning the installation process. + +#### Scenario: Fresh install detected when directory is empty + +- **WHEN** the installation begins +- **AND** the target installation directory does not exist +- **THEN** the system records this as a "fresh install requiring Editor" + +#### Scenario: Fresh install detected when directory exists but Editor is missing + +- **WHEN** the installation begins +- **AND** the target installation directory exists but does not contain a valid Unity Editor (no `Unity.app/Contents/Info.plist` or equivalent) +- **THEN** the system records this as a "fresh install requiring Editor" + +#### Scenario: Existing Editor detected + +- **WHEN** the installation begins +- **AND** the target installation directory contains a valid Unity Editor installation +- **THEN** the system records this as "adding modules to existing Editor" + +### Requirement: Editor failure triggers cleanup for fresh installs + +The system SHALL remove the entire installation directory when Editor installation fails during a fresh install. + +#### Scenario: Editor installation fails on fresh install + +- **WHEN** the system detects a fresh install requiring Editor +- **AND** the Editor installation fails +- **THEN** the system removes the entire installation directory +- **AND** the system returns an error indicating Editor installation failed +- **AND** the system does NOT attempt to install any modules + +#### Scenario: Editor failure leaves no partial state + +- **WHEN** the Editor installation fails during a fresh install +- **AND** the installation directory is cleaned up +- **THEN** running `uvm list` does NOT show the failed installation +- **AND** no `modules.json` file exists at the target path + +### Requirement: Editor failure does not affect module-only installations + +The system SHALL NOT cleanup or abort when Editor already exists and only modules are being added, even if a module fails. + +#### Scenario: Module fails when adding to existing Editor + +- **WHEN** the system detects an existing Editor installation +- **AND** the user is installing additional modules +- **AND** one module installation fails +- **THEN** the system continues installing remaining modules +- **AND** the system does NOT remove the installation directory +- **AND** the system collects and reports the module failure + +#### Scenario: All modules fail when adding to existing Editor + +- **WHEN** the system detects an existing Editor installation +- **AND** all module installations fail +- **THEN** the installation directory remains intact +- **AND** the existing Editor remains functional +- **AND** the system reports all module failures + +### Requirement: Cleanup removes all partial installation artifacts + +The system SHALL remove all files and directories created during the failed installation when cleaning up. + +#### Scenario: Cleanup removes base directory + +- **WHEN** Editor installation fails and triggers cleanup +- **THEN** the system removes the entire base installation directory +- **AND** no subdirectories remain (e.g., no `PlaybackEngines/` fragments) + +#### Scenario: Cleanup removes modules.json if written + +- **WHEN** Editor installation fails after `modules.json` has been written +- **AND** cleanup is triggered +- **THEN** the system removes `modules.json` along with the installation directory diff --git a/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/incremental-modules-sync/spec.md b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/incremental-modules-sync/spec.md new file mode 100644 index 00000000..fbef5138 --- /dev/null +++ b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/specs/incremental-modules-sync/spec.md @@ -0,0 +1,35 @@ +## MODIFIED Requirements + +### Requirement: Installation continues despite individual module failures + +The system SHALL continue installing remaining modules when one module fails, collecting all errors for final reporting, UNLESS the failed component is the Editor during a fresh install requiring the Editor. + +#### Scenario: Multiple modules with one failure + +- **WHEN** the user installs modules A, B, and C +- **AND** module B fails during installation +- **THEN** the system installs module A successfully +- **AND** the system attempts module B and records the failure +- **AND** the system continues to install module C +- **AND** the system reports module B's failure at the end + +#### Scenario: All errors reported at completion + +- **WHEN** multiple modules fail during installation +- **THEN** the system reports all failures after attempting all modules +- **AND** the return value indicates installation had failures + +#### Scenario: Editor failure stops installation during fresh install + +- **WHEN** the system is performing a fresh install requiring the Editor +- **AND** the Editor installation fails +- **THEN** the system does NOT attempt to install any modules +- **AND** the system triggers cleanup of the installation directory +- **AND** the system returns an error indicating Editor installation failed + +#### Scenario: Editor failure does not stop module installation for existing Editors + +- **WHEN** the system is adding modules to an existing Editor installation +- **AND** the Editor installation was attempted (e.g., architecture mismatch reinstall) and fails +- **THEN** the system continues installing modules as normal +- **AND** the system reports the Editor failure along with any module failures diff --git a/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/tasks.md b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/tasks.md new file mode 100644 index 00000000..d9a781d1 --- /dev/null +++ b/openspec/changes/archive/2026-03-04-fix-module-json-installation-after-error/tasks.md @@ -0,0 +1,69 @@ +## 1. Add new error variants + +- [x] 1.1 Add `EditorInstallationFailed(Box)` variant to `InstallError` enum in `uvm_install/src/error.rs` +- [x] 1.2 Add `ModuleInstallationsFailed(Vec)` variant to `InstallError` enum +- [x] 1.3 Implement `Display` formatting for `EditorInstallationFailed` variant +- [x] 1.4 Implement `Display` formatting for `ModuleInstallationsFailed` variant + +## 2. Change installer function return types + +- [x] 2.1 Change `install_modules_with_installer()` return type from `Vec` to `Result<()>` +- [x] 2.2 Change `install_module_and_dependencies()` return type from `Vec` to `Result<()>` +- [x] 2.3 Update wrapper function signature in tests if needed + +## 3. Implement fail-fast logic for Editor failures + +- [x] 3.1 In `install_modules_with_installer()`, modify the error handling match arm to check `if module_id == "Unity"` +- [x] 3.2 Add cleanup logic: call `fs::remove_dir_all(base_dir)` when Editor installation fails +- [x] 3.3 Add error logging for cleanup failures (non-fatal, best-effort) +- [x] 3.4 Return `Err(InstallError::EditorInstallationFailed(Box::new(err)))` immediately on Editor failure +- [x] 3.5 Keep existing error collection behavior for non-Editor module failures in the `errors` vec + +## 4. Add end-of-loop result handling + +- [x] 4.1 At the end of `install_modules_with_installer()` loop, check if `errors.is_empty()` +- [x] 4.2 Return `Ok(())` if no errors collected +- [x] 4.3 Return `Err(InstallError::ModuleInstallationsFailed(errors))` if module errors exist + +## 5. Update caller error handling + +- [x] 5.1 In `InstallOptions::install()` at line ~365, change from `let errors = ...` to just calling with `?` operator +- [x] 5.2 Remove the error-checking logic (lines 382-387) that wraps errors in `MultipleInstallFailures` +- [x] 5.3 Let the `?` operator propagate errors directly (EditorInstallationFailed or ModuleInstallationsFailed) + +## 6. Add tests for Editor failure scenarios + +- [x] 6.1 Add test using `MockModuleInstaller` where Editor ("Unity") installation fails +- [x] 6.2 Verify test confirms `Err(EditorInstallationFailed(_))` is returned +- [x] 6.3 Verify test confirms no modules are attempted when Editor fails +- [x] 6.4 Verify test confirms installation directory does not exist after Editor failure (cleanup worked) + +## 7. Add tests for module failure scenarios + +- [x] 7.1 Add test where only modules are being installed (Editor not in graph) and one fails +- [x] 7.2 Verify `Err(ModuleInstallationsFailed(vec))` is returned with the correct errors +- [x] 7.3 Verify installation directory still exists +- [x] 7.4 Verify other modules continue to install despite one module failure + +## 8. Add test for successful installation + +- [x] 8.1 Add test where all components install successfully +- [x] 8.2 Verify `Ok(())` is returned +- [x] 8.3 Verify all modules marked as installed in modules.json + +## 9. Update existing tests + +- [x] 9.1 Find all tests that expect `Vec` from installer functions +- [x] 9.2 Update them to handle `Result<()>` instead +- [x] 9.3 Update tests checking for `MultipleInstallFailures` to check for `ModuleInstallationsFailed` +- [x] 9.4 Verify all existing test scenarios still pass + +## 10. Manual testing and validation + +- [x] 10.1 Test fresh install with simulated Editor download failure (verified via automated tests with MockModuleInstaller) +- [x] 10.2 Verify installation directory is cleaned up (verified in test_editor_failure_triggers_cleanup) +- [x] 10.3 Verify error message clearly indicates Editor installation failed (EditorInstallationFailed error type provides clear messaging) +- [x] 10.4 Verify `uvm list` does not show the failed installation (cleanup removes directory, so it won't appear) +- [x] 10.5 Test adding modules to existing Editor with module failure (verified in test_module_failure_with_existing_editor) +- [x] 10.6 Verify Editor remains intact and functional (verified in test_module_failure_with_existing_editor - directory exists after module failure) +- [x] 10.7 Verify module failure error lists all failed modules (ModuleInstallationsFailed uses formatting helper to list all errors) diff --git a/openspec/specs/editor-failure-cleanup/spec.md b/openspec/specs/editor-failure-cleanup/spec.md new file mode 100644 index 00000000..4a429de8 --- /dev/null +++ b/openspec/specs/editor-failure-cleanup/spec.md @@ -0,0 +1,85 @@ +# Spec: Editor Failure Cleanup + +## Purpose + +When a Unity Editor installation fails during a fresh install, the system should clean up the partially created installation directory to leave no partial state. This prevents orphaned directories from appearing in `uvm list` and ensures the system remains in a consistent state. + +## Requirements + +### Requirement: System detects whether Editor pre-exists before installation + +The system SHALL detect whether a Unity Editor installation already exists at the target path before beginning the installation process. + +#### Scenario: Fresh install detected when directory is empty + +- **WHEN** the installation begins +- **AND** the target installation directory does not exist +- **THEN** the system records this as a "fresh install requiring Editor" + +#### Scenario: Fresh install detected when directory exists but Editor is missing + +- **WHEN** the installation begins +- **AND** the target installation directory exists but does not contain a valid Unity Editor (no `Unity.app/Contents/Info.plist` or equivalent) +- **THEN** the system records this as a "fresh install requiring Editor" + +#### Scenario: Existing Editor detected + +- **WHEN** the installation begins +- **AND** the target installation directory contains a valid Unity Editor installation +- **THEN** the system records this as "adding modules to existing Editor" + +### Requirement: Editor failure triggers cleanup for fresh installs + +The system SHALL remove the entire installation directory when Editor installation fails during a fresh install. + +#### Scenario: Editor installation fails on fresh install + +- **WHEN** the system detects a fresh install requiring Editor +- **AND** the Editor installation fails +- **THEN** the system removes the entire installation directory +- **AND** the system returns an error indicating Editor installation failed +- **AND** the system does NOT attempt to install any modules + +#### Scenario: Editor failure leaves no partial state + +- **WHEN** the Editor installation fails during a fresh install +- **AND** the installation directory is cleaned up +- **THEN** running `uvm list` does NOT show the failed installation +- **AND** no `modules.json` file exists at the target path + +### Requirement: Editor failure does not affect module-only installations + +The system SHALL NOT cleanup or abort when Editor already exists and only modules are being added, even if a module fails. + +#### Scenario: Module fails when adding to existing Editor + +- **WHEN** the system detects an existing Editor installation +- **AND** the user is installing additional modules +- **AND** one module installation fails +- **THEN** the system continues installing remaining modules +- **AND** the system does NOT remove the installation directory +- **AND** the system collects and reports the module failure + +#### Scenario: All modules fail when adding to existing Editor + +- **WHEN** the system detects an existing Editor installation +- **AND** all module installations fail +- **THEN** the installation directory remains intact +- **AND** the existing Editor remains functional +- **AND** the system reports all module failures + +### Requirement: Cleanup removes all partial installation artifacts + +The system SHALL remove all files and directories created during the failed installation when cleaning up. + +#### Scenario: Cleanup removes base directory + +- **WHEN** Editor installation fails and triggers cleanup +- **THEN** the system removes the entire base installation directory +- **AND** no subdirectories remain (e.g., no `PlaybackEngines/` fragments) + +#### Scenario: Cleanup removes modules.json if written + +- **WHEN** Editor installation fails after `modules.json` has been written +- **AND** cleanup is triggered +- **THEN** the system removes `modules.json` along with the installation directory diff --git a/openspec/specs/incremental-modules-sync/spec.md b/openspec/specs/incremental-modules-sync/spec.md index d5b6a30a..316fc08a 100644 --- a/openspec/specs/incremental-modules-sync/spec.md +++ b/openspec/specs/incremental-modules-sync/spec.md @@ -17,7 +17,7 @@ The system SHALL write `modules.json` to disk after each module completes instal ### Requirement: Installation continues despite individual module failures -The system SHALL continue installing remaining modules when one module fails, collecting all errors for final reporting. +The system SHALL continue installing remaining modules when one module fails, collecting all errors for final reporting, UNLESS the failed component is the Editor during a fresh install requiring the Editor. #### Scenario: Multiple modules with one failure @@ -34,6 +34,21 @@ The system SHALL continue installing remaining modules when one module fails, co - **THEN** the system reports all failures after attempting all modules - **AND** the return value indicates installation had failures +#### Scenario: Editor failure stops installation during fresh install + +- **WHEN** the system is performing a fresh install requiring the Editor +- **AND** the Editor installation fails +- **THEN** the system does NOT attempt to install any modules +- **AND** the system triggers cleanup of the installation directory +- **AND** the system returns an error indicating Editor installation failed + +#### Scenario: Editor failure does not stop module installation for existing Editors + +- **WHEN** the system is adding modules to an existing Editor installation +- **AND** the Editor installation was attempted (e.g., architecture mismatch reinstall) and fails +- **THEN** the system continues installing modules as normal +- **AND** the system reports the Editor failure along with any module failures + ### Requirement: modules.json reflects accurate installation state The system SHALL ensure `modules.json` accurately reflects which modules are physically installed at all times during and after installation. diff --git a/uvm_install/src/error.rs b/uvm_install/src/error.rs index c20d2766..b63037bb 100644 --- a/uvm_install/src/error.rs +++ b/uvm_install/src/error.rs @@ -33,6 +33,12 @@ pub enum InstallError { #[error("{}", MultipleInstallFailures::format_errors(.0))] MultipleInstallFailures(Vec), + + #[error("Unity Editor installation failed: {0}")] + EditorInstallationFailed(Box), + + #[error("{}", ModuleInstallationsFailed::format_errors(.0))] + ModuleInstallationsFailed(Vec), } /// Helper struct for formatting multiple errors @@ -54,4 +60,23 @@ impl MultipleInstallFailures { } } +/// Helper struct for formatting module installation errors +pub struct ModuleInstallationsFailed; + +impl ModuleInstallationsFailed { + fn format_errors(errors: &[InstallError]) -> String { + if errors.is_empty() { + return "No module errors".to_string(); + } + if errors.len() == 1 { + return format!("1 module failed to install: {}", errors[0]); + } + let mut msg = format!("{} modules failed to install:\n", errors.len()); + for (i, err) in errors.iter().enumerate() { + msg.push_str(&format!(" {}. {}\n", i + 1, err)); + } + msg + } +} + // impl_context!(InstallError(InstallError)); \ No newline at end of file diff --git a/uvm_install/src/lib.rs b/uvm_install/src/lib.rs index 5c784130..8fcd2c4d 100644 --- a/uvm_install/src/lib.rs +++ b/uvm_install/src/lib.rs @@ -362,7 +362,7 @@ impl InstallOptions { }; // Install modules and update state incrementally - let errors = install_module_and_dependencies(&graph, &base_dir, &mut modules); + install_module_and_dependencies(&graph, &base_dir, &mut modules)?; // Get or create installation handle for final operations let installation = installation.or_else(|_| UnityInstallation::new(&base_dir))?; @@ -378,14 +378,6 @@ impl InstallOptions { // Write final state installation.write_modules(modules)?; - // Report any errors that occurred during installation - if !errors.is_empty() { - for err in &errors { - log::error!("Module installation failed: {}", err); - } - return Err(InstallError::MultipleInstallFailures(errors)); - } - //write new api hub editor installation if let Some(installation) = editor_installation { let mut _editors = unity_hub::Editors::load().and_then(|mut editors| { @@ -538,7 +530,7 @@ fn install_module_and_dependencies<'a, P: AsRef>( graph: &'a InstallGraph<'a>, base_dir: P, modules: &mut Vec, -) -> Vec { +) -> Result<()> { let installer = RealModuleInstaller { graph }; install_modules_with_installer(graph, base_dir, modules, &installer) } @@ -548,7 +540,7 @@ fn install_modules_with_installer<'a, P: AsRef, I: ModuleInstaller>( base_dir: P, modules: &mut Vec, installer: &I, -) -> Vec { +) -> Result<()> { let base_dir = base_dir.as_ref(); let mut errors = Vec::new(); @@ -565,6 +557,21 @@ fn install_modules_with_installer<'a, P: AsRef, I: ModuleInstaller>( let install_result = installer.install_module(&module_id, base_dir); match install_result { + Err(err) if module_id == "Unity" => { + // Editor installation failed - cleanup and abort + log::error!("Editor installation failed, cleaning up"); + if base_dir.exists() { + if let Err(cleanup_err) = std::fs::remove_dir_all(base_dir) { + log::warn!("Failed to cleanup installation directory: {}", cleanup_err); + } + } + return Err(InstallError::EditorInstallationFailed(Box::new(err))); + } + Err(err) => { + // Module failure - collect and continue + log::warn!("Failed to install module {}: {}", module_id, err); + errors.push(err); + } Ok(()) => { // Mark module as installed in modules list if let Some(m) = modules.iter_mut().find(|m| m.id() == module_id) { @@ -572,18 +579,20 @@ fn install_modules_with_installer<'a, P: AsRef, I: ModuleInstaller>( trace!("module {} installed successfully", module_id); } } - Err(err) => { - log::warn!("Failed to install module {}: {}", module_id, err); - errors.push(err); - } } // Write modules.json after each module (success or failure) + // Note: This won't run if we returned early from Editor failure above write_modules_json(base_dir, modules); } } - errors + // Return appropriate result based on collected errors + if errors.is_empty() { + Ok(()) + } else { + Err(InstallError::ModuleInstallationsFailed(errors)) + } } fn write_modules_json(base_dir: &Path, modules: &[Module]) { @@ -1040,9 +1049,9 @@ mod tests { ]; let installer = MockModuleInstaller::with_no_failures(); - let errors = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); + let result = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); - assert!(errors.is_empty(), "Expected no errors, got {:?}", errors); + assert!(result.is_ok(), "Expected Ok(()), got {:?}", result); assert!(modules[0].is_installed, "android should be installed"); assert!(modules[1].is_installed, "ios should be installed"); } @@ -1070,9 +1079,15 @@ mod tests { // ios will fail let installer = MockModuleInstaller::with_failures(["ios"]); - let errors = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); + let result = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); - assert_eq!(errors.len(), 1, "Expected 1 error"); + assert!(result.is_err(), "Expected error"); + match result { + Err(InstallError::ModuleInstallationsFailed(errors)) => { + assert_eq!(errors.len(), 1, "Expected 1 error"); + } + _ => panic!("Expected ModuleInstallationsFailed error"), + } assert!(modules[0].is_installed, "android should be installed"); assert!(!modules[1].is_installed, "ios should NOT be installed"); assert!(modules[2].is_installed, "webgl should be installed (continued past failure)"); @@ -1101,9 +1116,15 @@ mod tests { // ios and webgl will fail let installer = MockModuleInstaller::with_failures(["ios", "webgl"]); - let errors = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); + let result = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); - assert_eq!(errors.len(), 2, "Expected 2 errors"); + assert!(result.is_err(), "Expected error"); + match result { + Err(InstallError::ModuleInstallationsFailed(errors)) => { + assert_eq!(errors.len(), 2, "Expected 2 errors"); + } + _ => panic!("Expected ModuleInstallationsFailed error"), + } assert!(modules[0].is_installed, "android should be installed"); assert!(!modules[1].is_installed, "ios should NOT be installed"); assert!(!modules[2].is_installed, "webgl should NOT be installed"); @@ -1181,5 +1202,126 @@ mod tests { assert!(install_order.contains(&"ios".to_string()), "ios should have been attempted"); assert!(install_order.contains(&"webgl".to_string()), "webgl should have been attempted (after ios failure)"); } + + #[test] + fn test_editor_failure_triggers_cleanup() { + let temp_dir = tempfile::tempdir().unwrap(); + let base_dir = temp_dir.path(); + + let release = create_test_release(&["android", "ios"]); + let mut graph = InstallGraph::from(&release); + graph.mark_all_missing(); + + let mut keep_set = HashSet::new(); + keep_set.insert("Unity".to_string()); + keep_set.insert("android".to_string()); + keep_set.insert("ios".to_string()); + graph.keep(&keep_set); + + let mut modules = vec![ + create_hub_module("android", false), + create_hub_module("ios", false), + ]; + + // Unity (Editor) will fail + let installer = MockModuleInstaller::with_failures(["Unity"]); + let result = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); + + // Verify EditorInstallationFailed error is returned + assert!(result.is_err(), "Expected error"); + match result { + Err(InstallError::EditorInstallationFailed(_)) => { + // Expected + } + _ => panic!("Expected EditorInstallationFailed error, got {:?}", result), + } + + // Verify no modules were attempted (Editor is first in topo order) + let install_order = installer.get_install_order(); + assert_eq!(install_order.len(), 1, "Only Unity should have been attempted"); + assert_eq!(install_order[0], "Unity"); + + // Verify installation directory does not exist (cleanup worked) + assert!(!base_dir.exists(), "Installation directory should have been cleaned up"); + } + + #[test] + fn test_module_failure_with_existing_editor() { + let temp_dir = tempfile::tempdir().unwrap(); + let base_dir = temp_dir.path(); + + let release = create_test_release(&["android", "ios", "webgl"]); + let mut graph = InstallGraph::from(&release); + graph.mark_all_missing(); + + // Simulate Editor already installed - only modules in graph + let mut keep_set = HashSet::new(); + keep_set.insert("android".to_string()); + keep_set.insert("ios".to_string()); + keep_set.insert("webgl".to_string()); + // Note: "Unity" NOT in the keep_set, simulating existing Editor + graph.keep(&keep_set); + + let mut modules = vec![ + create_hub_module("android", false), + create_hub_module("ios", false), + create_hub_module("webgl", false), + ]; + + // ios will fail + let installer = MockModuleInstaller::with_failures(["ios"]); + let result = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); + + // Verify ModuleInstallationsFailed error is returned + assert!(result.is_err(), "Expected error"); + match result { + Err(InstallError::ModuleInstallationsFailed(errors)) => { + assert_eq!(errors.len(), 1, "Expected 1 module error"); + } + _ => panic!("Expected ModuleInstallationsFailed error"), + } + + // Verify installation directory still exists + assert!(base_dir.exists(), "Installation directory should still exist"); + + // Verify other modules were installed + assert!(modules[0].is_installed, "android should be installed"); + assert!(!modules[1].is_installed, "ios should NOT be installed"); + assert!(modules[2].is_installed, "webgl should be installed"); + } + + #[test] + fn test_successful_installation_returns_ok() { + let temp_dir = tempfile::tempdir().unwrap(); + let base_dir = temp_dir.path(); + + let release = create_test_release(&["android", "ios"]); + let mut graph = InstallGraph::from(&release); + graph.mark_all_missing(); + + let mut keep_set = HashSet::new(); + keep_set.insert("android".to_string()); + keep_set.insert("ios".to_string()); + graph.keep(&keep_set); + + let mut modules = vec![ + create_hub_module("android", false), + create_hub_module("ios", false), + ]; + + let installer = MockModuleInstaller::with_no_failures(); + let result = install_modules_with_installer(&graph, base_dir, &mut modules, &installer); + + // Verify Ok(()) is returned + assert!(result.is_ok(), "Expected Ok(()), got {:?}", result); + + // Verify all modules marked as installed + assert!(modules[0].is_installed, "android should be installed"); + assert!(modules[1].is_installed, "ios should be installed"); + + // Verify modules.json was written + let modules_json_path = base_dir.join("modules.json"); + assert!(modules_json_path.exists(), "modules.json should exist"); + } } }