Run project services as core actors - #272
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughProject services are migrated from spawned child processes to in-process ChangesIn-process project service lifecycle migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AimuxDaemon
participant CoreProjectActor
participant Multiplexer
Client->>AimuxDaemon: ensureProject(projectId)
AimuxDaemon->>AimuxDaemon: check projectActors map
alt actor missing or stopped
AimuxDaemon->>CoreProjectActor: start()
CoreProjectActor->>Multiplexer: startProjectServiceHost()
Multiplexer-->>CoreProjectActor: ready
CoreProjectActor-->>AimuxDaemon: state(pid, projectRoot)
else actor already running
AimuxDaemon->>CoreProjectActor: getState()
end
AimuxDaemon-->>Client: project state
sequenceDiagram
participant RuntimeCoherence
participant ProjectService
participant Paths
RuntimeCoherence->>ProjectService: GET /health
ProjectService-->>RuntimeCoherence: pid + projectStateDir
RuntimeCoherence->>Paths: getProjectStateDirFor(projectRoot)
RuntimeCoherence->>RuntimeCoherence: compare expected vs actual projectStateDir
alt mismatch
RuntimeCoherence-->>RuntimeCoherence: mark service "mismatch"
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/daemon.ts (1)
640-661: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftWait for project-actor cleanup before exiting.
src/main.tscallsdaemon.stop(); process.exit(0);, butstop()firesactor.stop()without awaiting it. Sinceactor.stop()now does async cleanup (mux.cleanup()/removeMetadataEndpoint()), shutdown can cut that work off. Make the shutdown path async and await all actor stops before exiting.🤖 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 `@src/daemon.ts` around lines 640 - 661, The daemon shutdown path currently starts project actor cleanup without waiting for it, so async teardown can be interrupted by process exit. Update the stop flow in daemon.stop() to be async and await all actor.stop() calls (for example by collecting and awaiting them before clearing state and closing the server), then adjust the call site in main.ts to await daemon.stop() before process.exit(0). Use daemon.stop, actor.stop, and the shutdown logic around this.projectActors and this.server as the key places to update.
🧹 Nitpick comments (2)
src/multiplexer/session-launch.ts (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
projectRootForhelper across files.The same fallback logic (
host.projectRoot?.trim() || process.cwd()) is reimplemented independently here and, per graph evidence, inpersistence-methods.ts,runtime-lifecycle-methods.ts,services.ts, anddashboard-model.ts. Since this exact resolution logic is the crux of the whole PR (avoidingprocess.cwd()misuse in the multi-project daemon), consider extracting it to a single shared utility (e.g.src/multiplexer/project-root.ts) so future changes to the fallback behavior don't need to be replicated in five places.♻️ Suggested consolidation
+// src/multiplexer/project-root.ts +export function projectRootFor(host: { projectRoot?: unknown }): string { + return typeof host.projectRoot === "string" && host.projectRoot.trim() ? host.projectRoot.trim() : process.cwd(); +}Then import this from
session-launch.ts,persistence-methods.ts,runtime-lifecycle-methods.ts,services.ts, anddashboard-model.tsinstead of redefining it locally.🤖 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 `@src/multiplexer/session-launch.ts` around lines 33 - 36, The `projectRootFor` fallback logic is duplicated across `session-launch.ts`, `persistence-methods.ts`, `runtime-lifecycle-methods.ts`, `services.ts`, and `dashboard-model.ts`; extract it into one shared utility such as `project-root.ts` and update each of those helpers to import and use the shared function instead of redefining the same `host.projectRoot?.trim() || process.cwd()` behavior locally.src/multiplexer/services.ts (1)
23-26: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCentralize
projectRootForinstead of re-implementing it per file.This exact helper (name and body) is independently re-implemented in
src/multiplexer/persistence-methods.ts(Lines 67-70) with the sameprocess.cwd()fallback, and again insrc/multiplexer/runtime-lifecycle-methods.ts(Lines 122-125) — but there the fallback isgetRepoRoot()instead ofprocess.cwd(). Per graph evidence,dashboard-model.tsdefines yet another copy. Sinceprocess.cwd()is a single global value shared by the whole daemon process whilegetRepoRoot()is context-aware (via the new async-local project path context), these fallbacks are not equivalent — a future divergence (e.g. an actor host withoutprojectRootset) would silently behave differently depending on which file's copy runs, which is exactly the class of bug the async-local project-path-context work in this PR is meant to prevent for a daemon hosting multipleCoreProjectActors concurrently.Suggest exporting a single
projectRootFor/resolveProjectRoothelper fromsrc/paths.ts(built ongetRepoRoot()) and importing it everywhere instead of re-declaring it.♻️ Suggested consolidation
-function projectRootFor(host: ServiceHost): string { - return typeof host.projectRoot === "string" && host.projectRoot.trim() ? host.projectRoot.trim() : process.cwd(); -} +import { projectRootFor } from "../paths.js";🤖 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 `@src/multiplexer/services.ts` around lines 23 - 26, The helper `projectRootFor` is duplicated across multiple multiplexer modules with inconsistent fallbacks, so centralize it into one shared resolver and reuse that everywhere. Export a single `projectRootFor` or `resolveProjectRoot` from `src/paths.ts` built on the async-local repo-root lookup (`getRepoRoot()`), then replace the local copies in `services`, `persistence-methods`, `runtime-lifecycle-methods`, and the `dashboard-model` clone with imports of the shared helper. Keep the `ServiceHost`-based call sites using the shared function so all code paths resolve the same project root consistently.
🤖 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 `@src/core-project-actor.ts`:
- Around line 41-59: The CoreProjectActor.start startup flow can leave a live
Multiplexer behind if startProjectServiceHost() fails after this.mux is set.
Update CoreProjectActor.start to wrap the withProjectPaths startup block in
try/catch, and when an error occurs call cleanup() on the created Multiplexer
before rethrowing so a failed start does not orphan a host or get reused on
retry.
---
Outside diff comments:
In `@src/daemon.ts`:
- Around line 640-661: The daemon shutdown path currently starts project actor
cleanup without waiting for it, so async teardown can be interrupted by process
exit. Update the stop flow in daemon.stop() to be async and await all
actor.stop() calls (for example by collecting and awaiting them before clearing
state and closing the server), then adjust the call site in main.ts to await
daemon.stop() before process.exit(0). Use daemon.stop, actor.stop, and the
shutdown logic around this.projectActors and this.server as the key places to
update.
---
Nitpick comments:
In `@src/multiplexer/services.ts`:
- Around line 23-26: The helper `projectRootFor` is duplicated across multiple
multiplexer modules with inconsistent fallbacks, so centralize it into one
shared resolver and reuse that everywhere. Export a single `projectRootFor` or
`resolveProjectRoot` from `src/paths.ts` built on the async-local repo-root
lookup (`getRepoRoot()`), then replace the local copies in `services`,
`persistence-methods`, `runtime-lifecycle-methods`, and the `dashboard-model`
clone with imports of the shared helper. Keep the `ServiceHost`-based call sites
using the shared function so all code paths resolve the same project root
consistently.
In `@src/multiplexer/session-launch.ts`:
- Around line 33-36: The `projectRootFor` fallback logic is duplicated across
`session-launch.ts`, `persistence-methods.ts`, `runtime-lifecycle-methods.ts`,
`services.ts`, and `dashboard-model.ts`; extract it into one shared utility such
as `project-root.ts` and update each of those helpers to import and use the
shared function instead of redefining the same `host.projectRoot?.trim() ||
process.cwd()` behavior locally.
🪄 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
Run ID: 920c0f9a-7a0c-4a3b-8da8-84928b087be0
📒 Files selected for processing (15)
src/core-project-actor.tssrc/daemon.test.tssrc/daemon.tssrc/main.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-model.tssrc/multiplexer/index.tssrc/multiplexer/persistence-methods.tssrc/multiplexer/runtime-lifecycle-methods.tssrc/multiplexer/services.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/paths.test.tssrc/paths.ts
💤 Files with no reviewable changes (1)
- src/paths.test.ts
|
Independent review finding fixed in 1829932: legacy standalone project services now must exit before a CoreProjectActor starts; wedged legacy services are SIGKILLed and failure to exit aborts startup instead of creating dual writers. |
|
Independent review finding fixed in 1829932: project-service health verification now requires the expected projectStateDir in both the CLI wait path and runtime coherence report, so a healthy same-daemon endpoint cannot verify the wrong project. |
|
Independent review finding fixed in 1829932: CoreProjectActor startup failures now stop/delete the failed actor before retry, preventing half-started actor state from poisoning future ensures. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
src/main.ts (1)
793-797: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis is a runtime/CLI behavior change (project-service kind removed, daemon lifecycle reworked). Ensure
dist/is regenerated so the shipped build matches source;yarn typecheck/yarn vitestalone won't update it.As per coding guidelines: "For aimux runtime or CLI behavior changes, run
yarn buildto updatedist/; source-level validation withyarn vitestandyarn typecheckis not enough".🤖 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 `@src/main.ts` around lines 793 - 797, The runtime/CLI behavior change in loggingProcessKind and the daemon command path handling requires regenerating the shipped build output, since source checks alone won’t update dist. After updating the source behavior, run yarn build so dist/ is rebuilt and stays in sync with the changes; y arn typecheck and yarn vitest are not sufficient for this kind of CLI/runtime change.Source: Coding guidelines
🧹 Nitpick comments (2)
src/runtime-coherence.ts (1)
219-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate object literal between mismatch and ok branches.
The mismatch report (Lines 222-231) and the success report (Lines 232-240) share nearly every field (
daemonState,endpoint,pid,process,serviceInfo). Consider extracting a shared base object and overriding onlystatus/errorto reduce duplication.♻️ Proposed refactor
const expectedProjectStateDir = getProjectStateDirFor(input.projectRoot); const projectStateDir = typeof json?.projectStateDir === "string" ? json.projectStateDir : null; - if (projectStateDir !== expectedProjectStateDir) { - return { - status: "mismatch", - daemonState: null, - endpoint: input.endpoint, - pid: typeof json?.pid === "number" ? json.pid : input.endpoint.pid, - process: null, - serviceInfo, - error: `projectStateDir mismatch: expected ${expectedProjectStateDir} actual ${projectStateDir ?? "unknown"}`, - }; - } - return { - status: manifestsMatch(input.expected, serviceInfo) ? "ok" : "mismatch", - daemonState: null, - endpoint: input.endpoint, - pid: typeof json?.pid === "number" ? json.pid : input.endpoint.pid, - process: null, - serviceInfo, - error: null, - }; + const base = { + daemonState: null, + endpoint: input.endpoint, + pid: typeof json?.pid === "number" ? json.pid : input.endpoint.pid, + process: null, + serviceInfo, + } as const; + if (projectStateDir !== expectedProjectStateDir) { + return { + ...base, + status: "mismatch", + error: `projectStateDir mismatch: expected ${expectedProjectStateDir} actual ${projectStateDir ?? "unknown"}`, + }; + } + return { + ...base, + status: manifestsMatch(input.expected, serviceInfo) ? "ok" : "mismatch", + error: null, + };🤖 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 `@src/runtime-coherence.ts` around lines 219 - 240, The mismatch and success returns in runtime-coherence share the same object shape, so reduce duplication by extracting the common result fields into a shared base object and overriding only the varying status/error values. Update the logic around the projectStateDir check and the manifestsMatch result so the two branches reuse the same base data while keeping the existing behavior in the return object.src/main.ts (1)
227-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract the duplicated respawn/reattach block.
The pid-mismatch block (Lines 216-226) and this new projectStateDir block are near-identical (respawn-once, else remove endpoint, sleep, continue). Extracting a small helper (e.g.,
respawnOrReattach()) would remove the duplication and keep the two gates in sync.🤖 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 `@src/main.ts` around lines 227 - 247, The projectStateDir mismatch handling in main.ts duplicates the same respawn/reattach flow used by the pid-mismatch branch. Extract that shared logic into a small helper such as respawnOrReattach() and call it from both the existing pid check and this health.projectStateDir check, keeping the lastError/log.warn details specific while centralizing the respawnAttempted, stopProjectService, removeMetadataEndpoint, ensureProjectService, and delay/continue sequence.
🤖 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 `@src/main.ts`:
- Around line 227-247: The project service health check currently treats a
missing projectStateDir as a mismatch, which can incorrectly trigger respawn and
hide a version mismatch. Update the logic in the health verification block
around the projectStateDir comparison in main.ts so it only compares
expectedProjectStateDir against health.projectStateDir when that field is
actually present; keep the existing respawn path for real mismatches, but ignore
omitted values so older services can proceed to the manifest check and surface
ProjectServiceVersionError.
---
Outside diff comments:
In `@src/main.ts`:
- Around line 793-797: The runtime/CLI behavior change in loggingProcessKind and
the daemon command path handling requires regenerating the shipped build output,
since source checks alone won’t update dist. After updating the source behavior,
run yarn build so dist/ is rebuilt and stays in sync with the changes; y
arn typecheck and yarn vitest are not sufficient for this kind of CLI/runtime
change.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 227-247: The projectStateDir mismatch handling in main.ts
duplicates the same respawn/reattach flow used by the pid-mismatch branch.
Extract that shared logic into a small helper such as respawnOrReattach() and
call it from both the existing pid check and this health.projectStateDir check,
keeping the lastError/log.warn details specific while centralizing the
respawnAttempted, stopProjectService, removeMetadataEndpoint,
ensureProjectService, and delay/continue sequence.
In `@src/runtime-coherence.ts`:
- Around line 219-240: The mismatch and success returns in runtime-coherence
share the same object shape, so reduce duplication by extracting the common
result fields into a shared base object and overriding only the varying
status/error values. Update the logic around the projectStateDir check and the
manifestsMatch result so the two branches reuse the same base data while keeping
the existing behavior in the return object.
🪄 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
Run ID: 450baa78-17bc-476c-9d8c-2aae1a1780fb
📒 Files selected for processing (5)
src/daemon.test.tssrc/daemon.tssrc/main.tssrc/runtime-coherence.test.tssrc/runtime-coherence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/daemon.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core-project-actor.test.ts (1)
37-63: 📐 Maintainability & Code Quality | 🔵 TrivialMissing coverage for idempotent
start()andstop().The only test covers the failure→cleanup→retry-success path. Per the upstream contract (
core-project-actor.ts:41-74),start()is guarded byif (this.started) return this.getState();, and the AI summary states the class also implements an asyncstop(). Neither idempotent-start (second call while running should not re-invokemux.startProjectServiceHost) norstop()behavior is exercised here. Given the PR explicitly fixes related lifecycle bugs (stopping wedged legacy services, cleaning up failed actors before retry), test coverage forstop()and idempotency would materially increase confidence in this critical lifecycle class.🤖 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 `@src/core-project-actor.test.ts` around lines 37 - 63, Add coverage in CoreProjectActor tests for the lifecycle guards on CoreProjectActor.start() and the async stop() method. The current test only verifies the failure cleanup and retry path; add a case that calls start() twice while already running and asserts mux.startProjectServiceHost (or the equivalent start mock) is only invoked once and getState is returned on the second call, plus a stop() test that verifies cleanup/state transitions and that repeated stop() calls are safe or no-op as implemented.
🤖 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 `@src/core-project-actor.test.ts`:
- Around line 37-63: Add coverage in CoreProjectActor tests for the lifecycle
guards on CoreProjectActor.start() and the async stop() method. The current test
only verifies the failure cleanup and retry path; add a case that calls start()
twice while already running and asserts mux.startProjectServiceHost (or the
equivalent start mock) is only invoked once and getState is returned on the
second call, plus a stop() test that verifies cleanup/state transitions and that
repeated stop() calls are safe or no-op as implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ab74e9f-a89b-40f6-8393-f572c3483736
📒 Files selected for processing (2)
src/core-project-actor.test.tssrc/core-project-actor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core-project-actor.ts
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes