From 7e3b545960ee2664a98e3c577c1a3cb3a0455144 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 31 Jul 2026 20:57:28 -0500 Subject: [PATCH 01/13] fix(provider): represent provider install/init lifecycle in the UI Adding a provider showed a red "not initialized" badge for several seconds before flipping to "initialized". provider add --use=false persists the provider to config.yaml before init runs, and parseProviderEntries collapsed "not yet known" and "genuinely not initialized" into the same false, so the card rendered the destructive badge for both. Two lifecycle axes were collapsed into one boolean. They're now separate: - Persisted truth stays Initialized bool on disk: "did this binary run Exec.Init". No schema change, no migration. - Transient job state moves to a main-process ProviderJobs registry, fed by the CLI's structured status events and broadcast on the existing providers-changed channel. Main-process ownership means it survives the wizard closing, navigation, and window reload. This also distinguishes states the flag cannot: installing vs initializing vs updating, and failed vs never-initialized. Also included: - provider add/init/set-source emit structured progress (pkg/status Phase/Event/Reporter, as workspace up does), replacing the desktop's "Exit code: 0" string-sniffing. pkg/devcontainer/status -> pkg/status, since it's no longer devcontainer-specific. - Correctness fix: set-source left Initialized stale, so a provider updated to a new binary still reported initialized: true though the new binary's init never ran. Update and version-pin now chain provider init. - runStreaming hang: it registered close but not error, so a spawn failure never invoked onExit, hanging callers and leaking the concurrency slot. Affected every streaming command, not just providers. Squashed from the individual commits on this branch, rebased onto main after #798 merged. Conflicts resolved by keeping both branches' independent fixes (e.g. task PID-reuse guard, envelope validation) alongside this branch's new Pipeline/ProviderJobs work. --- Taskfile.yml | 8 +- cmd/provider/add.go | 19 +- cmd/provider/configure_shared.go | 20 +- cmd/provider/init.go | 16 +- cmd/provider/set_source.go | 18 +- cmd/provider/status.go | 60 ++++++ cmd/workspace/up/status.go | 5 +- desktop/e2e/app.e2e.ts | 5 +- desktop/e2e/fixtures/mock-devsy.cjs | 69 ++++++- desktop/e2e/providers.e2e.ts | 97 +++++++++ desktop/src/main/__tests__/cli.test.ts | 45 +++++ .../main/__tests__/ipc-provider-jobs.test.ts | 180 +++++++++++++++++ .../src/main/__tests__/provider-jobs.test.ts | 138 +++++++++++++ desktop/src/main/cli.ts | 19 +- desktop/src/main/index.ts | 13 ++ desktop/src/main/ipc.ts | 191 ++++++++++++++++-- desktop/src/main/provider-jobs.ts | 132 ++++++++++++ desktop/src/main/watcher.ts | 23 ++- .../components/provider/ProviderCard.svelte | 19 +- .../components/provider/ProviderCard.test.ts | 47 ++++- .../components/provider/ProviderSheet.svelte | 31 ++- .../components/provider/ProviderSheet.test.ts | 4 +- .../components/provider/ProviderWizard.svelte | 31 ++- .../workspace/WorkspaceWizard.svelte | 2 +- desktop/src/renderer/src/lib/ipc/commands.ts | 8 + desktop/src/renderer/src/lib/ipc/events.ts | 6 +- desktop/src/renderer/src/lib/ipc/mock.ts | 1 + .../src/renderer/src/lib/stores/providers.ts | 24 +-- desktop/src/renderer/src/lib/types/index.ts | 17 ++ .../renderer/src/lib/utils/log-parser.test.ts | 17 ++ .../src/renderer/src/lib/utils/log-parser.ts | 12 +- .../renderer/src/lib/utils/provider-status.ts | 48 +++++ .../src/pages/WorkspaceDetailPage.svelte | 2 +- pkg/agent/tunnelserver/tunnelserver.go | 2 + pkg/devcontainer/config/envelope.go | 33 +-- pkg/devcontainer/config/envelope_test.go | 38 ++++ pkg/status/status.go | 52 ++++- pkg/status/status_test.go | 29 +++ pkg/workspace/provider.go | 11 + pkg/workspace/provider_update_test.go | 41 ++++ 40 files changed, 1411 insertions(+), 122 deletions(-) create mode 100644 cmd/provider/status.go create mode 100644 desktop/src/main/__tests__/ipc-provider-jobs.test.ts create mode 100644 desktop/src/main/__tests__/provider-jobs.test.ts create mode 100644 desktop/src/main/provider-jobs.ts create mode 100644 desktop/src/renderer/src/lib/utils/provider-status.ts diff --git a/Taskfile.yml b/Taskfile.yml index b9363951a..04e9669df 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -80,8 +80,12 @@ tasks: dir: pkg/agent/tunnel cmd: | # sudo apt install protobuf-compiler - go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.5.1 - go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.4 + # Versions must match those recorded in the generated .pb.go headers; + # regenerating with a different one rewrites the whole file. + go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2 + go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11 + # protoc resolves plugins via PATH, and `go install` targets GOPATH/bin. + export PATH="$(go env GOPATH)/bin:$PATH" protoc -I . tunnel.proto --go_out=. --go_opt=paths=source_relative --go-grpc_out=. --go-grpc_opt=paths=source_relative cli:test: diff --git a/cmd/provider/add.go b/cmd/provider/add.go index 54769db41..b45a4b2e2 100644 --- a/cmd/provider/add.go +++ b/cmd/provider/add.go @@ -3,6 +3,7 @@ package provider import ( "context" "fmt" + "os" "strings" "github.com/devsy-org/devsy/cmd/flags" @@ -11,6 +12,7 @@ import ( "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/types" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" @@ -85,18 +87,31 @@ func (cmd *AddCmd) Run(ctx context.Context, devsyConfig *config.Config, args []s return err } + reporter, err := newStatusReporter(cmd.ResultFormat, os.Stdout) + if err != nil { + return err + } + + status.Enter(reporter, status.PhaseInstallingProvider, providerName) providerConfig, options, err := cmd.resolveProviderConfig(ctx, devsyConfig, providerName, args) if err != nil { + status.Fail(reporter, status.PhaseInstallingProvider, err) return err } + status.Leave(reporter, status.PhaseInstallingProvider, providerConfig.Name) log.Infof("installed provider: providerName=%s", providerConfig.Name) if !cmd.Use { log.Infof("To initialize the provider, run: devsy provider init %s", providerConfig.Name) + // No PhaseReady: installed but not initialized. return nil } - return cmd.useProvider(ctx, devsyConfig, providerConfig, options) + if err := cmd.useProvider(ctx, devsyConfig, providerConfig, options, reporter); err != nil { + return err + } + status.Leave(reporter, status.PhaseReady, providerConfig.Name) + return nil } func validateOptionalProviderName(providerName string) error { @@ -158,6 +173,7 @@ func (cmd *AddCmd) useProvider( devsyConfig *config.Config, providerConfig *provider.ProviderConfig, options []string, + reporter status.Reporter, ) error { // First add: there are no prior user values to merge, so // DiscardPriorValues is moot. Set it explicitly so future readers @@ -168,6 +184,7 @@ func (cmd *AddCmd) useProvider( UserOptions: options, DiscardPriorValues: true, SingleMachine: &cmd.SingleMachine, + Reporter: reporter, }) if configureErr != nil { devsyConfig, err := config.LoadConfig(cmd.Context, "") diff --git a/cmd/provider/configure_shared.go b/cmd/provider/configure_shared.go index 14539f7c1..95444122f 100644 --- a/cmd/provider/configure_shared.go +++ b/cmd/provider/configure_shared.go @@ -10,6 +10,7 @@ import ( "github.com/devsy-org/devsy/pkg/log" options2 "github.com/devsy-org/devsy/pkg/options" provider2 "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/status" ) // ProviderOptionsConfig parameterizes ConfigureProvider. @@ -32,6 +33,15 @@ type ProviderOptionsConfig struct { SkipInit bool SkipSubOptions bool SingleMachine *bool + + Reporter status.Reporter +} + +func (cfg ProviderOptionsConfig) reporter() status.Reporter { + if cfg.Reporter == nil { + return status.Nop() + } + return cfg.Reporter } func ConfigureProvider(ctx context.Context, cfg ProviderOptionsConfig) error { @@ -91,13 +101,18 @@ func configureProviderOptions( } // fill defaults + reporter := cfg.reporter() + status.Enter(reporter, status.PhaseResolvingOptions, cfg.Provider.Name) devsyConfig, err = options2.ResolveOptions( ctx, devsyConfig, cfg.Provider, options, cfg.SkipRequired, cfg.SkipSubOptions, cfg.SingleMachine, ) if err != nil { - return nil, fmt.Errorf("resolve options: %w", err) + err = fmt.Errorf("resolve options: %w", err) + status.Fail(reporter, status.PhaseResolvingOptions, err) + return nil, err } + status.Leave(reporter, status.PhaseResolvingOptions, cfg.Provider.Name) // run init command if !cfg.SkipInit { @@ -107,10 +122,13 @@ func configureProviderOptions( stderr := log.Writer(log.LevelError) defer func() { _ = stderr.Close() }() + status.Enter(reporter, status.PhaseRunningInit, cfg.Provider.Name) err = initProvider(ctx, devsyConfig, cfg.Provider, initIO{stdout: stdout, stderr: stderr}) if err != nil { + status.Fail(reporter, status.PhaseRunningInit, err) return nil, err } + status.Leave(reporter, status.PhaseRunningInit, cfg.Provider.Name) } return devsyConfig, nil diff --git a/cmd/provider/init.go b/cmd/provider/init.go index 857cf687e..0e75d281f 100644 --- a/cmd/provider/init.go +++ b/cmd/provider/init.go @@ -1,11 +1,14 @@ package provider import ( + "os" + "github.com/devsy-org/devsy/cmd/completion" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" ) @@ -39,14 +42,23 @@ func NewInitCmd(f *flags.GlobalFlags) *cobra.Command { if err != nil { return err } - return ConfigureProvider(cobraCmd.Context(), ProviderOptionsConfig{ + reporter, err := newStatusReporter(cmd.ResultFormat, os.Stdout) + if err != nil { + return err + } + if err := ConfigureProvider(cobraCmd.Context(), ProviderOptionsConfig{ Provider: p.Config, ContextName: devsyConfig.DefaultContext, UserOptions: cmd.Options, DiscardPriorValues: cmd.Reset, SkipInit: cmd.SkipInit, SingleMachine: &cmd.SingleMachine, - }) + Reporter: reporter, + }); err != nil { + return err + } + status.Leave(reporter, status.PhaseReady, name) + return nil }, ValidArgsFunction: func( rootCmd *cobra.Command, diff --git a/cmd/provider/set_source.go b/cmd/provider/set_source.go index 149e4bd6b..732defcc5 100644 --- a/cmd/provider/set_source.go +++ b/cmd/provider/set_source.go @@ -3,12 +3,14 @@ package provider import ( "context" "fmt" + "os" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" ) @@ -67,14 +69,23 @@ func (cmd *SetSourceCmd) Run(ctx context.Context, devsyConfig *config.Config, ar providerSource = args[1] } + reporter, err := newStatusReporter(cmd.ResultFormat, os.Stdout) + if err != nil { + return err + } + + status.Enter(reporter, status.PhaseInstallingProvider, args[0]) providerConfig, err := workspace.UpdateProvider(ctx, devsyConfig, args[0], providerSource) if err != nil { + status.Fail(reporter, status.PhaseInstallingProvider, err) return err } + status.Leave(reporter, status.PhaseInstallingProvider, providerConfig.Name) log.Infof("updated provider: providerName=%s", providerConfig.Name) if !cmd.Use { log.Infof("To initialize the provider, run: devsy provider init %s", providerConfig.Name) + // No PhaseReady: not ready until a following `provider init` runs. return nil } @@ -85,11 +96,16 @@ func (cmd *SetSourceCmd) Run(ctx context.Context, devsyConfig *config.Config, ar Provider: providerConfig, ContextName: devsyConfig.DefaultContext, UserOptions: cmd.Options, + Reporter: reporter, }); err != nil { return fmt.Errorf("configure provider: %w", err) } - return writeDefaultProvider(cmd.Context, providerConfig.Name) + if err := writeDefaultProvider(cmd.Context, providerConfig.Name); err != nil { + return err + } + status.Leave(reporter, status.PhaseReady, providerConfig.Name) + return nil } func (cmd *SetSourceCmd) runPinVersion( diff --git a/cmd/provider/status.go b/cmd/provider/status.go new file mode 100644 index 000000000..07936f047 --- /dev/null +++ b/cmd/provider/status.go @@ -0,0 +1,60 @@ +package provider + +import ( + "io" + + config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/output" + "github.com/devsy-org/devsy/pkg/status" +) + +// newStatusReporter drives provider progress output: one NDJSON status line +// per phase transition in JSON mode, human-readable info lines otherwise. +// Mirrors cmd/workspace/up's reporter so both pipelines look the same to a +// consumer reading the stream. +func newStatusReporter(resultFormat string, out io.Writer) (status.Reporter, error) { + mode, err := output.ResolveMode(resultFormat) + if err != nil { + return nil, err + } + + var r status.Reporter = plainStatusReporter{} + if mode == output.ModeJSON { + r = &jsonStatusReporter{out: out} + } + return status.ForPipeline(r, status.PipelineProvider), nil +} + +type jsonStatusReporter struct { + out io.Writer +} + +func (r *jsonStatusReporter) Report(e status.Event) { + _ = config2.WriteStatusJSON(r.out, e) +} + +type plainStatusReporter struct{} + +func (plainStatusReporter) Report(e status.Event) { + switch { + case e.Phase == status.PhaseFailed: + log.Errorf("provider: phase %q failed: %s", e.Step, e.Err) + case e.Started: + log.Infof("provider: %s", phaseLabel(e.Phase)) + } +} + +var phaseLabels = map[status.Phase]string{ + status.PhaseInstallingProvider: "installing provider", + status.PhaseResolvingOptions: "resolving options", + status.PhaseRunningInit: "running provider init", + status.PhaseReady: "ready", +} + +func phaseLabel(p status.Phase) string { + if label, ok := phaseLabels[p]; ok { + return label + } + return string(p) +} diff --git a/cmd/workspace/up/status.go b/cmd/workspace/up/status.go index c325aa70a..e0c38e9cb 100644 --- a/cmd/workspace/up/status.go +++ b/cmd/workspace/up/status.go @@ -11,10 +11,11 @@ import ( // newStatusReporter drives `up`'s progress output. func newStatusReporter(emitJSON bool, out io.Writer) status.Reporter { + var r status.Reporter = plainStatusReporter{} if emitJSON { - return &jsonStatusReporter{out: out} + r = &jsonStatusReporter{out: out} } - return plainStatusReporter{} + return status.ForPipeline(r, status.PipelineWorkspaceUp) } // jsonStatusReporter serializes write events. diff --git a/desktop/e2e/app.e2e.ts b/desktop/e2e/app.e2e.ts index 43ac75523..d2499b8d4 100644 --- a/desktop/e2e/app.e2e.ts +++ b/desktop/e2e/app.e2e.ts @@ -1,11 +1,14 @@ import type { ElectronApplication, Page } from "@playwright/test" import { expect, test } from "@playwright/test" -import { launchApp } from "./electron-app.js" +import { launchApp, resetMockState } from "./electron-app.js" let app: ElectronApplication let page: Page test.beforeAll(async () => { + // The badge counts below assert the mock's default fixture exactly, so + // start from a clean slate rather than inheriting another spec's CRUD. + resetMockState() ;({ app, page } = await launchApp()) }) diff --git a/desktop/e2e/fixtures/mock-devsy.cjs b/desktop/e2e/fixtures/mock-devsy.cjs index 58b826938..a114b1c04 100755 --- a/desktop/e2e/fixtures/mock-devsy.cjs +++ b/desktop/e2e/fixtures/mock-devsy.cjs @@ -110,6 +110,19 @@ function out(data) { ) } +// Single-line NDJSON status envelope, matching pkg/status + envelope.go. +function providerStatus(phase, started, step) { + out( + JSON.stringify({ + kind: "status", + pipeline: "provider", + phase, + ...(step ? { step } : {}), + started, + }), + ) +} + // Parse a slice of args (positional + recognized flags) into a result object. function parseArgs(args) { const positional = [] @@ -530,9 +543,16 @@ switch (cmd) { case "add": { const provName = extra if (provName) { - for (const key of Object.keys(state.providers)) { - state.providers[key].default = false + // The desktop passes --use=false, which installs without changing + // the default provider; only `provider use` does that. + const takesDefault = !rawArgs.includes("--use=false") + if (takesDefault) { + for (const key of Object.keys(state.providers)) { + state.providers[key].default = false + } } + // Written uninitialized, exactly as the real CLI does: `provider + // add --use=false` persists before any init runs. state.providers[provName] = { config: { name: provName, @@ -544,11 +564,39 @@ switch (cmd) { optionGroups: [], }, state: { initialized: false }, - default: true, + default: takesDefault, } saveState(state) } - out("") + // No ready phase: install finished, init has not run. + providerStatus("installing_provider", true) + providerStatus("installing_provider", false, provName) + process.exit(0) + break + } + case "init": { + const provName = extra || providerFlag + providerStatus("resolving_options", true, provName) + providerStatus("resolving_options", false, provName) + providerStatus("running_init", true, provName) + // Providers named *probe belong to lifecycle tests, where init must + // outlast the ~1.5s a new card takes to render or there is no + // in-flight state to observe. Everything else stays fast so unrelated + // specs aren't slowed down. + const initMs = /probe$/.test(provName) ? 5000 : 150 + setTimeout(() => { + // Re-read: the snapshot taken at startup is up to 5s stale, and + // writing it back would clobber any add/delete another spec + // performed against this shared fixture meanwhile. + const latest = loadState() + if (provName && latest.providers[provName]) { + latest.providers[provName].state.initialized = true + saveState(latest) + } + providerStatus("running_init", false, provName) + providerStatus("ready", false, provName) + process.exit(0) + }, initMs) break } case "delete": { @@ -594,6 +642,19 @@ switch (cmd) { out("") break } + case "set-source": { + const provName = extra + // Mirrors UpdateProvider: new binaries, so Initialized is cleared and + // the caller is expected to chain `provider init`. + if (provName && state.providers[provName]) { + state.providers[provName].state.initialized = false + saveState(state) + } + providerStatus("installing_provider", true, provName) + providerStatus("installing_provider", false, provName) + process.exit(0) + break + } case "versions": out([]) break diff --git a/desktop/e2e/providers.e2e.ts b/desktop/e2e/providers.e2e.ts index aed7e25b0..9cb3dc005 100644 --- a/desktop/e2e/providers.e2e.ts +++ b/desktop/e2e/providers.e2e.ts @@ -82,3 +82,100 @@ test.describe("Providers Page", () => { await expect(sheet).toContainText("docker") }) }) + +test.describe("Provider lifecycle badges", () => { + // The bug this guards: `provider add` persists the provider with + // initialized:false before init runs, so the card briefly rendered the red + // "not initialized" badge for several seconds. + test("never shows 'not initialized' while add+init is in flight", async () => { + const main = page.locator('[data-slot="sidebar-inset"] main') + + await page.evaluate(async () => { + const api = ( + window as unknown as { + electronAPI: { + invoke: (c: string, a?: Record) => Promise + } + } + ).electronAPI + // Mock CLI state persists across runs, so start from a clean slate; + // a leftover initialized provider would skip the window under test. + await api.invoke("provider_delete", { name: "lifecycleprobe" }) + await api.invoke("provider_add", { name: "lifecycleprobe" }) + // Not awaited: the assertions below run while init is still going. + void api.invoke("provider_init", { name: "lifecycleprobe" }) + }) + + const card = main.locator("button", { hasText: "lifecycleprobe" }).first() + await expect(card).toBeVisible({ timeout: 10000 }) + + // Sample continuously from in-flight through settled. The red badge must + // never appear at any point: not during install/init, and not in the + // window between the job clearing and the provider list catching up. + let sawBusy = false + let settled = false + const deadline = Date.now() + 8000 + while (Date.now() < deadline) { + const text = ((await card.textContent()) ?? "").toLowerCase() + expect(text).not.toContain("not initialized") + if (/installing|initializing/.test(text)) sawBusy = true + // Reached only once the busy label is replaced by the settled badge. + if (sawBusy && /(^|\s)initialized/.test(text)) { + settled = true + break + } + await page.waitForTimeout(100) + } + + expect(sawBusy, "expected a busy badge during install/init").toBe(true) + expect(settled, "expected the card to settle as initialized").toBe(true) + + // Mock CLI state is shared across specs, so don't leave this behind. + await page.evaluate(async () => { + await ( + window as unknown as { + electronAPI: { + invoke: (c: string, a?: Record) => Promise + } + } + ).electronAPI.invoke("provider_delete", { name: "lifecycleprobe" }) + }) + }) + + // An abandoned wizard (skip init, or close mid-flow) leaves the install's + // job open. Without an explicit release the card spins on "installing…" + // forever, since nothing else will ever finish that job. + test("releases the job when a provider is added but never initialized", async () => { + const main = page.locator('[data-slot="sidebar-inset"] main') + + const api = async (channel: string, args: Record) => + page.evaluate( + ([c, a]) => + ( + window as unknown as { + electronAPI: { + invoke: ( + c: string, + a?: Record, + ) => Promise + } + } + ).electronAPI.invoke(c as string, a as Record), + [channel, args] as const, + ) + + await api("provider_delete", { name: "skipprobe" }) + await api("provider_add", { name: "skipprobe" }) + + const card = main.locator("button", { hasText: "skipprobe" }).first() + await expect(card).toContainText(/installing/i, { timeout: 10000 }) + + // What the wizard does on skip/close. + await api("provider_release_job", { name: "skipprobe" }) + + // Settles to the honest uninitialized state rather than staying busy. + await expect(card).toContainText(/not initialized/i, { timeout: 10000 }) + + await api("provider_delete", { name: "skipprobe" }) + }) +}) diff --git a/desktop/src/main/__tests__/cli.test.ts b/desktop/src/main/__tests__/cli.test.ts index 7cdb68908..f0242feb8 100644 --- a/desktop/src/main/__tests__/cli.test.ts +++ b/desktop/src/main/__tests__/cli.test.ts @@ -1,6 +1,7 @@ // @vitest-environment node import { execFile, spawn } from "node:child_process" import { EventEmitter } from "node:events" +import { Readable } from "node:stream" import { beforeEach, describe, expect, it, vi } from "vitest" import { CliRunner } from "../cli.js" @@ -24,6 +25,17 @@ function fakeChild() { return child } +/** A child whose stdout/stderr are real streams, as readline requires. */ +function fakeStreamingChild() { + const child = new EventEmitter() as EventEmitter & { + stdout: Readable + stderr: Readable + } + child.stdout = new Readable({ read() {} }) + child.stderr = new Readable({ read() {} }) + return child +} + vi.mock("node:child_process", async (importOriginal) => { const actual = await importOriginal() return { @@ -213,6 +225,39 @@ describe("CliRunner", () => { }) }) + describe("runStreaming", () => { + it("reports an exit when the child fails to spawn", async () => { + // A spawn failure emits "error" and never "close". Callers wrap this in + // a promise settled from onExit, so without this the promise never + // settles and the provider job stays busy forever. + const child = fakeStreamingChild() + const mockSpawn = vi.mocked(spawn) as unknown as ReturnType + mockSpawn.mockReturnValue(child) + + const onExit = vi.fn() + await cli.runStreaming(["provider", "init", "docker"], () => {}, onExit) + child.emit("error", new Error("spawn ENOENT")) + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)) + const [code, cliError] = onExit.mock.calls[0] + expect(code).toBe(-1) + expect(cliError?.message).toContain("spawn ENOENT") + }) + + it("reports the exit once when error and close both fire", async () => { + const child = fakeStreamingChild() + const mockSpawn = vi.mocked(spawn) as unknown as ReturnType + mockSpawn.mockReturnValue(child) + + const onExit = vi.fn() + await cli.runStreaming(["provider", "init", "docker"], () => {}, onExit) + child.emit("error", new Error("boom")) + child.emit("close", 1) + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)) + }) + }) + describe("stripAnsi", () => { it("removes ANSI escape sequences", () => { const result = CliRunner.stripAnsi( diff --git a/desktop/src/main/__tests__/ipc-provider-jobs.test.ts b/desktop/src/main/__tests__/ipc-provider-jobs.test.ts new file mode 100644 index 000000000..d57526f48 --- /dev/null +++ b/desktop/src/main/__tests__/ipc-provider-jobs.test.ts @@ -0,0 +1,180 @@ +// @vitest-environment node +import { EventEmitter } from "node:events" +import { beforeEach, describe, expect, it, vi } from "vitest" +import { ProviderJobs } from "../provider-jobs.js" + +const handlers = new Map unknown>() + +vi.mock("electron", () => ({ + app: { getPath: () => "/tmp", getVersion: () => "0.0.0" }, + dialog: {}, + ipcMain: { + handle: (channel: string, fn: (...args: unknown[]) => unknown) => { + handlers.set(channel, fn) + }, + on: () => undefined, + }, +})) + +vi.mock("../analytics.js", () => ({ + hashWorkspaceRef: (v: string) => v, + trackEvent: () => undefined, +})) + +const { registerIpcHandlers } = await import("../ipc.js") + +/** A child that emits the given stdout lines, then exits with `code`. */ +function fakeChild(lines: string[], code: number) { + const child = new EventEmitter() as EventEmitter & { + exitCode: number | null + signalCode: string | null + kill: () => void + } + child.exitCode = null + child.signalCode = null + child.kill = () => undefined + return { child, lines, code } +} + +function setup( + script: (cliArgs: string[]) => { lines: string[]; code: number } = () => ({ + lines: [], + code: 0, + }), +) { + const providerJobs = new ProviderJobs() + const cli = { + run: vi.fn(async () => ({})), + runRaw: vi.fn(async () => ""), + runStreaming: vi.fn( + async ( + cliArgs: string[], + onLine: (line: string, stream: "stdout" | "stderr") => void, + onExit: (code: number, cliError?: unknown) => void, + ) => { + const { lines, code } = script(cliArgs) + const { child } = fakeChild(lines, code) + // Deliver lines then exit asynchronously, as a real child does. + setTimeout(() => { + for (const line of lines) onLine(line, "stdout") + onExit(code, code === 0 ? undefined : { message: "boom" }) + }, 0) + return child + }, + ), + cancelFor: vi.fn(async () => undefined), + } + const deps = { + cli, + state: { workspaceContext: () => "ctx", providerList: () => [] }, + logStore: { + createLogFile: () => "/tmp/log.txt", + appendLog: () => true, + closeLog: async () => undefined, + onDrain: async () => undefined, + }, + pty: { cancelFor: vi.fn(async () => undefined) }, + getMainWindow: () => null, + providerJobs, + } + // biome-ignore lint/suspicious/noExplicitAny: partial test doubles + registerIpcHandlers(deps as any) + return { providerJobs, cli } +} + +function invoke(channel: string, args: Record) { + const handler = handlers.get(channel) + if (!handler) throw new Error(`${channel} not registered`) + return handler({}, args) +} + +function statusLine(phase: string) { + return JSON.stringify({ kind: "status", pipeline: "provider", phase }) +} + +describe("provider job lifecycle over IPC", () => { + beforeEach(() => { + handlers.clear() + vi.clearAllMocks() + }) + + it("clears the job when init succeeds", async () => { + const { providerJobs } = setup(() => ({ + lines: [statusLine("running_init"), statusLine("ready")], + code: 0, + })) + + await invoke("provider_init", { name: "docker" }) + + expect(providerJobs.get("docker")).toBeUndefined() + }) + + it("records the failure when init fails", async () => { + const { providerJobs } = setup(() => ({ lines: [], code: 1 })) + + const result = (await invoke("provider_init", { name: "docker" })) as { + ok: boolean + } + + expect(result.ok).toBe(false) + expect(providerJobs.get("docker")?.error).toBeTruthy() + }) + + it("leaves the job open after add, for the chained init to close", async () => { + // Closing it here would flash the red badge between add and init. + const { providerJobs } = setup(() => ({ + lines: [statusLine("installing_provider")], + code: 0, + })) + + await invoke("provider_add", { name: "docker" }) + + expect(providerJobs.get("docker")?.activity).toBe("installing") + }) + + it("releases an abandoned job so the card stops spinning", async () => { + const { providerJobs } = setup(() => ({ lines: [], code: 0 })) + + await invoke("provider_add", { name: "docker" }) + expect(providerJobs.get("docker")).toBeDefined() + + await invoke("provider_release_job", { name: "docker" }) + + expect(providerJobs.get("docker")).toBeUndefined() + }) + + it("keeps a failed job's error when released", async () => { + const { providerJobs } = setup(() => ({ lines: [], code: 1 })) + + await invoke("provider_init", { name: "docker" }) + await invoke("provider_release_job", { name: "docker" }) + + expect(providerJobs.get("docker")?.error).toBeTruthy() + }) + + it("runs set-source then init on update, clearing the job once", async () => { + const seen: string[][] = [] + const { providerJobs } = setup((cliArgs) => { + seen.push(cliArgs) + return { lines: [], code: 0 } + }) + + await invoke("provider_update", { name: "docker" }) + + expect(seen.map((a) => a[1])).toEqual(["set-source", "init"]) + expect(providerJobs.get("docker")).toBeUndefined() + }) + + it("records the failure when the update's chained init fails", async () => { + const { providerJobs } = setup((cliArgs) => ({ + lines: [], + code: cliArgs[1] === "init" ? 1 : 0, + })) + + await expect( + invoke("provider_update", { name: "docker" }), + ).rejects.toThrow() + + expect(providerJobs.get("docker")?.error).toBeTruthy() + }) +}) diff --git a/desktop/src/main/__tests__/provider-jobs.test.ts b/desktop/src/main/__tests__/provider-jobs.test.ts new file mode 100644 index 000000000..0708c3c88 --- /dev/null +++ b/desktop/src/main/__tests__/provider-jobs.test.ts @@ -0,0 +1,138 @@ +// @vitest-environment node +import { beforeEach, describe, expect, it, vi } from "vitest" +import { ProviderJobs } from "../provider-jobs.js" + +describe("ProviderJobs", () => { + let jobs: ProviderJobs + + beforeEach(() => { + jobs = new ProviderJobs() + }) + + it("tracks activity and phase transitions", async () => { + jobs.start("docker", "installing") + expect(jobs.get("docker")).toEqual({ activity: "installing" }) + + jobs.report("docker", "installing_provider") + expect(jobs.get("docker")).toEqual({ + activity: "installing", + phase: "installing_provider", + }) + + await jobs.finish("docker") + expect(jobs.get("docker")).toBeUndefined() + }) + + it("ignores phase reports for a provider with no active job", () => { + jobs.report("docker", "running_init") + expect(jobs.get("docker")).toBeUndefined() + }) + + it("retains the failure so the UI can explain it", async () => { + jobs.start("docker", "initializing") + await jobs.finish("docker", "init: boom") + + expect(jobs.get("docker")).toEqual({ + activity: "initializing", + phase: "failed", + error: "init: boom", + }) + }) + + it("does not let a later release erase a recorded failure", async () => { + jobs.start("docker", "initializing") + await jobs.finish("docker", "init: boom") + + // The wizard closing after a failed init releases the job; that must not + // turn the failure into a clean success. + await jobs.finish("docker") + + expect(jobs.get("docker")?.error).toBe("init: boom") + }) + + it("refreshes provider state before clearing a finished job", async () => { + // Clearing first would briefly expose initialized:false from the stale + // list — the red badge this class exists to prevent. + const order: string[] = [] + jobs.setRefresh(async () => { + order.push(`refresh(job=${jobs.get("docker") ? "present" : "gone"})`) + }) + + jobs.start("docker", "installing") + await jobs.finish("docker") + + expect(order).toEqual(["refresh(job=present)"]) + expect(jobs.get("docker")).toBeUndefined() + }) + + it("does not clear a newer job started while refresh was in flight", async () => { + // refresh is a real CLI round-trip. If a second command starts during it, + // the first finish() must not delete the entry the new one is using — + // its report() calls would find no job and the card would look idle. + let releaseRefresh: (() => void) | undefined + jobs.setRefresh( + () => + new Promise((resolve) => { + releaseRefresh = resolve + }), + ) + + jobs.start("docker", "installing") + const finishing = jobs.finish("docker") + + // A re-init lands before the first finish resolves. + jobs.start("docker", "initializing") + releaseRefresh?.() + await finishing + + expect(jobs.get("docker")).toEqual({ activity: "initializing" }) + }) + + it("still tracks phases for the superseding job", async () => { + let releaseRefresh: (() => void) | undefined + jobs.setRefresh( + () => + new Promise((resolve) => { + releaseRefresh = resolve + }), + ) + + jobs.start("docker", "installing") + const finishing = jobs.finish("docker") + jobs.start("docker", "initializing") + releaseRefresh?.() + await finishing + + jobs.report("docker", "running_init") + + expect(jobs.get("docker")?.phase).toBe("running_init") + }) + + it("notifies listeners on every mutation", () => { + const listener = vi.fn() + jobs.onChange(listener) + + jobs.start("docker", "installing") + jobs.report("docker", "installing_provider") + jobs.clear("docker") + + expect(listener).toHaveBeenCalledTimes(3) + }) + + it("does not notify when clearing an untracked provider", () => { + const listener = vi.fn() + jobs.onChange(listener) + + jobs.clear("nonexistent") + + expect(listener).not.toHaveBeenCalled() + }) + + it("clears a retained failure so a re-added provider starts clean", () => { + jobs.start("docker", "installing") + jobs.report("docker", "failed", "boom") + jobs.clear("docker") + + expect(jobs.get("docker")).toBeUndefined() + }) +}) diff --git a/desktop/src/main/cli.ts b/desktop/src/main/cli.ts index 367919421..0145c3022 100644 --- a/desktop/src/main/cli.ts +++ b/desktop/src/main/cli.ts @@ -297,7 +297,11 @@ export class CliRunner { }) } - child.on("close", (code) => { + let settled = false + const finish = (code: number, cliError?: CLIError): void => { + // "error" and "close" can both fire; report the outcome once. + if (settled) return + settled = true this.activeChildren.delete(child) if (workspaceId) { const bucket = this.childrenByWorkspace.get(workspaceId) @@ -307,7 +311,18 @@ export class CliRunner { } } this.release() - onExit(code ?? -1, lastCliError) + onExit(code, cliError) + } + + // A spawn failure (missing binary, EACCES) emits "error" and never + // "close". Without this, onExit never fires: callers that wrap this in a + // promise hang forever, and the concurrency slot is never released. + child.on("error", (err) => { + finish(-1, { code: "spawn_failed", message: err.message }) + }) + + child.on("close", (code) => { + finish(code ?? -1, lastCliError) }) return child diff --git a/desktop/src/main/index.ts b/desktop/src/main/index.ts index 7cf9fafa2..e91735174 100644 --- a/desktop/src/main/index.ts +++ b/desktop/src/main/index.ts @@ -6,6 +6,7 @@ import { CliRunner } from "./cli.js" import { DaemonManager } from "./daemon-manager.js" import { registerIpcHandlers } from "./ipc.js" import { LogStore } from "./log-store.js" +import { ProviderJobs } from "./provider-jobs.js" import { PtyManager } from "./pty.js" import { DaemonState } from "./state.js" import { AppTray } from "./tray.js" @@ -160,6 +161,11 @@ app.whenReady().then(() => { stopAutoUpdater() }) + // Shared between the IPC handlers that run provider commands and the + // watcher that broadcasts provider state, so in-flight work is reported + // alongside what's on disk. + const providerJobs = new ProviderJobs() + // Register IPC handlers const { tunnelProcesses, @@ -171,6 +177,7 @@ app.whenReady().then(() => { logStore, pty: ptyManager, getMainWindow: () => mainWindow, + providerJobs, }) // Start state watcher @@ -179,7 +186,13 @@ app.whenReady().then(() => { daemon: daemonManager.daemonClient, state, getMainWindow: () => mainWindow, + providerJobs, }) + // Phase transitions are pushed immediately rather than waiting for the + // next disk poll, which is what made the old badge lag visible. + providerJobs.onChange(() => watcher.broadcastProviders()) + providerJobs.setRefresh(() => watcher.refreshProviders()) + void watcher.start().then(runInitialProviderUpdateCheck) scheduleProviderUpdateCheck() diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index 191867971..924846476 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -10,6 +10,11 @@ import type { CLIError } from "../shared/cli-error.js" import { parseCliEnvelope } from "../shared/cli-error.js" import { hashWorkspaceRef, trackEvent } from "./analytics.js" import { loadCatalog } from "./image-catalog.js" +import type { + ProviderActivity, + ProviderJobs, + ProviderPhase, +} from "./provider-jobs.js" import type { CliRunner } from "./cli.js" import type { LogStore } from "./log-store.js" import type { PtyManager } from "./pty.js" @@ -90,6 +95,7 @@ interface IpcDependencies { logStore: LogStore pty: PtyManager getMainWindow: () => BrowserWindow | null + providerJobs: ProviderJobs } /** Format a line in zap console format so log-parser.ts can parse it. */ @@ -101,7 +107,11 @@ interface ProgressSink { line(formatted: string): boolean done( finalLine: string, - extra?: { level?: "info" | "warn" | "error"; cliError?: CLIError }, + extra?: { + level?: "info" | "warn" | "error" + cliError?: CLIError + success?: boolean + }, ): Promise } @@ -122,6 +132,7 @@ function createLogSink( message?: string level?: "info" | "warn" | "error" cliError?: CLIError + success?: boolean }, ): void { if (timer) { @@ -161,7 +172,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { scheduleProviderUpdateCheck: () => void runInitialProviderUpdateCheck: () => void } { - const { cli, state, logStore, pty } = deps + const { cli, state, logStore, pty, providerJobs } = deps const tunnelProcesses = new Map< string, import("node:child_process").ChildProcess @@ -243,6 +254,72 @@ export function registerIpcHandlers(deps: IpcDependencies): { }) } + function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) + } + + /** + * Track a provider job for the duration of fn, so the job cannot outlive the + * work it describes. Opening a job in one place and closing it in another + * leaves the card spinning forever on any path that forgets — prefer this to + * calling start/finish by hand. + * + * Errors are recorded on the job and rethrown, leaving the caller's own + * error handling intact. + */ + async function withProviderJob( + name: string, + activity: ProviderActivity, + fn: () => Promise, + ): Promise { + providerJobs.start(name, activity) + try { + const result = await fn() + await providerJobs.finish(name) + return result + } catch (error) { + const cliError = (error as { cliError?: CLIError }).cliError + await providerJobs.finish(name, cliError?.message ?? errorMessage(error)) + throw error + } + } + + /** + * Run a provider CLI command, feeding its NDJSON status lines into the job + * registry so the UI tracks phases as they happen instead of only learning + * the outcome at exit. + */ + function runProviderWithStatus( + name: string, + cliArgs: string[], + ): Promise { + return new Promise((resolve, reject) => { + void cli.runStreaming( + cliArgs, + (line, stream) => { + if (stream !== "stdout") return + const envelope = parseCliEnvelope(line) + if (envelope?.kind === "status") { + providerJobs.report( + name, + envelope.phase as ProviderPhase, + envelope.error, + ) + } + }, + (code, cliError) => { + if (code === 0) { + resolve() + return + } + const message = + cliError?.message ?? `${cliArgs.join(" ")} exited with ${code}` + reject(Object.assign(new Error(message), { cliError })) + }, + ) + }) + } + /** * Compute provider update information by querying the CLI for all installed providers. */ @@ -361,15 +438,37 @@ export function registerIpcHandlers(deps: IpcDependencies): { if (args.singleMachine) { cliArgs.push("--single-machine") } - await cli.runRaw(cliArgs) + + // Not withProviderJob: the job outlives this call so the badge stays busy + // until the wizard's chained provider_init finishes. The opener closes it, + // via provider_release_job on any path that abandons the install. + providerJobs.start(args.name, "installing") + try { + await runProviderWithStatus(args.name, cliArgs) + } catch (error) { + await providerJobs.finish(args.name, errorMessage(error)) + throw error + } }, ) ipcMain.handle("provider_delete", async (_event, args: { name: string }) => { trackEvent("provider_delete") await cli.runRaw(["provider", "delete", args.name]) + // Drop any retained failure so a re-added provider starts clean. + providerJobs.clear(args.name) }) + // Releases a job the caller opened but will not finish — e.g. the wizard + // installs a provider, then the user skips initialization or closes the + // dialog. Without this the card would spin on "installing…" indefinitely. + ipcMain.handle( + "provider_release_job", + async (_event, args: { name: string }) => { + await providerJobs.finish(args.name) + }, + ) + ipcMain.handle("provider_use", async (_event, args: { name: string }) => { await cli.runRaw(["provider", "use", args.name]) }) @@ -380,12 +479,13 @@ export function registerIpcHandlers(deps: IpcDependencies): { // own-properties, so a thrown Error with a .cliError attached would lose it. ipcMain.handle("provider_init", async (_event, args: { name: string }) => { try { - await cli.runRaw(["provider", "init", args.name]) + await withProviderJob(args.name, "initializing", () => + runProviderWithStatus(args.name, ["provider", "init", args.name]), + ) return { ok: true } as const } catch (err) { const cliError = (err as { cliError?: CLIError }).cliError - const message = err instanceof Error ? err.message : String(err) - return { ok: false, message, cliError } as const + return { ok: false, message: errorMessage(err), cliError } as const } }) @@ -395,9 +495,24 @@ export function registerIpcHandlers(deps: IpcDependencies): { const cmdId = crypto.randomUUID() const win = deps.getMainWindow() + // Not withProviderJob: the job outlives the handler, closed from onExit. + providerJobs.start(args.name, "initializing") await cli.runStreaming( ["provider", "init", args.name], - (line, _stream, meta) => { + (line, stream, meta) => { + // Status envelopes drive the job registry; everything else is log + // text for the wizard's output pane. + if (stream === "stdout") { + const envelope = parseCliEnvelope(line) + if (envelope?.kind === "status") { + providerJobs.report( + args.name, + envelope.phase as ProviderPhase, + envelope.error, + ) + return + } + } const formatted = formatLogLine(line) win?.webContents.send("command-progress", { commandId: cmdId, @@ -406,7 +521,15 @@ export function registerIpcHandlers(deps: IpcDependencies): { done: false, }) }, - (code, cliError) => { + async (code, cliError) => { + // Finish first: it refreshes the provider list, so the wizard's + // done signal can't arrive while the card still reads uninitialized. + await providerJobs.finish( + args.name, + code === 0 + ? undefined + : (cliError?.message ?? `provider init exited with ${code}`), + ) const exitMsg = formatLogLine( `Exit code: ${code}`, code === 0 ? "INFO" : "ERROR", @@ -415,6 +538,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { commandId: cmdId, message: exitMsg, level: code === 0 ? "info" : "error", + success: code === 0, cliError, done: true, }) @@ -425,8 +549,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { }, ) + // set-source installs replacement binaries and clears Initialized, since + // the new binary has not run its init. Chain init so the provider ends up + // usable rather than sitting in a needs-re-init state the user must notice. ipcMain.handle("provider_update", async (_event, args: { name: string }) => { - await cli.runRaw(["provider", "set-source", args.name, "--use=false"]) + await withProviderJob(args.name, "updating", async () => { + await runProviderWithStatus(args.name, [ + "provider", + "set-source", + args.name, + "--use=false", + ]) + await runProviderWithStatus(args.name, ["provider", "init", args.name]) + }) }) ipcMain.handle("provider_options", async (_event, args: { name: string }) => { @@ -485,13 +620,18 @@ export function registerIpcHandlers(deps: IpcDependencies): { ipcMain.handle( "provider_set_version", async (_event, args: { name: string; tag: string }) => { - await cli.runRaw([ - "provider", - "set-source", - args.name, - "--version", - args.tag, - ]) + // Pinning a version swaps binaries via the same update path, so it + // clears Initialized and needs the same re-init as a source change. + await withProviderJob(args.name, "updating", async () => { + await runProviderWithStatus(args.name, [ + "provider", + "set-source", + args.name, + "--version", + args.tag, + ]) + await runProviderWithStatus(args.name, ["provider", "init", args.name]) + }) }, ) @@ -813,6 +953,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { const err = error as Error & { cliError?: CLIError } void sink.done(formatLogLine(err.message, "ERROR"), { level: "error", + success: false, cliError: err.cliError ?? { code: "up_failed", message: err.message, @@ -859,7 +1000,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { if (envelope?.kind === "result") { signalledDone = true releaseTask() - void sink.done(formatted) + void sink.done(formatted, { success: true }) return } @@ -868,6 +1009,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { releaseTask() void sink.done(formatted, { level: "error", + success: false, cliError: { code: "up_failed", message: envelope.message }, }) return @@ -887,7 +1029,9 @@ export function registerIpcHandlers(deps: IpcDependencies): { `Exit code: ${code}`, code === 0 ? "INFO" : "ERROR", ), - code === 0 ? undefined : { level: "error", cliError }, + code === 0 + ? { success: true } + : { level: "error", success: false, cliError }, ) }, wsId, @@ -899,6 +1043,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { const err = error as Error & { cliError?: CLIError } void sink.done(formatLogLine(err.message, "ERROR"), { level: "error", + success: false, cliError: err.cliError ?? { code: "up_follow_failed", message: err.message, @@ -943,6 +1088,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), + { success: code === 0 }, ) }, args.workspaceId, @@ -988,6 +1134,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), + { success: code === 0 }, ) }, args.workspaceId, @@ -1026,7 +1173,9 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code, cliError) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), - code === 0 ? undefined : { level: "error", cliError }, + code === 0 + ? { success: true } + : { level: "error", success: false, cliError }, ) }, args.workspaceId, @@ -1065,7 +1214,9 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code, cliError) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), - code === 0 ? undefined : { level: "error", cliError }, + code === 0 + ? { success: true } + : { level: "error", success: false, cliError }, ) }, args.workspaceId, diff --git a/desktop/src/main/provider-jobs.ts b/desktop/src/main/provider-jobs.ts new file mode 100644 index 000000000..8c9043d6c --- /dev/null +++ b/desktop/src/main/provider-jobs.ts @@ -0,0 +1,132 @@ +/** + * Tracks in-flight provider install/init/update work in the main process. + * + * The persisted `initialized` flag answers "is this provider usable", which + * is false both for a provider the user never initialized and for one being + * installed right now. Those render very differently, so the transient half + * of the lifecycle lives here rather than being inferred from disk state. + * + * Main-process ownership is deliberate: a renderer-local pending set is lost + * when the wizard closes, the user navigates away, or the window reloads, + * which is exactly when a multi-second install is still running. + */ + +/** Phases emitted by the Go provider pipeline (pkg/status). */ +export type ProviderPhase = + | "installing_provider" + | "resolving_options" + | "running_init" + | "ready" + | "failed" + +export type ProviderActivity = "installing" | "initializing" | "updating" + +export interface ProviderJob { + activity: ProviderActivity + phase?: ProviderPhase + /** Set when the job ended in failure; the job is retained so the UI can show why. */ + error?: string +} + +export class ProviderJobs { + private jobs = new Map() + // Lets an in-flight finish() tell whether it still owns the entry. + private generations = new Map() + private lastGeneration = 0 + private listeners = new Set<() => void>() + + /** + * Register a callback fired after every mutation, so a phase transition + * reaches the renderer without waiting for the next 3s disk poll. + */ + onChange(listener: () => void): () => void { + this.listeners.add(listener) + return () => this.listeners.delete(listener) + } + + private emit(): void { + for (const listener of this.listeners) listener() + } + + /** Begin tracking work on a provider, clearing any previous failure. */ + start(name: string, activity: ProviderActivity): void { + this.jobs.set(name, { activity }) + this.generations.set(name, ++this.lastGeneration) + this.emit() + } + + /** + * Record a phase transition. Ignored when no job is active, so a stray + * event can't resurrect a provider the UI already considers settled. + */ + report(name: string, phase: ProviderPhase, error?: string): void { + const job = this.jobs.get(name) + if (!job) return + if (phase === "failed") { + this.jobs.set(name, { ...job, phase, error: error ?? "failed" }) + } else { + this.jobs.set(name, { ...job, phase }) + } + this.emit() + } + + /** Stop tracking a provider, discarding any recorded failure. */ + clear(name: string): void { + this.generations.delete(name) + if (this.jobs.delete(name)) this.emit() + } + + /** + * Finish a job. A failure is retained so the UI can explain it; success + * drops the entry and lets the persisted `initialized` flag speak. + * + * On success the caller must have refreshed the provider list first: + * clearing the job while the list still shows the pre-init `initialized: + * false` would expose the same red badge this class exists to prevent, + * until the next poll caught up. + */ + async finish(name: string, error?: string): Promise { + if (error) { + const job = this.jobs.get(name) + this.jobs.set(name, { + activity: job?.activity ?? "initializing", + phase: "failed", + error, + }) + this.emit() + return + } + const job = this.jobs.get(name) + if (!job) return + // A recorded failure survives a later success-shaped finish. + if (job.error) return + + const generation = this.generations.get(name) + await this.refresh?.() + // refresh is a CLI round-trip; a newer job may own the entry by now. + if (this.generations.get(name) !== generation) return + + this.jobs.delete(name) + this.generations.delete(name) + this.emit() + } + + /** + * Supplies a way to re-read provider state from disk, so a finished job + * isn't cleared before the list reflects what the command just wrote. + */ + setRefresh(refresh: () => Promise): void { + this.refresh = refresh + } + + private refresh?: () => Promise + + get(name: string): ProviderJob | undefined { + return this.jobs.get(name) + } + + /** Snapshot for IPC, keyed by provider name. */ + snapshot(): Record { + return Object.fromEntries(this.jobs) + } +} diff --git a/desktop/src/main/watcher.ts b/desktop/src/main/watcher.ts index b698f9db7..dd8d7b165 100644 --- a/desktop/src/main/watcher.ts +++ b/desktop/src/main/watcher.ts @@ -5,6 +5,7 @@ import { watch } from "chokidar" import type { BrowserWindow } from "electron" import type { CliRunner } from "./cli.js" import type { DaemonClient } from "./daemon-client.js" +import type { ProviderJobs } from "./provider-jobs.js" import type { DaemonState } from "./state.js" interface WatcherDeps { @@ -12,6 +13,7 @@ interface WatcherDeps { daemon?: DaemonClient state: DaemonState getMainWindow: () => BrowserWindow | null + providerJobs: ProviderJobs } interface ContextEntry { @@ -144,6 +146,11 @@ export class Watcher { } } + /** Re-read provider state from disk now, without waiting for the poll. */ + async refreshProviders(): Promise { + await this.pollProviders() + } + private async pollProviders(): Promise { try { const raw = await this.queryWithFallback( @@ -160,15 +167,25 @@ export class Watcher { const providers = parseProviderEntries(raw) const changed = this.deps.state.updateProviders(providers as any[]) if (changed) { - this.send("providers-changed", { - providers: this.deps.state.providerList(), - }) + this.broadcastProviders() } } catch { // Silently ignore poll failures } } + /** + * Push the provider list plus in-flight job state. Sent on one channel so + * the two can't arrive out of order and render a provider as idle-and- + * uninitialized between an install finishing and its job clearing. + */ + broadcastProviders(): void { + this.send("providers-changed", { + providers: this.deps.state.providerList(), + jobs: this.deps.providerJobs.snapshot(), + }) + } + private async pollMachines(): Promise { try { const machines = await this.queryWithFallback( diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte b/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte index aa7a06572..af5a04c9a 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte +++ b/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte @@ -3,12 +3,13 @@ import { badgeVariants } from "$lib/components/ui/badge/index.js" import { Star, Loader2 } from "@lucide/svelte" import ProviderIcon from "./ProviderIcon.svelte" import { providerVersions } from "$lib/stores/providerVersions.js" -import { initializingProviders } from "$lib/stores/providers.js" +import { providerJobs } from "$lib/stores/providers.js" +import { providerStatus } from "$lib/utils/provider-status.js" import type { Provider } from "$lib/types/index.js" let { provider, onopen }: { provider: Provider; onopen?: () => void } = $props() -let isInitializing = $derived($initializingProviders.has(provider.name)) +let status = $derived(providerStatus(provider, $providerJobs[provider.name])) function sourceDisplay(p: Provider): string { if (p.source?.github) return p.source.github @@ -41,15 +42,19 @@ function sourceDisplay(p: Provider): string { {/if}
- {#if provider.state?.initialized} - initialized - {:else if isInitializing} + {#if status.kind === "ready"} + {status.label} + {:else if status.kind === "busy"} - initializing… + {status.label} + + {:else if status.kind === "failed"} + + {status.label} {:else} - not initialized + {status.label} {/if} {#if provider.version} {provider.version} diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts b/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts index 6e47be242..e56b0fea5 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts +++ b/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts @@ -14,11 +14,7 @@ vi.mock("$lib/stores/providerVersions.js", async () => { }) import ProviderCard from "./ProviderCard.svelte" -import { - initializingProviders, - markInitializing, - clearInitializing, -} from "$lib/stores/providers.js" +import { providerJobs } from "$lib/stores/providers.js" function makeProvider(name: string, extras: Partial = {}): Provider { return { @@ -56,8 +52,7 @@ describe("ProviderCard", () => { }) it("shows the initializing badge while an uninitialized provider is in flight", () => { - initializingProviders.set(new Set()) - markInitializing("ssh") + providerJobs.set({ ssh: { activity: "initializing", phase: "running_init" } }) const { container, unmount } = render(ProviderCard, { props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, }) @@ -65,12 +60,44 @@ describe("ProviderCard", () => { const text = container.textContent ?? "" expect(text.toLowerCase()).toContain("initializing") expect(text.toLowerCase()).not.toContain("not initialized") - clearInitializing("ssh") + providerJobs.set({}) unmount() }) - it("shows not initialized when no init is in flight", () => { - initializingProviders.set(new Set()) + // The original bug: `provider add` persists the provider before init runs, + // so the watcher reports initialized:false while the install is in flight. + it("shows installing rather than not initialized during install", () => { + providerJobs.set({ + ssh: { activity: "installing", phase: "installing_provider" }, + }) + const { container, unmount } = render(ProviderCard, { + props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, + }) + + const text = (container.textContent ?? "").toLowerCase() + expect(text).toContain("installing") + expect(text).not.toContain("not initialized") + providerJobs.set({}) + unmount() + }) + + it("shows a failure badge when the job recorded an error", () => { + providerJobs.set({ + ssh: { activity: "initializing", phase: "failed", error: "init: boom" }, + }) + const { container, unmount } = render(ProviderCard, { + props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, + }) + + const text = (container.textContent ?? "").toLowerCase() + expect(text).toContain("failed") + expect(text).not.toContain("not initialized") + providerJobs.set({}) + unmount() + }) + + it("shows not initialized when no job is in flight", () => { + providerJobs.set({}) const { container, unmount } = render(ProviderCard, { props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, }) diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte b/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte index 5ae33b6da..9f8b8afe9 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte +++ b/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte @@ -28,11 +28,7 @@ import { providerRename, providerSetVersion, } from "$lib/ipc/commands.js" -import { - providers, - markInitializing, - clearInitializing, -} from "$lib/stores/providers.js" +import { providers, providerJobs } from "$lib/stores/providers.js" import { providerVersions, loadVersionsFor, @@ -40,6 +36,7 @@ import { } from "$lib/stores/providerVersions.js" import { toasts } from "$lib/stores/toasts.js" import { extractErrorMessage } from "$lib/utils/error.js" +import { providerStatus } from "$lib/utils/provider-status.js" import type { Provider, ProviderOption } from "$lib/types/index.js" let { @@ -70,6 +67,7 @@ let updating = $state(false) let confirmSwitchOpen = $state(false) let targetTag = $state("") let switching = $state(false) +let status = $derived(providerStatus(provider, $providerJobs[provider.name])) function openVersionSwitch(tag: string) { targetTag = tag @@ -181,8 +179,11 @@ function handleUpdate() { async function runUpdate() { updating = true try { + // Also re-initializes: the new binaries have not run their init, and + // set-source clears the initialized flag accordingly. await providerUpdate(provider.name) toasts.success(`Updated ${provider.name}`) + providers.set(await providerList()) await loadVersionsFor(provider.name) await refreshUpdates() } catch (err) { @@ -198,6 +199,7 @@ async function runSwitch() { try { await providerSetVersion(provider.name, targetTag) toasts.success(`Switched ${provider.name} to ${targetTag}`) + providers.set(await providerList()) await loadVersionsFor(provider.name) await refreshUpdates() } catch (err) { @@ -227,7 +229,6 @@ function extractCliError(err: unknown): CLIError | null { async function handleInitialize() { initializing = true initError = null - markInitializing(provider.name) try { await providerInit(provider.name) const updated = await providerList() @@ -248,7 +249,6 @@ async function handleInitialize() { } } finally { initializing = false - clearInitializing(provider.name) } } @@ -367,8 +367,19 @@ async function handleSaveOptions() { Default {/if} - {#if provider.state?.initialized} - initialized + {#if status.kind === "ready"} + {status.label} + {:else if status.kind === "busy"} + + + {status.label} + + {:else if status.kind === "failed"} + + {status.label} + + {:else} + {status.label} {/if} {#if provider.description} @@ -377,7 +388,7 @@ async function handleSaveOptions() {
- {#if provider.state?.initialized !== true} + {#if status.kind !== "ready" && status.kind !== "busy"}