fix(provider): represent provider install/init lifecycle in the UI - #811
Draft
skevetter wants to merge 11 commits into
Draft
fix(provider): represent provider install/init lifecycle in the UI#811skevetter wants to merge 11 commits into
skevetter wants to merge 11 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
from
July 29, 2026 23:43
c6c1cbb to
79efbc9
Compare
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
4 times, most recently
from
July 30, 2026 02:49
8fe37ca to
fe69ca6
Compare
updateProvider re-resolved the source and downloaded replacement binaries but never touched Initialized, so a provider updated via `provider set-source <name> --use=false` kept reporting initialized: true even though the new binary's Exec.Init had not run. loadInitializedProvider and the provider list both trust that flag, so workspace create would admit a provider that was never initialized against its current source. Reset the flag on update. The default --use=true path reloads config and re-runs ConfigureProvider afterwards, so it still ends up initialized; only the install-without-init path is now reported accurately. Covers the --version pin path too, which routes through UpdateProvider.
Two defects made `task cli:build:grpc` unusable as documented: The pinned generators (protoc-gen-go v1.36.4, grpc v1.5.1) were not the ones that produced the committed .pb.go files (v1.36.11, v1.6.2), so running the task downgraded the output and produced a 162-line diff unrelated to any actual change. It also installed the plugins to GOPATH/bin without putting that on PATH, so protoc could not resolve them and the task failed outright on a machine that did not already have them installed. Pin the versions the committed output was generated with and export the plugin dir. The task now regenerates byte-identical output.
Provider install/init will emit the same Phase/Event stream workspace up does. Phase names alone cannot tell the two apart, so a shared sink has no way to route or filter them. Add Pipeline to status.Event and the NDJSON StatusEnvelope. Producers declare their pipeline once via ForPipeline rather than at every Enter/Leave/Fail call site, so existing reporting code is untouched. Deliberately not on the tunnel's StatusUpdate: the tunnel exists for container-side reporting, and provider commands run host-side, so no provider event can reach it. The host stamps the pipeline when it builds its reporter, which would overwrite any wire value anyway. Adding the field would have committed us to a CLI<->agent wire surface that nothing reads. Parsing tolerates a missing pipeline so status lines from an older CLI are still recognized.
Provider add/init/set-source emitted only freeform log text, so the desktop had to sniff "Exit code: 0" out of the log stream to decide a command had finished. Emit the same Phase/Event stream workspace up does, tagged with the provider pipeline. Phases: installing_provider covers source resolution and binary download; resolving_options and running_init split provider init, since only the latter executes provider-supplied code. PhaseReady is emitted only when the provider is actually usable. The --use=false paths on add and set-source install without initializing, and callers chain `provider init` afterwards, so reporting ready there would signal completion while the provider was still uninitialized.
Adding a provider flashed "not initialized" for several seconds. The badge read the persisted `initialized` flag, which `provider add` writes as false before init runs, and the watcher faithfully reported that. The existing mitigation was a renderer-local pending set that only covered init, and was marked after `provider add` had already returned — so it missed the whole multi-second install. Move lifecycle ownership into the main process: a ProviderJobs registry fed by the CLI's status events, broadcast on the same providers-changed channel as the provider list so the two cannot arrive out of order. Job state now survives the wizard closing, navigation, and window reload, which is exactly when a long install is still running. The registry also distinguishes states the flag cannot: installing vs initializing vs updating, and failed vs never-initialized. Update and version-pin now chain `provider init`, since both swap binaries and clear the flag; without it the badge would correctly, but unhelpfully, stay red until the user initialized by hand.
Drives the real Electron app through add+init and samples the card continuously, asserting the red "not initialized" badge never appears and that a busy label does. The test fails against the pre-fix precedence, so it genuinely covers the reported bug rather than restating the new code. Writing it exposed a gap the unit tests could not: a finished job cleared immediately while the provider list was only re-read on the next 3s poll, briefly exposing the same red badge. ProviderJobs.finish now refreshes the list before clearing, and the streaming init path awaits that refresh before telling the wizard it is done. The mock CLI now emits provider status envelopes and persists the uninitialized-then-initialized transition, matching the real pipeline. Init is slowed only for *probe providers, since a new card takes ~1.5s to render and there would otherwise be no in-flight window to observe.
app.e2e asserts the mock fixture's exact counts (2 workspaces, 2 providers) but never reset the mock's persisted state, so any spec that created or deleted a workspace in an earlier run left it asserting against inherited data. It read 3 instead of 2 whenever the state file survived. Other specs that depend on the fixture already call resetMockState; do the same here.
The renderer decided whether a command succeeded by string-matching its final log line against "Exit code: 0". That is fragile in itself, and it became a real hazard once provider commands started emitting NDJSON status lines: filtering those out of the log stream could remove the very text the check depended on. The main process already knows the exit code, so have it say so. Add success to the command-progress payload and set it at every terminal event; isCommandSuccess now prefers it and only falls back to sniffing for events emitted without it.
provider_add deliberately leaves its job open so the badge stays busy across the add→init handoff. Nothing closed it when init never ran, so skipping initialization or closing the wizard mid-flow left the card spinning on "installing…" until the app restarted. Add provider_release_job and call it from the wizard's skip and reset paths. Releasing a job preserves an already-recorded failure, so closing the wizard after a failed init still reports the failure rather than quietly clearing it. Covered both in unit tests over the registry and an e2e case that drives the real app; both fail without the fix.
Every handler opened a job and closed it by hand in matching try/catch blocks. That is how the stuck-spinner bug happened: one path forgot, and a provider stayed busy forever. Add withProviderJob, which starts a job, closes it when the work resolves, and records the error when it rejects. The four self-contained handlers use it, so forgetting is no longer possible there. Two sites still manage jobs directly because the job outlives the call by design: provider_add hands off to the wizard's chained init, and provider_init_streaming returns as soon as the child spawns and closes from onExit. Both now say so, and provider_release_job covers the handoff. Tests drive the job lifecycle through the IPC handlers; the add-handoff and release cases fail if add closes its own job.
Five issues from a CodeRabbit review of the provider lifecycle work: runStreaming registered "close" but not "error", so a spawn failure (missing binary, EACCES) never invoked onExit. Callers that wrap it in a promise hung forever with the job stuck busy, and the concurrency slot leaked. Report the failure as an exit instead, guarding against a double report. finish() awaited a CLI round-trip and then deleted by name without checking the entry was still its own, so a job started during that window was cleared out from under a running command — its phase reports then no-op'd and the card looked idle. Track a generation per start() and verify before deleting. The wizard released the job even while init was still running, dropping the status of a live command. Only release when nothing is in flight; a failed job stays safe either way since finish() preserves the failure. Also: render the uninitialized badge in the provider sheet, which showed no lifecycle badge at all in that state, and re-read state inside the mock CLI's deferred init so a stale snapshot can't clobber a concurrent spec's write.
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
from
July 30, 2026 14:20
fe69ca6 to
551c499
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Note
Stacked on #798. Base is
feat/async-workspace-up, so this diff shows only the provider work. Merge #798 first; the base then retargets tomainautomatically.Adding a provider showed a red "not initialized" badge for several seconds before flipping to "initialized".
provider add --use=falsepersists the provider toconfig.yamlbefore init runs (cmd/provider/add.goreturns early, skipping the only code that setsInitialized = true), and the watcher faithfully reported that.parseProviderEntriescollapsed "not yet known" and "genuinely not initialized" into the samefalse, so the card rendered the destructive badge for both.The existing mitigation was a renderer-local pending set that only covered init, and was marked after
providerAddhad already returned — missing the entire multi-second install.Approach
Two lifecycle axes were collapsed into one boolean. They're now separate:
Initialized boolon disk — "did this binary runExec.Init". No schema change, no migration.ProviderJobsregistry, fed by the CLI's structured status events and broadcast on the existingproviders-changedchannel. Main-process ownership means it survives the wizard closing, navigation, and window reload — exactly when a long install is still running.This also distinguishes states the flag cannot: installing vs initializing vs updating, and failed vs never-initialized.
Also included
provider add/init/set-sourceemit structured progress (pkg/statusPhase/Event/Reporter, asworkspace updoes), replacing the desktop's"Exit code: 0"string-sniffing.pkg/devcontainer/status→pkg/status, since it's no longer devcontainer-specific.set-sourceleftInitializedstale, so a provider updated to a new binary still reportedinitialized: truethough the new binary's init never ran — a false positive admitting an uninitialized provider throughloadInitializedProvider. Update and version-pin now chainprovider init.runStreaminghang: it registeredclosebut noterror, so a spawn failure never invokedonExit— callers hung forever and the concurrency slot leaked. Affected every streaming command, not just providers.task cli:build:grpcpinned generator versions that didn't match the committed.pb.gofiles and didn't put the plugin dir onPATH, so it failed or produced a 162-line phantom diff.Verification
Driven against the running Electron app, not just tests — that's what caught two bugs the unit tests missed (a job cleared before the provider list refreshed, and a premature
readyphase).golangci-lintcleanA CodeRabbit review of this branch surfaced 5 findings (3 major); all are addressed in
c6c1cbb.