-
Notifications
You must be signed in to change notification settings - Fork 246
feat(desktop): support runtime override fallback #467
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
base: cline/split-440-runtime-stage-code
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,17 @@ set "SCRIPT_DIR=%~dp0" | |||||||
| set "RESOURCES_DIR=%SCRIPT_DIR%.." | ||||||||
| set "CLI_ENTRY=%RESOURCES_DIR%\app.asar.unpacked\cli\cli.js" | ||||||||
|
|
||||||||
| REM Runtime-update override (see POSIX shim for rationale): fail loud | ||||||||
| REM on missing file rather than silently falling back to bundled. | ||||||||
| if defined KANBAN_CLI_OVERRIDE ( | ||||||||
| if not exist "%KANBAN_CLI_OVERRIDE%" ( | ||||||||
| echo error: KANBAN_CLI_OVERRIDE points to missing file: %KANBAN_CLI_OVERRIDE% >&2 | ||||||||
| endlocal | ||||||||
| exit /b 1 | ||||||||
|
Comment on lines
+19
to
+20
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.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/desktop/build/bin/kanban.cmd
Line: 19-20
Comment:
The `endlocal` call here is inconsistent with every other early-exit path in this script. The parallel error paths (e.g., "Kanban CLI not found") do not call `endlocal` before `exit /b 1`, and neither does the POSIX shim have an equivalent. Removing it keeps all error exits uniform.
```suggestion
exit /b 1
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||
| ) | ||||||||
| set "CLI_ENTRY=%KANBAN_CLI_OVERRIDE%" | ||||||||
| ) | ||||||||
|
|
||||||||
| REM Windows packaged layout: | ||||||||
| REM Kanban\resources\bin\kanban.cmd (this file) | ||||||||
| REM RESOURCES_DIR = Kanban\resources | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,21 @@ import { powerSaveBlocker } from "electron"; | |
|
|
||
| import { RuntimeChildManager } from "./runtime-child.js"; | ||
|
|
||
|
|
||
| interface RuntimeOrchestratorOptions { | ||
|
|
||
| host: string; | ||
| port: number; | ||
| healthTimeoutMs: number; | ||
| resolveCliShimPath: () => string; | ||
| /** Re-evaluated on every spawn. `null` ⇒ use the shim's bundled cli. */ | ||
| resolveCliEntryOverride?: () => string | null; | ||
| /** Called when a staged spawn fails its readiness probe. The | ||
| * orchestrator retries once with the bundled cli on this same launch; | ||
| * the callback should clear/roll back the version it just tried. | ||
| * `cliEntry` is the exact override path used for the failed spawn — | ||
| * capturing it at spawn time avoids racing with a concurrent | ||
| * background staging that may have moved the pointer to a *new* | ||
| * version (which we must not roll back). */ | ||
| onCliEntryOverrideFailed?: (reason: string, cliEntry: string) => void; | ||
|
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.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/desktop/src/runtime-orchestrator.ts
Line: 21
Comment:
**Async callback silently discarded**
`onCliEntryOverrideFailed` is typed as `() => void`, but TypeScript allows assigning an `async` function here. The retry call site does not `await` the return value, so any async work in the callback races with the immediately-following `resolveCliEntryOverride()` call. If the pointer is not yet cleared, the retry will pick up the same staged path again instead of falling back to the bundled CLI.
How can I resolve this? If you propose a fix, please make it concise. |
||
| fetchImpl?: typeof fetch; | ||
| attachedProbeIntervalMs?: number; | ||
| attachedProbeFailureThreshold?: number; | ||
|
|
@@ -71,6 +79,15 @@ export class RuntimeOrchestrator extends EventEmitter<RuntimeOrchestratorEventMa | |
| // at spawn time. Initial value `null` distinguishes "not yet looked | ||
| // up" from "looked up and resolved to a string". | ||
| private cachedShimPath: string | null = null; | ||
| // Captured at spawn time so the failure handler rolls back the | ||
| // version that actually ran — not whatever the pointer happens to | ||
| // say at failure time, which a concurrent background stage may have | ||
| // already replaced. | ||
| private currentSpawnOverridePath: string | null = null; | ||
| // Latched during the one same-launch fallback retry, so a callback | ||
| // that synchronously clears the pointer + a still-broken bundled | ||
| // runtime can't loop forever. | ||
| private overrideRetryInFlight = false; | ||
|
|
||
| // Latched once `shutdown()` / `dispose()` begin. Every `await` boundary | ||
| // in the lifecycle methods (`connect`, `restart`, `startOwnRuntime`) | ||
|
|
@@ -353,6 +370,35 @@ export class RuntimeOrchestrator extends EventEmitter<RuntimeOrchestratorEventMa | |
| this.manager.removeAllListeners("error"); | ||
| this.manager = null; | ||
| } | ||
| // A staged runtime that failed its readiness probe is broken; | ||
| // notify the host (clears the pointer) and retry the same | ||
| // launch with the bundled cli. The latch prevents an infinite | ||
| // loop if the bundled runtime is also broken. | ||
| const reason = err instanceof Error ? err.message : String(err); | ||
| const failedOverride = this.currentSpawnOverridePath; | ||
| if (failedOverride && !this.overrideRetryInFlight) { | ||
| this.currentSpawnOverridePath = null; | ||
| this.overrideRetryInFlight = true; | ||
| try { | ||
| this.opts.onCliEntryOverrideFailed?.(reason, failedOverride); | ||
| } catch (cbErr) { | ||
| console.warn( | ||
| "[desktop] onCliEntryOverrideFailed threw:", | ||
| cbErr instanceof Error ? cbErr.message : cbErr, | ||
| ); | ||
| } | ||
| console.warn( | ||
| `[desktop] Staged runtime failed (${reason}); falling back to bundled.`, | ||
| ); | ||
| if (this.terminated) return; | ||
| try { | ||
| await this.startOwnRuntime(); | ||
| } finally { | ||
| this.overrideRetryInFlight = false; | ||
| } | ||
|
Comment on lines
+393
to
+398
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.
The Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/desktop/src/runtime-orchestrator.ts
Line: 393-398
Comment:
**`overrideRetryInFlight` not reset on early `terminated` return**
The `if (this.terminated) return` at line 393 exits before the `try/finally` block, so `overrideRetryInFlight` remains `true` if `shutdown()` races in at that exact point. In practice this is benign — a terminated orchestrator spawns nothing further — but moving the check inside the `try` would make the reset unconditional and easier to reason about.
How can I resolve this? If you propose a fix, please make it concise. |
||
| return; | ||
| } | ||
| this.currentSpawnOverridePath = null; | ||
| // Suppress on terminated — caller (drain inside shutdown/dispose) | ||
| // already moved past the point where it cares about the spawn | ||
| // failure, and re-throwing would surface as an unhandled | ||
|
|
@@ -389,10 +435,31 @@ export class RuntimeOrchestrator extends EventEmitter<RuntimeOrchestratorEventMa | |
| return resolved; | ||
| } | ||
|
|
||
| private resolveCliEntryOverride(): string | undefined { | ||
| this.currentSpawnOverridePath = null; | ||
| const resolver = this.opts.resolveCliEntryOverride; | ||
| if (!resolver) return undefined; | ||
| let override: string | null; | ||
| try { | ||
| override = resolver(); | ||
| } catch (err) { | ||
| console.warn( | ||
| "[desktop] resolveCliEntryOverride threw:", | ||
| err instanceof Error ? err.message : err, | ||
| ); | ||
| return undefined; | ||
| } | ||
| if (!override) return undefined; | ||
| this.currentSpawnOverridePath = override; | ||
| console.log(`[desktop] Runtime override → ${override}`); | ||
| return override; | ||
| } | ||
|
|
||
| private createManager(): RuntimeChildManager { | ||
| const manager = new RuntimeChildManager({ | ||
| cliPath: this.getValidatedShimPath(), | ||
| shutdownTimeoutMs: DEFAULT_CHILD_SHUTDOWN_TIMEOUT_MS, | ||
| cliEntryOverride: this.resolveCliEntryOverride(), | ||
| }); | ||
|
|
||
|
|
||
|
|
||
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.
echoline expands%KANBAN_CLI_OVERRIDE%without quoting. On Windows, if the value contains shell metacharacters (&,|,>,<), cmd.exe splits the line at those characters and executes the trailing token as a separate command. The POSIX shim double-quotes$KANBAN_CLI_OVERRIDEat every expansion site; the same caution applies here.Prompt To Fix With AI