fix: report engine available when another fusion process owns it - #1704
Conversation
The dashboard's engine-availability health check only counted engines this process started. A second launch (e.g. `pnpm dev dashboard` alongside an already-running `fusion`) is correctly refused the per-machine engine singleton lock, so its engine map stays empty and the dashboard showed a false "engine not running" banner even though an engine was live on the machine. ProjectEngineManager now records projects whose singleton lock is held by another process (via EngineAlreadyRunningError) and exposes hasRunningEngine(), which the health endpoint consults so the banner reflects machine-level truth. Reconciliation still retries so this process takes over if the other exits, and the "refusing to start" log fires once per project instead of on every 30s reconciliation tick. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit cb64e87 on Jun 21, 2026 10:35am UTC. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesEngine banner fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes a false "engine not running" banner by teaching
Confidence Score: 3/5Mostly safe — the core bug fix is correct and well-tested, but
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant D as Dashboard Process
participant PEM as ProjectEngineManager
participant SL as Engine Singleton Lock
participant EP as External (Owner) Process
participant UI as /api/health
EP->>SL: acquireEngineSingleton(projectId) ✓ (owns lock)
D->>PEM: startAll() / reconcile()
PEM->>SL: acquireEngineSingleton(projectId)
SL-->>PEM: EngineAlreadyRunningError
Note over PEM: externalEngines.add(projectId)<br/>log warning once
PEM-->>D: throws EngineAlreadyRunningError (swallowed)
D->>UI: GET /api/health
UI->>PEM: hasRunningEngine()
PEM-->>UI: "true (externalEngines.size > 0)"
UI-->>D: "{ engine: { available: true } }"
Note over EP: External process exits, releases lock
D->>PEM: "reconcile() — retries because has(projectId)=false"
PEM->>SL: acquireEngineSingleton(projectId) ✓ (lock now free)
Note over PEM: externalEngines.delete(projectId)
PEM->>PEM: engine.start()
PEM->>PEM: engines.set(projectId, engine)
Note over PEM: This process now owns the engine
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant D as Dashboard Process
participant PEM as ProjectEngineManager
participant SL as Engine Singleton Lock
participant EP as External (Owner) Process
participant UI as /api/health
EP->>SL: acquireEngineSingleton(projectId) ✓ (owns lock)
D->>PEM: startAll() / reconcile()
PEM->>SL: acquireEngineSingleton(projectId)
SL-->>PEM: EngineAlreadyRunningError
Note over PEM: externalEngines.add(projectId)<br/>log warning once
PEM-->>D: throws EngineAlreadyRunningError (swallowed)
D->>UI: GET /api/health
UI->>PEM: hasRunningEngine()
PEM-->>UI: "true (externalEngines.size > 0)"
UI-->>D: "{ engine: { available: true } }"
Note over EP: External process exits, releases lock
D->>PEM: "reconcile() — retries because has(projectId)=false"
PEM->>SL: acquireEngineSingleton(projectId) ✓ (lock now free)
Note over PEM: externalEngines.delete(projectId)
PEM->>PEM: engine.start()
PEM->>PEM: engines.set(projectId, engine)
Note over PEM: This process now owns the engine
|
…or != no engine) Document the false "engine not running" banner root cause and fix as a docs/solutions learning, and add an "Engine Singleton Lock" entry to CONCEPTS.md: a failed per-machine lock acquisition is proof an engine is running elsewhere, not "no engine." Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/project-engine-manager.ts (1)
468-488:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale external ownership as soon as the singleton is acquired.
If a previous attempt marked the project external, then the other process exits,
acquireEngineSingleton()succeeds, butengine.start()fails, Line 488 never runs andhasRunningEngine()keeps reporting an engine that no longer exists.🐛 Proposed fix to clear the marker after singleton acquisition
}); this.singletonLocks.set(projectId, singleton); + // Acquiring the singleton proves any previously observed external owner is gone. + this.externalEngines.delete(projectId); const engine = new ProjectEngine( runtimeConfig, this.centralCore, engineOptions, @@ this.engines.set(projectId, engine); this.starting.delete(projectId); - // We now own the engine — clear any prior "owned by another process" marker. - this.externalEngines.delete(projectId); runtimeLog.log( `Started engine for ${project.name ?? projectId} (${projectId})`, );🤖 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/engine/src/project-engine-manager.ts` around lines 468 - 488, The externalEngines marker is cleared after a successful engine.start() call, but if engine.start() fails and throws an error, the marker never gets cleared, causing hasRunningEngine() to report stale external ownership. Move the this.externalEngines.delete(projectId) call to immediately after this.singletonLocks.set(projectId, singleton) is executed, so the marker is cleared as soon as the singleton is acquired, before any engine startup attempt. This ensures that even if the subsequent engine.start() call fails, the stale external ownership marker will not persist.
🤖 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/dashboard/src/__tests__/server.test.ts`:
- Around line 418-435: Add a new regression test that covers the legacy fallback
branch where the hasRunningEngine method is not available on the engineManager.
Create a test case similar to "reports the engine available when another fusion
process owns it" but instead remove the hasRunningEngine mock entirely and set
getAllEngines to return a non-empty Map (indicating a running engine exists).
Verify that the GET request to "/api/health" still returns status 200 with
engine.available set to true. This ensures the backward-compat fallback logic in
hasDashboardEngine works correctly when the newer hasRunningEngine method is
missing from the engineManager.
In `@packages/dashboard/src/server.ts`:
- Around line 485-489: The comment block explaining the hasRunningEngine
behavior starting around line 485 needs to be converted to follow the FNXC
requirement-trace format for consistency with repository coding guidelines. Add
a FNXC-formatted header comment preceding the existing behavior explanation
using the format FNXC:<Area-of-product> yyyy-MM-dd-hh:mm: (replacing the
placeholders with appropriate values), and keep the existing descriptive comment
about how hasRunningEngine counts both owned and detected fusion process engines
as the body of the requirement trace. This ensures the behavior documentation
follows the repo's standard for requirement history tracking.
In `@packages/engine/src/__tests__/project-engine-manager.test.ts`:
- Around line 747-770: The current test only exercises the direct ensureEngine()
call path and verifies that the refusal warning is logged once, but it does not
test the reconciliation wrapper that periodically attempts to start the engine.
Modify this test to drive the reconciliation path by using vi.useFakeTimers() to
simulate time passing and allow the reconciliation mechanism to trigger multiple
intervals, then verify that both the "Refusing to start engine for proj_aaa"
warning and any "Failed to start engine..." warning from the outer
reconciliation layer remain deduplicated and logged only once. Use fake timer
advancement (vi.advanceTimersByTime) instead of real polling to meet coding
guidelines for regression tests.
In `@packages/engine/src/project-engine-manager.ts`:
- Around line 54-62: Add the required FNXC requirement trace to the
externalEngines field documentation. In the JSDoc comment block for the
externalEngines Set, include an FNXC comment in the format
FNXC:<Area-of-product> yyyy-MM-dd-hh:mm: to document this user-facing behavior
change regarding dashboard banner reporting. Apply the same FNXC trace addition
to the related code block at lines 122-128. The FNXC comment should be placed
within the existing documentation to mark this as an important UX/behavior
requirement.
- Around line 455-464: The EngineAlreadyRunningError is being rethrown even
after being identified as an external engine, causing the outer reconciliation
catch block to emit a "Failed to start engine..." warning on every interval.
After logging the "Refusing to start engine..." message and recording the
projectId in the externalEngines set within the deduplication check, do not
rethrow the error. Instead, return early from the current operation or use early
exit logic so that the exception does not propagate to the outer catch block
that treats it as an unexpected failure requiring a warning on each
reconciliation tick.
---
Outside diff comments:
In `@packages/engine/src/project-engine-manager.ts`:
- Around line 468-488: The externalEngines marker is cleared after a successful
engine.start() call, but if engine.start() fails and throws an error, the marker
never gets cleared, causing hasRunningEngine() to report stale external
ownership. Move the this.externalEngines.delete(projectId) call to immediately
after this.singletonLocks.set(projectId, singleton) is executed, so the marker
is cleared as soon as the singleton is acquired, before any engine startup
attempt. This ensures that even if the subsequent engine.start() call fails, the
stale external ownership marker will not persist.
🪄 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: 4b2bfa92-01f7-4e39-903d-5ad47a7c082f
📒 Files selected for processing (5)
.changeset/engine-not-running-banner.mdpackages/dashboard/src/__tests__/server.test.tspackages/dashboard/src/server.tspackages/engine/src/__tests__/project-engine-manager.test.tspackages/engine/src/project-engine-manager.ts
- Stop reconciliation/startAll/onProjectAccessed from warning every tick for externally-owned engines: swallow EngineAlreadyRunningError in the outer catches (it's expected and already logged once in createAndStart) - Add FNXC:DashboardHealth requirement-trace comments on the externalEngines field and the dashboard hasRunningEngine health check - Add regression test: reconciliation stays quiet across ticks for an externally-owned engine (inner refusal logged once, outer failure suppressed) - Add regression test: hasDashboardEngine legacy fallback to getAllEngines when hasRunningEngine is unavailable on the manager Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…quire Move externalEngines.delete(projectId) to immediately after acquiring the singleton lock instead of after engine.start() succeeds. If a project was marked external, the holder exits, acquire succeeds, but start() then throws, the success-path delete never ran and hasRunningEngine() reported a phantom engine forever. Added a regression test for the failed-takeover path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addressed in |
- Fix frontmatter enums (root_cause: logic_error, component: tooling); add #1699 to related_prs and last_updated - Reflect post-review behavior: external marker cleared at lock-acquire time; reconcile/startAll/onProjectAccessed swallow EngineAlreadyRunningError - Add rejected approach (don't merge has()/hasRunningEngine) and the keep-them-distinct invariant - Tighten socket-name detail (sha1[:16]); cross-link the browser-testing doc Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What & why
Starting the dashboard with
pnpm dev dashboard --no-authshowed an "engine not running" banner even though an engine was running. The banner is wrong, not the engine.The engine uses a per-machine singleton lock (
engine-singleton-lock.ts): only one fusion process may run the engine for a given project. When a second process launches (a dev dashboard alongside an already-runningfusion, a leftoverpnpm fusion, a daemon, etc.), it is correctly refused the lock withEngineAlreadyRunningError— which is positive proof an engine is live.The bug: that proof was thrown away.
ProjectEngineManager.createAndStartlogged "Refusing to start" and rethrew without recording that an engine exists, and the dashboard health checkhasDashboardEngineonly counted engines this process owns (getAllEngines()). Result:/api/healthreportedengine.available: false→ banner. Reconciliation then retried every 30s, re-colliding and re-logging each tick.Surfaced by
feat: start engines by default(#1699), which made the dashboard actively attempt engine startup.Changes
ProjectEngineManagertracks anexternalEnginesset — projects whose lock is owned by another process — populated onEngineAlreadyRunningError, cleared when this process takes over and instopAll(). NewhasRunningEngine()andgetExternalEngineIds().hasDashboardEngine(dashboardserver.ts) consultshasRunningEngine()(with a fallback for older managers / test doubles), so the banner reflects machine-level truth. Reconciliation still retries, so this process takes over if the lock-holder exits.Tests
project-engine-manager.test.ts: reports a running engine when the lock is held elsewhere; logs the refusal only once across repeated attempts; takes over and clears the external marker once the lock frees.server.test.ts:/api/healthreportsavailable: truewhen another process owns the engine.These paths had no prior coverage — tests assumed the manager always owns what it starts.
Verification
tsc --noEmitcleanSummary by CodeRabbit
Release Notes
Bug Fixes
/api/healthso engine availability reflects externally running engines, not only engines owned by the current process.Improvements
Documentation