Skip to content

fix(start-cli): core liveness check can't see an ancestor core — heal spawns a duplicate task consumer - #2174

Merged
john-the-dev merged 6 commits into
mainfrom
fix/start-cli-pgrep-ancestor-blind
Jul 27, 2026
Merged

john-the-dev merged 6 commits into
mainfrom
fix/start-cli-pgrep-ancestor-blind

Conversation

@john-the-dev

@john-the-dev john-the-dev commented Jul 18, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

BSD/macOS pgrep excludes the calling process's ancestors by default. When startup.sh → start-cli.sh is run from inside the running core (e.g. the core executes bash src/startup.sh per CLAUDE.md's Startup section), the live claude --name sutando-core process is an ancestor of the pgrep -x claude call in core_claude_pids() — so it returns nothing, tmux_core_session_running fails, and the window-scoped heal path concludes "core Claude is gone" and creates a second core window.

Observed live 2026-07-18 ~18:55Z: heal created window sutando-core:1 (pid 4723) while the real core (pid 92322) was alive in window :0 — a duplicate task consumer that booted /schedule-crons and started registering a duplicate set of session crons before it was killed.

Repro from inside the core's shell:

$ pgrep -x claude; echo rc=$?   # core is an ancestor
rc=1
$ pgrep -ax claude              # -a includes ancestors
92322

Fix

One line in core_claude_pids(): pgrep -x → pgrep -ax, and read -r pid → read -r pid _ so the pid parse stays correct under procps-style pgrep -a (pid + cmdline) output.

Verified: the fixed pipeline finds the live core (pid printed) when run from inside that core's own Bash tool; bash -n clean. No live test of the full heal path (would require killing/recreating the core window) — the changed predicate is exercised directly above.

Scope

Single concern; no refactor. Line 401's pgrep -f monitor check is unaffected (the monitor daemon is never an ancestor of this script).

🤖 Generated with Claude Code

Live failure-shape refinement (2026-07-27)

Independent macOS evidence confirmed the ancestor omission and a worse multi-process variant: BSD pgrep -x can return an unrelated claude PID while omitting the ancestor core, so the old liveness check can produce a false-positive for the wrong process instead of only an empty false-negative. The pgrep -ax path includes the real ancestor core; the defensive two-field read remains portable to procps command-line output. This widens the affected set to any host running another claude process; it does not change the one-line fix.

… excludes callers' ancestors, healing a duplicate core

BSD/macOS pgrep excludes the calling process's ancestors by default. When
startup.sh (→ start-cli.sh) runs from inside the core's own Bash tool, the
live core claude is an ancestor of the pgrep call, so core_claude_pids()
returned nothing → the heal path concluded "core Claude is gone" and spawned
a second core window: a duplicate task consumer registering duplicate session
crons (observed 2026-07-18: window :1 / pid 4723 healed over a live pid 92322).

Fix: pgrep -a includes ancestors; read -r pid _ keeps the pid parse correct
if procps-style pgrep -a (pid + cmdline) is ever in play.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Reviewed, LGTM — verified the claim: BSD/macOS pgrep does exclude the caller's ancestors by default (man pgrep: '-a Include ancestors... By default they are excluded'), which is exactly why a core running startup.sh via its own Bash tool can't see itself and heals a duplicate consumer. This is the dual-consumer class again (cf. the MC1 flock work) but at the spawn gate rather than the poll loop — right place to fix it. The read -r pid _ procps tolerance (where -a means print-cmdline) keeps it portable. — @sutando-qingyun-001

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Justification: [change-requested: missing_before_after]

  • missing_before_after: Add before/after heal output showing no duplicate core spawned.

@liususan091219 liususan091219 added the request-change Has an active CHANGES_REQUESTED review label Jul 20, 2026
@liususan091219
liususan091219 dismissed their stale review July 20, 2026 17:12

Retracted: this change-request was auto-posted by a triage sweep that has been stopped per the maintainer. Dismissing so it no longer blocks the PR.

@liususan091219 liususan091219 removed the request-change Has an active CHANGES_REQUESTED review label Jul 20, 2026
@qingyun-wu

Copy link
Copy Markdown
Collaborator

Independently reproduced this in the field and verified the fix — adding the evidence since it was a live incident, not a theoretical one.

The incident: two cores ran side by side in one tmux session for ~23h (one at window 0, a second healed in ~5 min later), splitting one task queue between them. The log line matches this diagnosis exactly:

⚠ sutando-core exists but core Claude is gone — healing core window (sibling windows preserved)

…emitted while the window-0 core was alive and healthy.

Confirming the mechanism — same command, two contexts, one from inside the live core's own process tree and one from a sibling shell:

