fix(cli): reuse project stores for skill discovery#2102
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDashboard project-scoped plugin-skill discovery now reuses backend-aware stores, suppresses persistent runtime-state updates for temporary loaders, manages project-store shutdown, and makes PostgreSQL runtime-role creation race-safe. ChangesDashboard plugin-skill discovery
PostgreSQL role setup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Greptile SummaryThis PR fixes project-scoped plugin skill discovery for the CLI server surfaces. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "fix(cli): keep skill discovery loaders r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes dashboard skill discovery in PostgreSQL backend mode by ensuring project-scoped plugin skill discovery reuses backend-aware, per-project TaskStore instances (and their AsyncDataLayer) instead of constructing standalone SQLite-backed stores that now fail after VAL-REMOVAL-005.
Changes:
- Rework project-scoped plugin skill discovery to obtain plugin state via
TaskStore.getPluginStore()and reuse the dashboard’s per-project store cache. - Keep project stores cached for the dashboard process lifetime while still stopping request-scoped plugin loaders after metadata collection.
- Add a regression test that exercises the real Skills adapter callback wiring, plus update the dashboard test fixture (
getAsyncLayer()and plugin loader mocks).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli/src/commands/dashboard.ts | Reuses backend-aware per-project TaskStore/PluginStore for plugin skill discovery to avoid falling into removed SQLite runtime paths under PG. |
| packages/cli/src/commands/tests/dashboard.test.ts | Adds a regression test that calls the real getPluginSkills adapter callback and verifies no SQLite PluginStore is constructed. |
| .changeset/calm-skills-postgres.md | Adds a patch changeset describing the PostgreSQL-mode dashboard skill discovery fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/dashboard.ts (1)
993-1034: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftCache stampede, resource leaks, and in-flight lifecycle races.
The project-store cache suffers from a critical concurrency flaw where resolving the store without caching the Promise leads to severe resource leaks and database schema race conditions:
- Embedded PG Leak:
getProjectStorecaches the resolvedTaskStore. Concurrent requests for an uncached path will bypass the cache, boot multiple embedded PostgreSQL clusters, and permanently leak them when one overwrites the other inprojectStoresandprojectStoreShutdowns.- Teardown Race:
closeProjectStorestakes a synchronous snapshot ofprojectStoreShutdownswhilecreateTaskStoreForBackendpromises might still be in-flight. When the promise resolves, itsshutdowncallback is lost, leading toawait projectStore.close()which fails to stop the embedded cluster.- TOCTOU Schema Race:
await stateStore.init()ingetProjectScopedPluginSkillswill execute concurrently on the same cached store instance if multiple requests arrive, triggering the SQLite TOCTOU schema-migration race explicitly documented on lines 940-946.These can be fully resolved by enforcing a promise-based caching boundary that encompasses both the store boot and the satellite store initializations.
packages/cli/src/commands/dashboard.ts#L993-L1034: Cache the Promise of the boot result, ensure satellite stores are initialized within the same synchronized block, and await the inflight promises during teardown.packages/cli/src/commands/dashboard.ts#L1857-L1860: Remove the unprotectedawait stateStore.init()call since the promise-basedgetProjectStorenow safely handles initialization.🔒️ Proposed robust promise-based caching boundary
1. Apply this fix to the caching logic (lines 993-1034):
- const projectStores = new Map<string, TaskStore>(); - const projectStoreShutdowns = new Map<string, () => Promise<void>>(); + type CachedProjectStore = { taskStore: TaskStore; shutdown?: () => Promise<void> }; + const projectStores = new Map<string, Promise<CachedProjectStore>>(); let projectStoresClosePromise: Promise<void> | undefined; async function getProjectStore(projectPath: string): Promise<TaskStore> { - const cached = projectStores.get(projectPath); - if (cached) return cached; - let projectStore: TaskStore; - if (projectPath === cwd) { - if (!store) throw new Error("cwd TaskStore not yet initialized"); - projectStore = store; - } else { - const boot = await createTaskStoreForBackend({ rootDir: projectPath }); - if (boot) { - projectStore = boot.taskStore; - projectStoreShutdowns.set(projectPath, boot.shutdown); - } else { - projectStore = new TaskStore(projectPath); - await projectStore.init(); - } - } - projectStores.set(projectPath, projectStore); - return projectStore; + if (shutdownInProgress) throw new Error("Dashboard is shutting down"); + let promise = projectStores.get(projectPath); + if (!promise) { + promise = (async () => { + if (projectPath === cwd) { + if (!store) throw new Error("cwd TaskStore not yet initialized"); + return { taskStore: store }; + } + let projectStore: TaskStore; + let shutdown: (() => Promise<void>) | undefined; + const boot = await createTaskStoreForBackend({ rootDir: projectPath }); + if (boot) { + projectStore = boot.taskStore; + shutdown = boot.shutdown; + } else { + projectStore = new TaskStore(projectPath); + await projectStore.init(); + } + await projectStore.getPluginStore().init(); + return { taskStore: projectStore, shutdown }; + })(); + projectStores.set(projectPath, promise); + } + const cached = await promise; + return cached.taskStore; } async function closeProjectStores(): Promise<void> { projectStoresClosePromise ??= (async () => { - const stores = Array.from(projectStores.entries()).filter(([, projectStore]) => projectStore !== store); + const promises = Array.from(projectStores.values()); projectStores.clear(); - const shutdowns = new Map(projectStoreShutdowns); - projectStoreShutdowns.clear(); - await Promise.allSettled(stores.map(async ([projectPath, projectStore]) => { - const shutdown = shutdowns.get(projectPath); - if (shutdown) { - await shutdown(); - } else { - await projectStore.close(); - } + await Promise.allSettled(promises.map(async (promise) => { + try { + const { taskStore, shutdown } = await promise; + if (taskStore === store) return; + if (shutdown) { + await shutdown(); + } else { + await taskStore.close(); + } + } catch { + // Best-effort teardown: ignore initialization errors during shutdown + } })); })(); await projectStoresClosePromise; }2. Apply this fix to the plugin discovery logic (lines 1857-1860):
const scopedTaskStore = await getProjectStore(normalizedRootDir); const stateStore = scopedTaskStore.getPluginStore(); - await stateStore.init(); const enabledPlugins = await stateStore.listPlugins({ enabled: true });🤖 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 `@packages/cli/src/commands/dashboard.ts` around lines 993 - 1034, Replace the resolved-store cache in getProjectStore with promise-based caching that synchronizes boot and all satellite-store initialization before publishing the result; track and await in-flight promises during closeProjectStores so shutdown callbacks cannot be lost and each store is closed exactly once. At packages/cli/src/commands/dashboard.ts lines 1857-1860, remove the direct await stateStore.init() in getProjectScopedPluginSkills because initialization is now handled by getProjectStore’s synchronized promise boundary.
🤖 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 `@packages/core/src/postgres/migrations/0006_project_ownership.sql`:
- Around line 36-42: Replace the database-local pg_advisory_xact_lock in the
migration with a cluster-wide bootstrap mechanism for creating fusion_runtime,
or make the CREATE ROLE operation idempotent and retryable on duplicate-role
failures. Ensure concurrent project database initialization cannot fail due to
races when creating the shared PostgreSQL role.
---
Outside diff comments:
In `@packages/cli/src/commands/dashboard.ts`:
- Around line 993-1034: Replace the resolved-store cache in getProjectStore with
promise-based caching that synchronizes boot and all satellite-store
initialization before publishing the result; track and await in-flight promises
during closeProjectStores so shutdown callbacks cannot be lost and each store is
closed exactly once. At packages/cli/src/commands/dashboard.ts lines 1857-1860,
remove the direct await stateStore.init() in getProjectScopedPluginSkills
because initialization is now handled by getProjectStore’s synchronized promise
boundary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e564aa24-0c31-474a-bc33-419bf10458e2
📒 Files selected for processing (6)
.changeset/calm-skills-postgres.mdpackages/cli/src/commands/__tests__/dashboard.test.tspackages/cli/src/commands/dashboard.tspackages/core/src/__tests__/plugin-hot-reload.test.tspackages/core/src/plugin-loader.tspackages/core/src/postgres/migrations/0006_project_ownership.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/calm-skills-postgres.md
- packages/cli/src/commands/tests/dashboard.test.ts
plarson
force-pushed
the
fix/postgres-skills-discovery
branch
from
5e63447 to
8423208
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/__tests__/plugin-hot-reload.test.ts (1)
314-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
getPlugininstead of type casting to access the installation state.Using
(mockPluginStore as any)bypasses TypeScript's type checking to access internal properties. You can retrieve the updated state cleanly by using the publicgetPluginmethod provided by the mock store.♻️ Proposed refactor
await pluginLoader.loadPlugin("hot-reload-test"); - expect((mockPluginStore as any)._installation.state).toBe("installed"); + expect((await mockPluginStore.getPlugin("hot-reload-test")).state).toBe("installed"); await pluginLoader.stopAllPlugins(); expect(pluginLoader.isPluginLoaded("hot-reload-test")).toBe(false); - expect((mockPluginStore as any)._installation.state).toBe("installed"); + expect((await mockPluginStore.getPlugin("hot-reload-test")).state).toBe("installed");🤖 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 `@packages/core/src/__tests__/plugin-hot-reload.test.ts` around lines 314 - 320, Update the installation-state assertions in the hot-reload test to retrieve the plugin through the mock store’s public getPlugin method instead of casting mockPluginStore to any and accessing _installation directly. Preserve the existing "installed" expectations before and after stopAllPlugins.
🤖 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.
Nitpick comments:
In `@packages/core/src/__tests__/plugin-hot-reload.test.ts`:
- Around line 314-320: Update the installation-state assertions in the
hot-reload test to retrieve the plugin through the mock store’s public getPlugin
method instead of casting mockPluginStore to any and accessing _installation
directly. Preserve the existing "installed" expectations before and after
stopAllPlugins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe2e4139-1ccc-404a-9b1c-3adb646cc481
📒 Files selected for processing (6)
.changeset/calm-skills-postgres.mdpackages/cli/src/commands/__tests__/dashboard.test.tspackages/cli/src/commands/dashboard.tspackages/core/src/__tests__/plugin-hot-reload.test.tspackages/core/src/plugin-loader.tspackages/core/src/postgres/migrations/0006_project_ownership.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/calm-skills-postgres.md
- packages/cli/src/commands/tests/dashboard.test.ts
- packages/core/src/plugin-loader.ts
- packages/cli/src/commands/dashboard.ts
plarson
force-pushed
the
fix/postgres-skills-discovery
branch
from
8423208 to
8558de2
Compare
plarson
force-pushed
the
fix/postgres-skills-discovery
branch
from
8558de2 to
cf2a9d3
Compare
This was referenced Jul 15, 2026
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
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.
Summary
TaskStorecache during project-scoped plugin skill discoveryTaskStore.getPluginStore()instead of constructing bare SQLite-defaultPluginStore/TaskStoreinstancesgetAsyncLayer()Root cause
GET /api/skills/discoveredresolved the project correctly, thengetProjectScopedPluginSkills()constructed new stores without anAsyncDataLayer. AfterVAL-REMOVAL-005, that enters the physically removed synchronous SQLite runtime and returns HTTP 500 even when PostgreSQL health, projects, tasks, and both project engines are healthy.The existing route tests mocked the Skills adapter callback, so they did not exercise this CLI wiring.
Verification
pnpm lintpnpm --filter @runfusion/fusion typecheckpnpm --filter @runfusion/fusion buildpnpm check:changesets --strictgit diff --checkLive Atlas validation against the migrated embedded PostgreSQL runtime:
/api/skills/discovered?projectId=proj_84f4645c2da64288: HTTP 200, 36 skills/api/skills/discovered?projectId=proj_7538a9dd46c24c5f: HTTP 200, 36 skillsSummary by CodeRabbit
fusion_runtimerole creation race-safe during concurrent PostgreSQL migrations.persistRuntimeStateoption to control whether plugin runtime state changes are persisted.