Skip to content

fix: harden control-plane health diagnostics#71

Open
RaviTharuma wants to merge 2 commits intoAVIDS2:mainfrom
RaviTharuma:fix/control-plane-health
Open

fix: harden control-plane health diagnostics#71
RaviTharuma wants to merge 2 commits intoAVIDS2:mainfrom
RaviTharuma:fix/control-plane-health

Conversation

@RaviTharuma
Copy link
Copy Markdown
Contributor

Summary

  • add shared control-plane helpers for the real HTTP health endpoint and process identification
  • fix memorix doctor to probe the live HTTP control-plane endpoint instead of /health
  • prevent memorix background status from misclassifying a real memorix serve-http PID as an unrelated reused process

Verification

  • npm test -- --run tests/cli/control-plane-shared.test.ts
  • npm test -- --run tests/integration/serve-http.test.ts
  • npm run build
  • npm run lint

Linked issue

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 68800cc2ff

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if (process.platform === 'linux') {
return execSync(`tr '\0' ' ' < /proc/${pid}/cmdline`, { encoding: 'utf-8' }).trim();
}
return execSync(`ps -p ${pid} -o command=`, { encoding: 'utf-8' }).trim();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Replace Unix-only process lookup for non-Linux platforms

The non-Linux branch runs ps -p ... -o command= to identify whether the PID belongs to memorix serve-http, but on Windows this command is not available under the default cmd.exe shell. In that environment isMemorixBackgroundProcess will always hit the catch path and return false, so background status can mislabel a real unhealthy Memorix PID as “reused” and delete background.json, leaving the running control plane unmanaged.

Useful? React with 👍 / 👎.

Comment on lines +11 to +15
const timer = setTimeout(() => controller.abort(), timeoutMs);
const res = await fetch(`http://127.0.0.1:${port}${CONTROL_PLANE_HEALTH_PATH}`, {
signal: controller.signal,
});
clearTimeout(timer);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear health-check abort timer on failure paths

The timeout is only cleared after a successful fetch, so when the request fails fast (for example, ECONNREFUSED) the function returns from catch but leaves a pending timer alive until timeoutMs elapses. Since doctor now uses this shared helper for its probes, failed checks can keep the CLI process running for an extra 2–3 seconds per probe even though the network failure happened immediately.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix doctor/background control-plane health diagnostics for HTTP mode

1 participant