Refactor daemon supervisor boundary#280
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe daemon module is split into ChangesDaemon module split
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer as core-command-client/runtime-restart
participant Supervisor as daemon-supervisor.ts
participant State as daemon-state.ts
participant Daemon as Spawned Daemon Process
Consumer->>Supervisor: ensureDaemonRunning(options)
Supervisor->>State: loadDaemonInfo()
State-->>Supervisor: AimuxDaemonInfo or null
alt daemon info valid and healthy
Supervisor->>Daemon: GET /health
Daemon-->>Supervisor: health status
else no valid daemon
Supervisor->>Supervisor: acquire startup lock
Supervisor->>Daemon: spawn(command)
Supervisor->>Daemon: poll /health until match
Supervisor->>State: saveDaemonInfo(info)
Supervisor->>Supervisor: release startup lock
end
Supervisor-->>Consumer: AimuxDaemonInfo
Consumer->>Supervisor: requestDaemonJson(path)
Supervisor->>State: loadDaemonInfo()
Supervisor->>Daemon: HTTP request
Daemon-->>Supervisor: JSON response
Supervisor-->>Consumer: parsed result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 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/daemon-state.ts`:
- Around line 111-126: `loadDaemonState` can still throw when `loadJson` returns
valid JSON that is `null` or a non-object, because `raw.projects` and
`raw.updatedAt` are accessed without validating `raw` first. Update
`loadDaemonState` to defensively verify the parsed value from `loadJson` is an
object with the expected shape before reading its fields, and fall back to the
default daemon state when it is `null` or malformed; keep the existing
`projects` filtering logic intact.
In `@src/daemon-supervisor.ts`:
- Around line 97-116: The lock acquisition in tryAcquireDaemonStartLock is not
fully serialized because the lock directory can be observed before owner.json is
safely published. Update the acquire path so owner.json is written atomically
only after the lock is unquestionably owned, and make the stale-lock check in
readLockPid/tryAcquireDaemonStartLock ignore or retry while owner.json is
missing or incomplete instead of deleting the directory immediately. Keep the
fix localized to tryAcquireDaemonStartLock and the owner.json publication logic
so concurrent daemon starts cannot both proceed.
🪄 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: a0eede7d-4208-4a24-ac91-a5b1d9b2783e
📒 Files selected for processing (14)
src/core-command-client.test.tssrc/core-command-client.tssrc/core-sidecar-boundary.test.tssrc/daemon-state.tssrc/daemon-supervisor.tssrc/daemon.test.tssrc/daemon.tssrc/main.tssrc/mobile-push-bridge.test.tssrc/mobile-push-bridge.tssrc/multiplexer/persistence-methods.tssrc/one-shot-node-inventory.test.tssrc/runtime-coherence.tssrc/runtime-restart.ts
Comment on lines
+111
to
+126
| export function loadDaemonState(): DaemonState { | ||
| const raw = loadJson<DaemonState>(getDaemonStatePath(), { | ||
| version: 1, | ||
| updatedAt: new Date(0).toISOString(), | ||
| projects: {}, | ||
| }); | ||
| const projects: Record<string, ProjectServiceState> = {}; | ||
| for (const [projectId, entry] of Object.entries(raw.projects ?? {})) { | ||
| if (entry) projects[projectId] = entry; | ||
| } | ||
| return { | ||
| version: 1, | ||
| updatedAt: raw.updatedAt, | ||
| projects, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
loadDaemonState can throw on a corrupt/null state file.
loadJson only returns the fallback on a missing file or a JSON.parse throw. A file containing valid JSON null (or any non-object) parses successfully and is cast to DaemonState, after which raw.projects / raw.updatedAt dereference null and throw TypeError. The ?? {} guard doesn't help because raw.projects is evaluated before the coalesce.
🛡️ Proposed null-safe fix
- const projects: Record<string, ProjectServiceState> = {};
- for (const [projectId, entry] of Object.entries(raw.projects ?? {})) {
+ const projects: Record<string, ProjectServiceState> = {};
+ for (const [projectId, entry] of Object.entries(raw?.projects ?? {})) {
if (entry) projects[projectId] = entry;
}
return {
version: 1,
- updatedAt: raw.updatedAt,
+ updatedAt: raw?.updatedAt ?? new Date(0).toISOString(),
projects,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function loadDaemonState(): DaemonState { | |
| const raw = loadJson<DaemonState>(getDaemonStatePath(), { | |
| version: 1, | |
| updatedAt: new Date(0).toISOString(), | |
| projects: {}, | |
| }); | |
| const projects: Record<string, ProjectServiceState> = {}; | |
| for (const [projectId, entry] of Object.entries(raw.projects ?? {})) { | |
| if (entry) projects[projectId] = entry; | |
| } | |
| return { | |
| version: 1, | |
| updatedAt: raw.updatedAt, | |
| projects, | |
| }; | |
| } | |
| export function loadDaemonState(): DaemonState { | |
| const raw = loadJson<DaemonState>(getDaemonStatePath(), { | |
| version: 1, | |
| updatedAt: new Date(0).toISOString(), | |
| projects: {}, | |
| }); | |
| const projects: Record<string, ProjectServiceState> = {}; | |
| for (const [projectId, entry] of Object.entries(raw?.projects ?? {})) { | |
| if (entry) projects[projectId] = entry; | |
| } | |
| return { | |
| version: 1, | |
| updatedAt: raw?.updatedAt ?? new Date(0).toISOString(), | |
| projects, | |
| }; | |
| } |
🤖 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-state.ts` around lines 111 - 126, `loadDaemonState` can still
throw when `loadJson` returns valid JSON that is `null` or a non-object, because
`raw.projects` and `raw.updatedAt` are accessed without validating `raw` first.
Update `loadDaemonState` to defensively verify the parsed value from `loadJson`
is an object with the expected shape before reading its fields, and fall back to
the default daemon state when it is `null` or malformed; keep the existing
`projects` filtering logic intact.
Comment on lines
+97
to
+116
| function tryAcquireDaemonStartLock(): string | null { | ||
| const lockPath = daemonStartLockPath(); | ||
| mkdirSync(dirname(lockPath), { recursive: true }); | ||
| const acquire = (): string | null => { | ||
| try { | ||
| mkdirSync(lockPath); | ||
| writeFileSync(join(lockPath, "owner.json"), `${JSON.stringify({ pid: process.pid })}\n`); | ||
| return lockPath; | ||
| } catch (error) { | ||
| if ((error as { code?: string }).code !== "EEXIST") throw error; | ||
| return null; | ||
| } | ||
| }; | ||
| const acquired = acquire(); | ||
| if (acquired) return acquired; | ||
| const pid = readLockPid(lockPath); | ||
| if (pid && isPidAlive(pid)) return null; | ||
| rmSync(lockPath, { recursive: true, force: true }); | ||
| return acquire(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for other callers/paths that rely on the daemon-start lock and any existing serialization guarantees
rg -nP -C3 'daemonStartLockPath|tryAcquireDaemonStartLock|releaseDaemonStartLock|owner\.json' srcRepository: TraderSamwise/aimux
Length of output: 9584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the daemon supervisor lock/startup flow and related tests.
sed -n '84,130p' src/daemon-supervisor.ts
printf '\n----\n'
sed -n '296,360p' src/daemon-supervisor.ts
printf '\n----\n'
rg -n -C3 'probeDefaultDaemon|EADDRINUSE|listen\\(' src/daemon-supervisor.ts src/**/*.test.tsRepository: TraderSamwise/aimux
Length of output: 3864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the daemon startup branch to see whether the lock is the only guard
# or whether later probe/listen checks prevent a duplicate spawn.
sed -n '240,360p' src/daemon-supervisor.tsRepository: TraderSamwise/aimux
Length of output: 4062
Make owner.json publication atomic in tryAcquireDaemonStartLock A contender can still see the lock directory before owner.json exists or is fully written, treat the lock as stale, delete it, and acquire the lock itself. The pre-spawn probe and EADDRINUSE path don’t serialize startup, so overlapping daemon starts are still possible.
🤖 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-supervisor.ts` around lines 97 - 116, The lock acquisition in
tryAcquireDaemonStartLock is not fully serialized because the lock directory can
be observed before owner.json is safely published. Update the acquire path so
owner.json is written atomically only after the lock is unquestionably owned,
and make the stale-lock check in readLockPid/tryAcquireDaemonStartLock ignore or
retry while owner.json is missing or incomplete instead of deleting the
directory immediately. Keep the fix localized to tryAcquireDaemonStartLock and
the owner.json publication logic so concurrent daemon starts cannot both
proceed.
This was referenced Jul 4, 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
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.
Summary
Verification
Summary by CodeRabbit
Bug Fixes
Refactor
Tests