feat: add initializing and failed states to provider init lifecycle - #847
Conversation
…ycle Provider init previously only tracked a persisted Initialized bool, so the UI/CLI/MCP tool couldn't distinguish "in progress" or "crashed/failed" from "never started" while a provider's init command ran. Adds a cross-process tri-state (not_initialized/initializing/initialized/failed) derived from an advisory file lock held for the duration of Exec.Init, so a crashed process self-heals to "failed" instead of silently reverting.
✅ Deploy Preview for images-devsy-sh canceled.
|
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughProvider initialization now records attempts and errors, prevents concurrent initialization, and resolves four lifecycle states. Workspace loading and CLI, MCP, and desktop outputs expose the resolved provider status. ChangesProvider initialization lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderInit as initProvider
participant ProviderState as ProviderConfig
participant Workspace as LoadAllProviders
participant Clients as CLI/MCP/Desktop
ProviderInit->>ProviderState: persist initialization attempt
ProviderInit->>ProviderState: persist success or truncated failure
Workspace->>ProviderState: resolve provider initialization state
Workspace->>Clients: provide provider status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
✅ Deploy Preview for devsydev canceled.
|
skevetter
changed the title
# Conflicts: # desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte # desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts # desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte # desktop/src/renderer/src/lib/components/provider/ProviderSheet.test.ts # desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte # desktop/src/renderer/src/lib/stores/providers.ts
No mutex needed around the config mutation in initProvider: the flock already serializes concurrent callers for the same provider, in-process or across processes, since each call opens an independent file description. Also trims verbose comments per review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/provider/configure_shared.go (1)
185-231: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
Initializedbefore re-running init, or a failed retry is reported asinitialized.
initProvidersetsentry.InitAttempted = trueand clearsentry.InitErrorbefore running the command, but it never resetsentry.Initialized.ResolveInitStateinpkg/provider/initlock.goreturnsInitStateInitializedas soon asstate.Initializedis true, before it ever looks atInitAttemptedorInitError.For a provider that previously initialized successfully, a later re-init attempt that fails leaves
Initialized=trueandInitError=<message>both persisted.ResolveInitStatewill still reportinitialized, so the failure is invisible todevsy provider list, theprovider_listMCP tool, and the desktop UI, even thoughInitErroris populated on disk.Reset
InitializedtofalsealongsideInitAttemptedandInitErrorbefore running the command, so a re-init in progress correctly resolves toinitializing, and a failed re-init correctly resolves tofailed.🐛 Proposed fix to reset Initialized on each attempt
entry.InitAttempted = true entry.InitError = "" + entry.Initialized = false if err := config.SaveConfig(devsyConfig); err != nil { return fmt.Errorf("save init state: %w", err) }Consider adding a regression test in
pkg/provider/initlock_test.gothat callsResolveInitStatewith&config.ProviderConfig{Initialized: true, InitError: "boom"}to lock in the intended precedence once fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/provider/configure_shared.go` around lines 185 - 231, Update initProvider to reset entry.Initialized to false alongside InitAttempted and InitError before saving state and running the initialization command, so retries resolve as initializing or failed rather than initialized. Preserve the existing success path that sets Initialized to true, and add a regression test in ResolveInitState coverage for an initialized provider with InitError to verify the failure state is reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/provider/configure_shared.go`:
- Around line 233-240: Update truncateInitError to truncate by Unicode rune
count rather than byte indices, while preserving the 500-rune maximum and
existing ellipsis behavior. Ensure the returned string remains valid UTF-8 when
multibyte characters occur at the truncation boundary.
In `@pkg/provider/initlock.go`:
- Around line 40-66: Update ResolveInitState to use the lock’s TryRLock() for
status probing instead of TryLock(), allowing concurrent read-only probes while
still failing when an exclusive initialization holds the lock. Preserve
TryLock() in initProvider for the active mutating initialization path and keep
the existing state and error handling unchanged.
---
Outside diff comments:
In `@cmd/provider/configure_shared.go`:
- Around line 185-231: Update initProvider to reset entry.Initialized to false
alongside InitAttempted and InitError before saving state and running the
initialization command, so retries resolve as initializing or failed rather than
initialized. Preserve the existing success path that sets Initialized to true,
and add a regression test in ResolveInitState coverage for an initialized
provider with InitError to verify the failure state is reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9a69a25-46b6-4770-af11-54e15b02656a
📒 Files selected for processing (9)
cmd/mcp/tools_provider.gocmd/provider/configure_shared.gocmd/provider/list.godesktop/src/main/watcher.tsdesktop/src/renderer/src/lib/types/index.tspkg/config/config.gopkg/provider/initlock.gopkg/provider/initlock_test.gopkg/workspace/provider.go
skevetter
changed the title
…ead lock for probes - initProvider left Initialized=true across a failed re-init, so a provider that previously succeeded reported "initialized" through a subsequent failed retry instead of "failed". Now reset alongside InitAttempted. - truncateInitError sliced by byte index, which can split a multi-byte rune and produce invalid UTF-8. Truncate by rune count instead. - ResolveInitState used an exclusive TryLock to merely probe the init lock, so two concurrent status reads could transiently report "initializing" for each other. Use TryRLock, which is compatible with other readers and still blocked by the exclusive lock initProvider holds while running.
1 participant
Summary
Initialized, so any process reading provider state (CLI, desktop UI, MCP tool) saw "not initialized" the entire timeExec.Initran, then "initialized" — with no visible in-between state, and no distinction between "never started" and "started but crashed/failed."not_initialized/initializing/initialized/failed) resolved viapkg/provider.ResolveInitState, using an advisory file lock (gofrs/flock, same pattern as existing workspace/task locks) held for the duration ofExec.Initas the liveness signal — self-healing tofailedif the initializing process crashes, since the OS releases the lock.devsy provider list(plain + JSON), theprovider_listMCP tool, and the desktop app (replacing a per-renderer-only "initializing" fake store with real backend-derived state inProviderCard/ProviderSheet).Test plan
go test ./pkg/provider/... ./cmd/provider/... ./pkg/workspace/... ./cmd/mcp/...— includes newpkg/provider/initlock_test.gocovering never-attempted, live-lock (initializing), and crash-then-free-lock (failed) cases.npx vitest runindesktop/(290 tests) andnpx svelte-check— both clean.Summary by CodeRabbit
New Features
Bug Fixes