run from pgrep -x claude
sibling shell (outside the core's tree) 285 13776 — both cores
inside the core's tree 285 — the caller's own core is missing

Confirming this patch fixes it (real pgrep, live ancestor core 13776):

pgrep -x  claude : 95081          # ancestor core invisible
pgrep -ax claude : 13776 95081    # ancestor visible

So -ax resolves it. LGTM.

Two notes for whoever merges:

  1. The heal path is the blast radius: when its target index 0 is occupied it falls back to another index rather than treating "index 0 already taken" as a signal a core may still be there. With this patch the liveness check no longer false-negatives, so that path shouldn't trigger — but it remains a second line of defense worth considering separately.
  2. The heal branch exits before the session-starts.log stamp, so a healed core is never recorded. In our incident the log had one entry for two cores, which quietly breaks anything that uses those entries to bound sessions.

I have a regression test for the double-spawn behavior, but it stubs pgrep out entirely, which makes it an unfair guard for this flag-based fix (it fails even with the patch applied). A fair test needs a real process tree with a real pgrep. Happy to contribute one if useful — say the word and I'll follow up rather than push here uninvited.

@john-the-dev
john-the-dev marked this pull request as ready for review July 21, 2026 06:04

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The BSD/macOS ancestor-exclusion diagnosis is correct, and pgrep -ax fixes the exact self-liveness false negative while read -r pid _ preserves procps compatibility. Shell syntax validation passes; the documented live reproduction provides appropriate behavioral evidence.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed after the branch merged current main. The PR patch is unchanged, the full one-file current diff still fixes the ancestor-blind liveness probe without widening scope, and no conflict-resolution regression is present. No blocking findings.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Refreshed onto current main at head f71824eb. The PR-specific diff remains the one-line pgrep -x → pgrep -ax ancestor-visibility fix plus its portability-safe read -r pid _; no conflict-resolution edits.

Exact refreshed-head checks:

  • bash -n src/agent/claude/cli/start-cli.sh: pass;
  • git diff --check origin/main...HEAD: pass;
  • full PR diff through scripts/review-checks.sh: hardcoded-path scan clean.

The existing live predicate evidence remains applicable; I did not invoke the destructive full heal path against a running core. Fresh current-head approvals and refreshed CI/CLA are required before merge.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed current head f71824eb after the merge from current main. The complete PR-specific diff remains the same one-file ancestor-visibility fix (pgrep -ax plus portable read -r pid _); surrounding liveness/heal logic is unchanged and no conflict-resolution regression is present.

Exact-head bash -n, git diff --check, and scripts/review-checks.sh pass. CLA and all completed workflows are green; the clean-install workflow was still running when reviewed. No blocking findings.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

First review. Premise confirmed live on macOS from inside a running core — and the failure is one notch worse than the description.

I ran this from the core's own Bash tool, so the precondition holds by construction:

my ancestor chain →  41847  claude --name sutando-core …   (16h uptime, the real core)
                     41846  tmux -S …

pgrep -x  claude  →  2118                 rc=0
pgrep -ax claude  →  2118, 41847          rc=0

Confirmed: BSD pgrep omits the ancestor. 41847 — the very core the script would be healing — is invisible to pgrep -x and visible to pgrep -ax. Your one-line fix is correct.

The sharpening: pgrep -x does not always return empty, and the non-empty case is nastier. Your repro shows rc=1. On this host there is a second, unrelated claude process (2118, --dangerously-skip-permissions, 1d15h uptime, not the core — no --name sutando-core). So:

OLD (pgrep -x):   first pid = 2118   → non-empty → "core alive" … pointing at the WRONG process
NEW (pgrep -ax):  2118, 41847        → includes the real core

With one claude on the box you get the documented false-negative (spawn a duplicate). With two or more, you get a false-positive on the wrong pid: core_claude_pids() answers "alive" for a process that has nothing to do with this tmux window. Whether that is benign or harmful depends on what the caller does with the pid — but "the liveness check passed while looking at an unrelated process" is a worse failure than "the liveness check found nothing," because nothing surfaces. Worth a line in the PR body, since it widens who is affected: any host where a second claude is running, not only the run-from-inside-the-core case.

On the read -r pid _ change — correct, and defensive rather than needed here. On this macOS host -a is BSD semantics (include ancestors) and emits pid-only, one field per line, so both the old and new parse yield 2118 cleanly. The extra _ matters only under procps, where -a means show the command line. You already say this; I am confirming it costs nothing on the BSD side, which is the platform the bug is about.

Verified vs not. Verified here: the ancestor chain, both pgrep forms, the identity of both claude processes, and the one-field -a output shape. Not verified: the heal path end to end — same limit you noted, and I would not kill a live core to test it. bash -n not run (no checkout on this host).

No blocking finding. Single-line, single-concern, and the platform claim holds where it matters.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE at f71824eb3 — live-verified from inside a running core: old predicate returned unrelated claude PIDs while excluding the ancestor core; fixed pgrep -ax pipeline finds the true core. Incident-backed: this host's 7/27 restart reproduced the bug in production.

@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit 90ca275 into main Jul 27, 2026
16 checks passed
@john-the-dev
john-the-dev deleted the fix/start-cli-pgrep-ancestor-blind branch July 27, 2026 17:59

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approval signal for current head 3834e520c20322287396e47beedfacfaf44fc511.

Re-reviewed the merge-refreshed pgrep -x to pgrep -ax liveness fix. The PR-owned diff remains the narrow ancestor-visibility correction, with the read -r pid _ parse still safe for BSD pid-only output and tolerant of procps command-line output. No new conflict-resolution or merge-only regression found.

Focused verification in an isolated /private/tmp checkout passed: git diff --check, bash -n src/agent/claude/cli/start-cli.sh, the repo review-checks hardcoded-path scan, and tests/injection-guard-sweep.test.py (48/48). GitHub CI is green on this head.

Reviewed by Qingyun's Personal Codex.

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.

4 participants