-
Notifications
You must be signed in to change notification settings - Fork 0
fix(lifecycle): stop install fallback when cancellation races managed install #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
2 changes: 2 additions & 0 deletions
2
openspec/changes/fix-install-cancel-method-fallback/.openspec.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| schema: spec-driven | ||
| created: 2026-07-03 |
28 changes: 28 additions & 0 deletions
28
openspec/changes/fix-install-cancel-method-fallback/design.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| ## Context | ||
|
|
||
| `waitForSpawnedCommand()` intentionally returns exit code `1` when the CLI context is cancelled, even if the child already exited `0`. Managed installers such as Bun use this helper, so a timed-out or cancelled install can leave a package on disk while `executeMethod()` returns `false`. | ||
|
|
||
| `installAgent()` already rolls back when `executeMethod()` returns `true` and cancellation is observed before persistence. It does not handle the case where cancellation makes a successful install look like a failed method attempt. | ||
|
|
||
| ## Goals / Non-Goals | ||
|
|
||
| - Goals: prevent duplicate global installs and orphaned packages when cancellation races a successful managed install subprocess exit. | ||
| - Non-Goals: redesign cancellation semantics, change batch command orchestration, or alter update/uninstall flows in this change. | ||
|
|
||
| ## Decisions | ||
|
|
||
| - Check `getCliContext().cancelled` at the start of each install-method loop iteration and return failure immediately. | ||
| - When `executeMethod()` returns `false` and the context is cancelled, call `rollbackManagedInstall()` for the current method and return failure without trying later methods. | ||
| - Mirror the existing rollback helper used for post-success cancellation paths. | ||
|
|
||
| ## Risks / Trade-offs | ||
|
|
||
| - Best-effort rollback on cancellation may call uninstall for a method whose install never completed; existing uninstall commands are already tolerant of absent packages. | ||
|
|
||
| ## Migration Plan | ||
|
|
||
| - Patch release only; no migration required. | ||
|
|
||
| ## Open Questions | ||
|
|
||
| - None. |
24 changes: 24 additions & 0 deletions
24
openspec/changes/fix-install-cancel-method-fallback/proposal.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| ## Why | ||
|
|
||
| When a managed install subprocess exits successfully but the CLI context is already cancelled, `waitForSpawnedCommand()` reports failure. `installAgent()` treats that as a failed method attempt and continues to the next install method without rolling back the package that was just installed on disk, leaving duplicate globals and inconsistent state. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Stop install method fallback when the CLI context is cancelled. | ||
| - Roll back the current managed install method when cancellation turns a successful subprocess exit into a reported failure. | ||
| - Add regression tests covering cancellation after a successful managed install subprocess exit. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### New Capabilities | ||
|
|
||
| - None. | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `agent-update`: cancelled managed installs must not fall through to later install methods and must roll back packages installed by the cancelled attempt. | ||
|
|
||
| ## Impact | ||
|
|
||
| - Affected code: `src/package-manager/index.ts`, `test/package-manager/index.test.ts`. | ||
| - No CLI flags, schema version, or command catalog changes. |
40 changes: 40 additions & 0 deletions
40
openspec/changes/fix-install-cancel-method-fallback/specs/agent-update/spec.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| ## MODIFIED Requirements | ||
|
|
||
| ### Requirement: Managed lifecycle cancellation MUST terminate Windows wrapper process trees before wrapper fallback kill | ||
|
|
||
| When Quantex cancels a managed lifecycle installer on Windows and a child process identifier is available, it SHALL attempt process-tree termination before directly killing the wrapper process. Quantex MUST preserve sticky cancellation semantics if process-tree termination is unavailable, denied, or races with child exit. | ||
|
|
||
| #### Scenario: Windows wrapper child owns a long-running installer descendant | ||
|
|
||
| - **GIVEN** Quantex is running a managed installer through a Windows wrapper process | ||
| - **AND** the wrapper has started a long-running installer descendant | ||
| - **WHEN** the managed installer operation is cancelled by signal or timeout | ||
| - **THEN** Quantex attempts process-tree termination for the wrapper process identifier before direct wrapper termination | ||
| - **AND** the installer descendant does not continue producing installer progress after Quantex returns the cancelled result | ||
| - **AND** Quantex does not persist normal installed-agent state for the cancelled operation | ||
|
|
||
| #### Scenario: Batch install does not continue after timeout cancellation | ||
|
|
||
| - **GIVEN** the user runs `quantex install <slow-agent> <fast-agent> --timeout <duration>` | ||
| - **AND** the first agent's install work exceeds the configured timeout and late-completion grace window | ||
| - **WHEN** Quantex emits a timeout cancellation result for the command | ||
| - **THEN** it does not install or persist state for `<fast-agent>` | ||
| - **AND** it does not persist normal installed-agent state for the cancelled `<slow-agent>` operation | ||
|
|
||
| #### Scenario: Batch update does not continue after timeout cancellation | ||
|
|
||
| - **GIVEN** the user runs `quantex update --all --timeout <duration>` | ||
| - **AND** an early update item exceeds the configured timeout and late-completion grace window | ||
| - **WHEN** Quantex emits a timeout cancellation result for the command | ||
| - **THEN** it does not perform later update work for remaining agents in the same command | ||
| - **AND** it does not persist normal installed-agent state for the cancelled update operation | ||
|
|
||
| #### Scenario: Cancelled managed install does not fall through to later install methods | ||
|
|
||
| - **GIVEN** an agent definition lists multiple managed install methods in priority order | ||
| - **AND** the first managed install subprocess exits successfully on disk | ||
| - **AND** the CLI context is cancelled before Quantex records the install as successful | ||
| - **WHEN** Quantex evaluates the first managed install attempt | ||
| - **THEN** it rolls back packages installed by that attempt when rollback is supported | ||
| - **AND** it does not run later install methods for the same agent in the same command | ||
| - **AND** it does not persist normal installed-agent state for the cancelled operation | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| ## 1. Implementation | ||
|
|
||
| - [x] 1.1 Guard `installAgent()` method fallback when the CLI context is cancelled | ||
| - [x] 1.2 Roll back the current managed install method when cancellation turns a successful subprocess exit into failure | ||
|
|
||
| ## 2. Validation | ||
|
|
||
| - [x] 2.1 Add regression tests for cancelled install method fallback and rollback | ||
| - [x] 2.2 Run `bun run lint`, `bun run format:check`, `bun run typecheck`, and `bun run test` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,6 +219,10 @@ export async function installAgent(agent: AgentDefinition): Promise<AgentOperati | |
| const methods = await getOrderedInstallMethods(agent) | ||
|
|
||
| for (const method of methods) { | ||
| if (getCliContext().cancelled) { | ||
| return { success: false } | ||
| } | ||
|
|
||
| if (await executeMethod(agent, method, 'install')) { | ||
| if (getCliContext().cancelled) { | ||
| await rollbackManagedInstall(agent, method) | ||
|
|
@@ -241,6 +245,11 @@ export async function installAgent(agent: AgentDefinition): Promise<AgentOperati | |
| throw error | ||
| } | ||
| } | ||
|
|
||
| if (getCliContext().cancelled) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cancellation guard looks correct: loop-start check avoids work when already cancelled; post- |
||
| await rollbackManagedInstall(agent, method) | ||
| return { success: false } | ||
| } | ||
| } | ||
|
|
||
| return { success: false } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking (archive): This delta re-lists existing Windows wrapper and batch cancellation scenarios. Archive closure must merge additively into
openspec/specs/agent-update/spec.md— add only the new scenario at lines 32–40, do not replace or drop the existing requirement text.