fix(agent-core): probe pid liveness before marking restored tasks lost#2190
fix(agent-core): probe pid liveness before marking restored tasks lost#2190Nas01010101 wants to merge 1 commit into
Conversation
markLoadedTasksLost reclassified every non-terminal ghost task as lost without checking whether its process was still running. When a second process resumed a session whose tasks were still alive, the first process tasks were marked lost, and the resume hint tells the model a lost task may be resumed, which starts a duplicate worker alongside the live one. Process tasks already persist their pid, so probe it with process.kill(pid, 0) and leave a live task untouched. Closes MoonshotAI#1924
🦋 Changeset detectedLatest commit: 1ccc3d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ccc3d39ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // kimi-code process that is still driving it. Marking it `lost` tells the | ||
| // model it may resume the task, which starts a second worker alongside | ||
| // the first one. | ||
| if (info.kind === 'process' && pidAlive(info.pid)) continue; |
There was a problem hiding this comment.
When a second process restores the session while the original task is alive, this continue leaves the nonterminal task only in ghosts. Production invokes reconcile() once from restoreAfterReplay; afterward getTask() and wait() return the cached ghost without reloading its persisted record. When the owning process later writes completed or failed, the resumed process therefore reports the task as running forever, TaskOutput remains not_ready, and no terminal notification is emitted. Skipped ghosts need an ownership/status refresh mechanism so they converge after the original task terminates.
Useful? React with 👍 / 👎.
| export function pidAlive(pid: number): boolean { | ||
| if (!Number.isInteger(pid) || pid <= 0) return false; | ||
| try { | ||
| process.kill(pid, 0); |
There was a problem hiding this comment.
For a stale running record left by a crash, the OS may have reused its PID for an unrelated process before the session is restored. kill(pid, 0) then returns success even though the recorded task is dead, causing reconciliation to suppress the lost state and recovery indefinitely. Persist and compare a process-birth or owner-instance identity in addition to the numeric PID.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,24 @@ | |||
| /** | |||
| * Liveness probe for persisted task pids. | |||
There was a problem hiding this comment.
The new helper header starts with a free-form label instead of the required `<domain>` domain (Ln) — <role> identity line, so its layer and external responsibility are not documented consistently. Rewrite the first line in the scoped format and consolidate the later function and branch comments into that header.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L11-L15
Useful? React with 👍 / 👎.
| // A ghost whose OS process is still running belongs to another live | ||
| // kimi-code process that is still driving it. Marking it `lost` tells the | ||
| // model it may resume the task, which starts a second worker alongside | ||
| // the first one. |
There was a problem hiding this comment.
This newly added explanatory block is beside method statements, while the scoped convention requires comments to live solely in the top-of-file header and forbids comments beside functions, methods, or statements. Fold the externally relevant rationale into the existing module header and leave this control flow self-explanatory.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L11-L15
Useful? React with 👍 / 👎.
1 participant
Related Issue
Resolve #1924
Problem
See linked issue.
In short: when a session is restored,
markLoadedTasksLostreclassifies every non-terminal ghost task aslostwithout checking whether the task's process is still running. If a second process resumes a session whose tasks are still alive, the first process's tasks are markedlost— and theresume_hintliststask.lostas a recovery case that the model may resume, so a duplicate worker gets started alongside the one that is still running.The material for the check is already on disk:
ProcessTaskInfocarriespid, andpersist.tsround-trips it.What changed
packages/agent-core-v2/src/agent/task/pidAlive.ts: aprocess.kill(pid, 0)probe.ESRCHmeans dead;EPERMmeans the process exists but belongs to another user, so it counts as alive; any other errno assumes alive so a live task is never clobbered. This mirrors the probe already used by the server-side instance registry (packages/kap-server/src/instanceRegistry.ts).markLoadedTasksLostnow skips ghosts of kindprocesswhose pid is still alive. Everything else is unchanged: terminal statuses are still skipped, dead process tasks are still markedlost, and non-process kinds (which have no pid to probe) keep their previous behaviour.Scope note: this is the small, self-contained half of the problem. It does not add a per-session cross-process lock, and agent-kind tasks still have no pid to probe — those were raised as larger design options in the issue and are deliberately left out of this PR.
Testing
test/agent/task/pidAlive.test.tscovers the probe directly: current pid alive, non-positive/non-integer pids rejected,ESRCHdead,EPERMalive, unknown errno alive, and that it probes with signal0so the target is never disturbed.test/agent/task/reconcile.test.tsgains two regression tests: a ghost whose process is alive staysrunningand emits notask.terminated, and with one live and one dead ghost only the dead one is markedlost.pid: 1, which is alive on Linux). They now mock the probe to a dead pid by default, so their expectations are deterministic and unchanged.markLoadedTasksLostfails exactly the two new reconcile tests, with the original nine still passing.pnpm testforpackages/agent-core-v2/test/agent/task/: 200 passed.pnpm lint,pnpm typecheckandpnpm sherifare clean. Three plugin test files (app/plugin/archive,manager,manager-consumption, 11 tests) fail identically on unmodifiedmainin my environment, so they are pre-existing and unrelated.Checklist
gen-changesetsskill, or this PR needs no changeset. — patch changeset added for@moonshot-ai/kimi-code, since this internal-package fix changes user-visible CLI behaviour.gen-docsskill, or this PR needs no doc update. — no documented behaviour changes;lostkeeps its meaning, it is just no longer applied to a live task.