Skip to content

fix(runtime): recover a live daemon from a damaged locator - #6892

Merged
huangruiteng merged 6 commits into
loopx-project:mainfrom
JasonBuildAI:codex/effect-runtime-locator-recovery-e4v7
Oct 22, 2026
Merged

huangruiteng merged 6 commits into
loopx-project:mainfrom
JasonBuildAI:codex/effect-runtime-locator-recovery-e4v7

Conversation

@JasonBuildAI

@JasonBuildAI JasonBuildAI commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Goal And Delivered Outcome

  • Outcome basis / optional anchor: effect runtime: a corrupt locator spawns a second daemon and doctor reports stopped #6871 — a corrupt Effect runtime locator silently cold-starts a second daemon while loopx doctor reports the runtime as stopped.
  • Goal/source and gap: Make locator inspection distinguish "absent" from "present but invalid", keep the diagnostic and recorded-PID liveness in the lifecycle readback, and reuse an already-running daemon before cold start. Before this change, _read_info collapsed JSON/schema/fingerprint/field failures into None; callers treated that as "no runtime", and doctor had no way to show the live recorded PID.
  • Observable before → after, with the validation row that proves it: With only the live locator's schema_version corrupted, before: doctor stopped, diagnostic null, a new daemon started while the old PID stayed alive and serving. After: doctor running, locator_state=invalid, recorded_pid_alive=true, diagnostic runtime_locator_schema_mismatch, and the next RPC reuses the original PID. The real-daemon test in tests/control_plane/test_effect_runtime_locator_recovery.py proves this end to end.
  • Issue/task and intended base: Fixes effect runtime: a corrupt locator spawns a second daemon and doctor reports stopped #6871. Intended base: loopx-project/loopx:main at 764b989dc.

Author Declaration

  • Written by: model_agent (OpenAI Codex) at human direction and review.
    AI disclosure: this change was drafted with AI assistance at my request, under my direction and review.
    The problem and constraints were set by the human requester; this PR is submitted for human review.

Implemented against

Criterion (spec clause) Disposition Symbol / path Test or command
Distinguish "no locator" from "locator present but unreadable" implemented _inspect_info, _RuntimeLocatorRead in loopx/control_plane/effect_runtime.py tests/control_plane/test_effect_runtime_locator_recovery.py
Keep a diagnostic code implemented _RuntimeLocatorDiagnostic; lifecycle diagnostic_code same test; collect_effect_runtime_readiness
Doctor reports PID liveness alongside locator state implemented collect_effect_runtime_readiness, render_doctor_markdown doctor markdown assertions in the same test
Before cold-starting, reuse a live daemon that still answers on the recorded port/token implemented _recover_runtime_info, _runtime_info_for_request real-daemon schema-corruption test
Do not trust damaged bytes without proof implemented authenticated runtime.ping must match PID, fingerprint and locator path same test
Malformed bytes do not crash doctor implemented UnicodeError/JSON/type guards in _inspect_info malformed JSON and invalid UTF-8 tests
  • Self-check before submission: Read the issue, reproduced the defect against current main, and confirmed the old behavior before changing code. Deliberately left out unrelated refactors and new dependencies. A locator whose token/endpoint are destroyed cannot be authenticated safely; that path cold-starts and retains the runtime_locator_unreadable diagnostic.

Scope And Continuation

  • Completed scope and remaining work: complete within this scope. The issue's corrupt-locator path is fixed; no successor is needed.
  • Slice boundary / successor: N/A. The change stays in the existing Python Effect runtime adapter; the TypeScript server protocol and locator schema are unchanged.
  • Review follow-up (4b340d9e6): a deep probe now refreshes the locator projection after it may have started the runtime, preserves a damaged-locator diagnostic on success, and reports host-permission or generic locator read failures as invalid rather than missing. Focused tests cover both outcomes.
  • Second review follow-up (607a787ce): recovery now propagates a host-permission denial instead of treating it as no runtime and cold-starting, and readiness inspects locator state/PID liveness even when the Node probe is unavailable. Regressions cover both boundaries.
  • Third review follow-up (9dadb773d): out-of-range recorded PIDs are rejected before the OS liveness probe and platform overflow is contained, so damaged locator bytes cannot crash readiness or the installation-only doctor. API and real console-command regressions cover the boundary.
  • Reporter follow-up (1940477fb): added explicit recovery tests for a ping from a different PID, a rejected token, a different fingerprint, and a different published locator path. Each case asserts that the damaged locator is not reused.

Validation

  • Tested revision: 1940477fb7913811db0315547d02ceb82647af8a (all listed checks ran on the final tree).
  • Run state: finished
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
static passed uv run --extra test ruff check ... → All checks passed!; git diff --check 764b989dce...HEAD clean.
unit passed uv run --extra test pytest -q tests/control_plane/test_effect_runtime_locator_recovery.py tests/control_plane/test_effect_runtime_host_permission.py tests/control_plane/test_turn_journal_runtime_readiness.py → 51 passed in 56.58s.
integration passed Effect runtime/readiness suite over 10 files → 220 passed, 7 skipped, 3 warnings in 152.15s; skips are the platform-gated cases already present in the suite.
real_entrypoint passed Real Node daemon through effect_runtime.effect_runtime_result; corrupting only schema_version reports running/invalid/PID alive and reuses the same PID; loopx doctor markdown readback includes the diagnostic.
real_backend passed Node.js managed runtime over loopback in an isolated temporary runtime directory; no external service or private state.
regression_parity passed New recovery tests failed 2 before the fix and pass after; malformed JSON and invalid UTF-8 retain runtime_locator_unreadable instead of looking absent. Follow-up tests prove deep success refreshes absent → valid and preserves damaged → invalid, while host-permission and generic read errors remain visible.
regression_parity passed The second review regressions failed 2 before the follow-up (must not cold start; locator_state was missing) and pass after; the tests prove host-permission recovery does not launch a replacement and Node-unavailable readiness still reports a valid live locator.
regression_parity passed The oversized-PID API and console-command regressions failed 2 before 9dadb773d (liveness was incorrectly reported as False) and pass after; both retain locator_state=invalid with unknown liveness and structured doctor JSON.
regression_parity passed The four reporter-requested identity rejections pass: different PID, rejected token, different fingerprint, and different published locator path all return no recoverable runtime, so a damaged locator cannot route a request to another daemon.
static passed Semantic inventory advisory reports only the local _RuntimeLocatorState / _RuntimeLocatorDiagnostic aliases for the branch; decision: remain local to effect_runtime.py, extending the existing loopx_effect_runtime_lifecycle_v0 readback; no registered vocabulary or shared contract change.
static failed uv run --extra test python -m mypy --follow-imports=silent loopx/control_plane/effect_runtime.py loopx/doctor.py exits 1 with the same 48 errors and 4 unique diagnostic shapes as base 764b989dc; this branch adds no new normalized mypy diagnostic.
integration failed Exact-head premerge standard against the original PR base 764b989dc: all direct checks, module ceiling, 8/8 risk-profile smokes and public boundary passed. Four catalog canaries failed and each was reproduced independently on that base: install-local-smoke (WinError 1314, symlink privilege), codex-cli-packaged-install-smoke (Bash unavailable), loopx-update-smoke (next_action=None), and semantic-vocabulary-drift-smoke (44 twins vs budget 43).
  • Coverage and gaps: The real-daemon test covers schema corruption with an intact endpoint/token, malformed JSON, invalid UTF-8, and a dead recorded PID; it also verifies restart stops the original daemon. Four focused negative tests pin the recovery identity checks for PID, token, fingerprint and locator path. A locator that loses its token/port cannot be authenticated and intentionally cold-starts. No live Lark/frontend path is involved; this is a CLI/runtime readback change.

Frontend / Visual Evidence

  • UI impact: none
  • Before: N/A
  • After: N/A
  • First viewport and preview: N/A
  • States and viewports shown: N/A
  • Source data: none
  • Attention review: The change adds two doctor text rows (locator_state, recorded_pid_alive) to the existing runtime lifecycle block; no new screen, hero or CTA.
  • Craft review: N/A (no visual surface).

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring (no functional changes)
  • Documentation update
  • Test update

LoopX Area

  • Control plane (goals, todos, quota, scheduler, registry, runtime)
  • Benchmark boundary (adapters, runners, verifiers, scoring, evidence)
  • Capability or extension (providers, adapters, skills)
  • Public docs or presentation surface (README, protocols, dashboard)
  • Build, packaging, installer, or CI
  • Host or runtime integration

Technical Direction

Shared-authority RFC fixture impact

  • Production-scale fixture schema: N/A
  • Semantic dimensions changed, or reviewed no-impact rationale: local locator diagnostic vocabulary only; no shared-authority fixture change.
  • Provider conformance arms run: N/A
  • Read-only legacy/file/PostgreSQL three-arm rehearsal (required for promotion, runtime-routing, or compatibility-projection changes): N/A

Boundary Checklist

  • Neither the diff nor this PR body/comments/attachments disclose private state, credentials, raw traces or verifier output, internal links, or local machine paths (including .loopx/, .codex/goals/, and live ACTIVE_GOAL_STATE.md).
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the linked issue/task.
  • Existing English/Chinese RFC pairs (including ledger entries) both have a diff, and the local premerge check python3 scripts/check_rfc_bilingual.py --base origin/main --head HEAD passes, or no paired RFC changed.
  • I completed the visual evidence section for UI changes, or marked UI impact none.
  • Every commit includes a DCO Signed-off-by trailer (git commit -s).

A damaged locator was collapsed into no runtime, so the next request cold-started a second daemon while doctor reported stopped. Distinguish a missing locator from an unreadable one, retain the diagnostic and recorded PID liveness in the lifecycle projection, and reuse a recorded endpoint only after an authenticated ping proves the same PID, fingerprint and locator path.

Signed-off-by: JasonBuildAI <jasonbuildai@gmail.com>
Explain locator_state and recorded_pid_alive so operators can distinguish an absent locator from a damaged one that still authenticates a live runtime.

Signed-off-by: JasonBuildAI <jasonbuildai@gmail.com>

@JasonBuildAI JasonBuildAI left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review Verdict: Changes Requested

P2 — exact-head readback is stale after a deep probe can successfully start or reuse the runtime. In loopx/control_plane/effect_runtime.py:1484-1495, the deep path refreshes runtime_identity but uses the pre-probe locator_state and recorded_pid_alive, then forcibly sets diagnostic_code to None. I reproduced this with an isolated real runtime: collect_effect_runtime_readiness(deep=True) returned state=running, locator_state=missing, recorded_pid_alive=None, while the runtime had just published a valid v0 locator with a live PID. The same path also erases runtime_locator_schema_mismatch/runtime_locator_unreadable after a successful deep probe even though the locator remains damaged. The issue requires doctor readback to distinguish and retain these facts; the deep mode does not. Recompute the locator projection after the semantic probe and preserve the locator diagnostic when the locator is still invalid; add deep=True coverage.

P2 — a present but permission-denied locator is still reported as locator_state=missing. locator_state is initialized to missing at line 1351, and the EffectRuntimeHostPermissionError handler at lines 1379-1383 never changes it. An isolated PermissionError probe returned diagnostic_code=runtime_host_permission_denied together with locator_state=missing. On POSIX, a directory at the locator path follows the generic OSError branch in _inspect_info and is also classified as present=False. This re-collapses the missing/invalid distinction that the issue is intended to establish. Classify the locator as invalid when its read is denied or the path exists but is not a readable file, and cover that state in the readiness tests.

P2 delivery blocker — hosted exact-head CI is not green. The open PR head bf3bdad6c29ec46395c0371998abac2ade1e5424 currently has Release Artifacts / build failed. The failure is in personal-workspace-contract.test.mjs (Lark App count does not flash a false zero while loading), which is unrelated to this diff, but several required matrix jobs are still pending and merge readiness is blocked. This is not a defect in the locator change; it is a gate that must be resolved or explicitly dispositioned before merge.

Answers To The Six Review Questions

  1. Real value: Yes, but narrow. An operator with a corrupt/restored locator gets a silent orphan daemon and a false stopped health result under the current failure mode. This changes that to authenticated reuse plus a visible diagnostic. Normal users are unaffected; the value is defensive operational recovery, not a common-path feature.
  2. Scope: The change stays within the existing runtime adapter, doctor projection, focused tests, and installation docs. The _serving_runtime_identity and restart call-site updates are necessary companion paths, not unrelated refactoring. I found no opportunistic changes.
  3. Logic bugs / boundaries: The main authenticated-reuse path is sound: it requires a live recorded PID, loopback endpoint/token, matching fingerprint, and an authenticated runtime.ping with PID/path identity. The two projection defects above are the substantive boundary failures. I did not find a duplicate-daemon path in the tested healthy-locator scenario.
  4. Abstraction/design: Proportionate. The local _RuntimeLocatorRead, _RuntimeLocatorState, and _RuntimeLocatorDiagnostic remain owned by effect_runtime.py; the existing runtime.ping is reused and no new shared protocol/registry is introduced. The implementation is not over-abstracted for the reported behavior.
  5. Do the tests prove the fix: They prove the headline case strongly: a real daemon is started, only schema_version is corrupted, the next RPC reuses the same PID, restart stops it, and malformed JSON/invalid UTF-8/dead PID are classified. They do not cover the two failing projections above (deep=True refresh/diagnostic retention and permission-denied state), nor the negative authenticated-ping cases such as wrong token/PID/fingerprint. Those gaps allowed the reproduced defects through.
  6. Checks: I reran the focused suite (21 passed), integration suite (97 passed), Ruff (All checks passed!), and the exact mypy command (49 diagnostic lines, 7 unique messages, identical on base). The semantic inventory reported only the two local aliases. The full standard canary reproduced 14 selected / 4 failures, with 8/8 risk-profile smokes and public-boundary passing; I independently confirmed the four failures: Windows symlink privilege, missing Bash, the base next_action=None assertion, and 44 vs 43 vocabulary twins (the latter two also fail on base). git diff --check is clean. The packet's local-test claims are supported; the hosted Release Artifacts build failure above is not covered by the packet.

Lens Checks

  • Typed state rules: local locator vocabulary, additive to the existing lifecycle projection; no new shared owner.
  • Domain neutrality: runtime-locator wording is appropriately local and not benchmark/product-specific.
  • Behavior-change disclosure: docs disclose the main reuse/diagnostic behavior, but the deep-mode stale readback is undisclosed.
  • Guidance vs obligation: no new machine-enforced caller obligation.
  • Default-off isolation: not applicable; this is always-on diagnostic hardening.
  • Authority semantics: no actor-lifecycle, permission, or authority change.

Report the post-probe locator state and liveness instead of the pre-probe snapshot. Keep damaged-locator diagnostics on successful deep probes, classify host permission and generic read failures as invalid rather than absent, and cover both outcomes.

Signed-off-by: JasonBuildAI <jasonbuildai@gmail.com>
Propagate a host-permission denial from authenticated locator recovery instead of treating it as no runtime and cold-starting. Inspect locator state and PID liveness even when the Node probe is unavailable, while keeping readiness unavailable. Add regressions for both boundaries.

Signed-off-by: JasonBuildAI <jasonbuildai@gmail.com>
@BiBoyang

Copy link
Copy Markdown

Reporter here (I filed #6871). Reading the diff, the reuse path looks sound to me: a damaged locator is only adopted after an authenticated runtime.ping that matches pid, fingerprint and info_path, and an unreadable locator keeps its diagnostic instead of looking absent.

One gap before merge: _recover_runtime_info refuses reuse on six conditions, but the new tests only exercise the success path and the dead-PID case. Could you add cases for wrong PID, wrong token, fingerprint mismatch, and info_path mismatch? Those four branches decide whether a request is sent to the wrong daemon, so they are the part I would most want pinned by a test.

@loopx-agent loopx-agent 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.

Reviewer: model_agent | model=gpt-6.1-sol | provider=OpenAI | declaration_source=runtime_reported | reasoning_effort=max

结论:REQUEST_CHANGES,评审 head:607a787cef6ea7c23c6b793982b96a079126f063。新增 locator 检查存在一个已独立复现的 P2:损坏 JSON 中的超大 PID 会使 doctor --installation-only 抛异常,无法返回结构化诊断。主恢复路径和既有修复仍分别认可。

动机

受影响的是遇到本机运行时定位文件损坏、并依赖 doctor 判断如何恢复的用户。
旧实现把损坏的定位文件当作不存在,下一次请求会启动第二个 daemon;当前 head 在认证成功时复用原进程,并显示定位文件诊断与 PID 存活信息。
真实 daemon 已证明 schema 损坏后复用原 PID、深度诊断保留损坏状态、重启停止原进程;但超大 PID 会使新增检查抛出异常,doctor 无法返回 JSON。
本次范围是现有本机运行时的发现、认证复用和健康回读;操作系统权限、模型服务与完整安装资格分别保留其验证边界。

改动思路

定位文件诊断、认证探测和 doctor 投影继续留在既有 Python 宿主适配器,复用 TypeScript runtime.ping;无需第二个服务或新的生命周期协议。
当前提案的复用与诊断范围合理,但须先修复损坏数值导致的异常,才能完成这一恢复切片。

定位文件是记录端点、token 和进程标识的私有 JSON,不等同于进程确实服务。提案先区分缺失与损坏,再让损坏但保留端点的候选接受真实 runtime.ping 认证,最后决定复用或交给既有冷启动 owner;doctor 仅显示派生事实。这个范围有实际价值:不修改会继续产生孤儿进程。新类型与小 helper 留在宿主适配器也合理,无须让用户手工启动 daemon 或建立新协议。

具体改动

先读 #6871、完整既有 review 及后续说明,再以修改前的 docs/guides/installing-loopx.md、修订 764b989dce88ef43ade5aba5af4958ee2cf3a821 为已接受基线。IR-1 对应既有自动启动/复用和问题要求的活 daemon 恢复,已实现;IR-2 对应 loopback/token/当前源码身份边界,已实测四条拒绝路径;IR-3 对应可用、真实的 doctor 诊断及问题要求的缺失/损坏区分,当前未满足超大 PID 反例。IR 标签只是本评审的条款索引。

关键代码讲解

  • _inspect_info:解析 JSON,保留损坏原因与已记录 PID 的存活事实。问题在第 530–534 行:只检查“正整数”,就先调用 OS 存活探测;schema 拒绝在其后。
  • _recover_runtime_info:在原 request/startup owner 内使用既有认证 ping,校验返回 PID、当前 fingerprint 和 locator 路径。真实端点的错误 token、错误 PID、另一运行上下文 fingerprint 和不同 locator 路径均被拒绝采用。
  • collect_effect_runtime_readiness:分别投影 locator 与 runtime,并在 deep probe 后重新读取。doctor 两处 Markdown 显示和安装指南双语说明随之更新;三份测试覆盖 deep、权限拒绝、Node 不可用及正常生命周期。完整六文件 diff 已读,未把旧评审仅有的修复当成整个提案。

对主干的风险

P2:在对损坏 locator 做 PID 探测前安全处理数值范围。 用隔离目录写入非当前 schema、pid=10**30 的合法 JSON,实际基线 API 正常返回,真实 doctor --installation-only 为 exit 0 且输出 JSON;当前 head 的 API 抛 OverflowError: Python int too large to convert to C long,同一 CLI 为 exit 1、回溯且无 JSON。触发链是 _inspect_info → _pid_is_alive → process_is_alive → os.kill,发生在 schema 检查前。请对外部 PID 做安全范围/转换处理,或在本地检查层容纳该数值异常,返回损坏诊断和未知/无效存活事实;增加 API 与实际 CLI 回归,保留正常 live/dead PID 及认证复用覆盖。

独立完整相关矩阵为 221 passed,无 skip。同输入的真实 Node 对照也重现原问题:基线 schema 损坏后产生第二个 PID,原进程仍服务;当前 head 复用原 PID、deep 诊断继续显示 invalid,restart 真正停止原进程。此前 deep 投影与 host-permission 两项修复已有当前证据,四种认证反例也已通过;测试的正面价值不能消除上述新数值缺陷。

风险 premerge 的 6 项 direct 通过,14 项 selected 均执行,完整门仍失败。语义 twin 门在真实 base/head 具有相同完整维护项身份与 44/43 失败,属于既存问题。首次把测试临时目录放在 Git 工作区内导致两项假设失败,原记录保留;移到 Git 外后 catalog e2e 通过,install-local-smoke 仍在 head 与实际基线均遇到原 120 秒超时。两次超时没有足够细节证明同因,保持 unresolved,未认定它是本 PR 新缺陷,也未放宽期限或冒称安装通过。Ruff、diff 与边界扫描通过;按当前策略未查询或等待 CI。

语义与 CI 对齐

_RuntimeLocatorState 与 _RuntimeLocatorDiagnostic 是现有适配器内的两个 Literal 闭集;既有 lifecycle v0 只增加派生显示字段,没有新建共享执行权威。typed 检查仍须覆盖外部数值进入 OS API 的边界:IR-3 的当前义务是损坏状态可诊断,新增路径却丢失结构化结果。最小修复位于该 PID 检查;重跑 locator/readiness 相关测试,并用隔离 locator 对实际 doctor --installation-only 做同一反例。缺少新 vocabulary 注册不是这里的阻塞原因。

我的整体评价

实际认证恢复有用、机制与问题规模相称;保留私有 v0 locator 和既有 reader facade 有当前调用依据。可延后把重复读取合并为一次 inspection,前提是保持诊断与认证语义,这不是额外合并门。长程正常恢复路径为 preserved,但用户体验在损坏数值上是 regression,因此当前切片的交付仍 not_yet_proven。定位文件诊断、认证探测和 doctor 投影继续留在既有 Python 宿主适配器,复用 TypeScript runtime.ping;无需第二个服务或新的生命周期协议。当前提案的复用与诊断范围合理,但须先修复损坏数值导致的异常,才能完成这一恢复切片。 请修复并给出新的精确 head;平台安装、真实模型和长期收益不由本地通过数关闭。

English verdict: REQUEST_CHANGES — 607a787cef6ea7c23c6b793982b96a079126f063. A damaged locator with an oversized positive PID triggers the new pre-schema liveness probe and crashes readiness plus the real installation-only doctor; actual base returns JSON. Bound/contain PID decoding and add API/CLI coverage. 221 tests, real same-PID reuse/deep/restart and four identity-rejection probes pass. Existing twin debt and the unresolved installer timeout remain separate.

Comment thread loopx/control_plane/effect_runtime.py Outdated
)
pid = payload.get("pid")
recorded_pid_alive = (
_pid_is_alive(pid)

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.

[P2] Safely validate/bound externally decoded PID or catch numeric conversion overflow locally; preserve invalid/unknown liveness and safe diagnostic; add API and CLI regression.

Observed: The new liveness probe executes before schema validation. OverflowError escapes readiness; actual doctor --installation-only exits1 with a traceback and no JSON. Same input on actual base exits0 with structured output.

Validation: Use a disposable runtime directory, damaged schema and pid=10**30. Assert API and actual installation-only doctor return public-safe diagnostic/unknown liveness without throwing; keep normal live/dead PID and authenticated reuse tests.

Reject out-of-range recorded PIDs before the OS liveness probe and contain platform overflow so damaged locator data cannot crash readiness or the installation-only doctor. Cover both the API projection and the actual console command.

Signed-off-by: JasonBuildAI <jasonbuildai@gmail.com>

@JasonBuildAI JasonBuildAI left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head review: 9dadb773d

Reviewed the diff, tests, and the three follow-up boundaries on the exact head. No substantive correctness, scope, or value blocker found in the changed scope.

  • The third review boundary is fixed: _recorded_pid_alive rejects booleans, non-positive/non-integer values, values above _RUNTIME_PID_MAX, and contains OverflowError before the OS liveness probe. Readiness and loopx doctor --installation-only --format json now retain locator_state=invalid with unknown liveness instead of crashing.
  • The new API and real console-command regressions failed 2 before 9dadb773d and pass 2 after (2 passed in 1.71s).
  • Final-tree checks: focused 47 passed; Effect runtime/readiness suite 216 passed, 7 skipped; Ruff All checks passed!; mypy exits 1 with the same 48 errors and four normalized diagnostic shapes as base 764b989dc; semantic inventory reports only the two local locator aliases.
  • Premerge: direct checks, module ceiling, 8/8 risk-profile smokes and the public boundary passed. The four catalog failures reproduce independently on the original base and are not introduced here.
  • Hosted CI: typescript-core (1/3) fails in loopx/control_plane/presentation/goal_source_links.ts, which this PR does not modify; checks fails because typescript-coverage is skipped but required as success. Six Python test shards are still pending, and the PR is BEHIND.

Verdict: the prior request for changes is addressed on the exact head; no merge is requested. The PR remains for maintainer review and a final rebase/re-run.

Signed-off-by: JasonBuildAI <jasonbuildai@gmail.com>
@JasonBuildAI

Copy link
Copy Markdown
Contributor Author

@BiBoyang Added in 1940477fb.

The four rejection branches now have explicit tests in tests/control_plane/test_effect_runtime_locator_recovery.py:

  • ping returns a different PID;
  • the runtime rejects the recorded token;
  • ping returns a different fingerprint;
  • ping reports a different published info_path.

Each test calls _runtime_info_for_request and asserts it returns None, so a damaged locator cannot be reused to route a request to another daemon.

Validation on 1940477fb: focused tests 51 passed in 56.58s; Effect runtime/readiness suite 220 passed, 7 skipped, 3 warnings in 152.15s; Ruff All checks passed!; git diff --check and semantic inventory clean. No merge is requested.

@songoow songoow 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.

Reviewer: model_agent; OpenAI GPT-6 Astra; provider OpenAI; self_reported.

Reviewed head: 1940477fb7913811db0315547d02ceb82647af8a

动机

这是对 #6871 所述 Effect runtime locator 损坏问题的修复:一个 daemon 仍在运行时,仅损坏保存端点信息的 locator schema,旧代码会把不可用的 locator 当作“没有 runtime”,诊断也不能解释仍存活的 PID,后续请求还可能启动另一个 daemon。受影响的是调用 Python/CLI 控制平面的用户、doctor 的操作者,以及需要在已有 runtime 上继续工作或关闭它的恢复流程。

预期前后变化是:区分 locator 缺失、无效和可用,保留独立的存活线索;只有通过当前 daemon 的认证及身份校验,才复用损坏记录指向的进程。无效文件不因此变成合法文件,doctor 仍明确报告其损坏原因。非目标包括自动迁移或改写未知 locator schema、根据 PID 存活就授予 runtime 身份、绕过本机 host permission,或创建另一个后台服务管理器。

改动思路

新 _RuntimeLocatorRead 把 locator 的存在性、解析诊断、记录 PID 的存活线索和可信连接信息分开,避免原先单一 None 混淆多种状态。恢复逻辑仍在现有 Effect runtime owner 内完成:正常记录沿原路径使用;损坏但有候选端点的记录进行短、有界的 authenticated runtime.ping,再判断 PID、fingerprint 和发布路径是否对应当前实例。请求、restart 和 readiness 复用这个身份边界。

这是合理的局部分层:新的诊断字段是一次读取的派生信息,不是第二份 runtime authority;真正的可用性仍由现有 daemon 返回。文件写入、连接 token、RPC 及进程关闭的 owner 没有外移。更小的“只给 doctor 加一句错误文字”不能避免后续请求重复启动进程,因此把恢复接入实际 request/restart 路径是必要的。

具体改动

关键代码讲解

  • _recorded_pid_alive(loopx/control_plane/effect_runtime.py:436)只把合法正整数 PID 交给存活检查,排除 bool、非整数和超出明确边界的数值,并兜住 OverflowError。这覆盖旧 review 指出的巨大 JSON PID 导致 doctor 丢失 JSON 输出的问题;当前 head 已有相应 API 与 installation-doctor 回归用例。
  • _inspect_info / _RuntimeLocatorRead(同文件 :488、:496)区分 missing、schema/fingerprint/内容无效、占用路径和读取错误;持续 permission denial 保持既有 typed host-permission 语义。返回 raw payload 只是候选恢复材料,不能直接当作可信连接信息。
  • _recover_runtime_info / _runtime_info_for_request(同文件 :903、:963)对损坏 locator 的候选执行一次认证 ping。失败、PID 不匹配、fingerprint 不匹配或返回的发布路径不匹配不会得到可复用 info;只有身份一致的 daemon 可继续服务,避免把“某个 PID 还活着”当成充分证据。
  • restart_effect_runtime(同文件 :703)使用同一恢复后的 serving identity 关闭原实例,避免请求恢复能用、restart 却因损坏 locator 找不到进程的分裂行为。
  • collect_effect_runtime_readiness(同文件 :1399)向 doctor 投影 locator 状态与 liveness;深探针实际启动或确认 daemon 后刷新投影,同时保留无效 locator 的诊断。Node 不可用与文件损坏是不同事实,读回没有用其中一个消掉另一个。

正向路径是只损坏当前活 daemon 的 schema,读取保留 runtime_locator_schema_mismatch,认证 ping 证明是同一实例,后续请求复用 PID,restart 仍能关闭它。负向路径是认证拒绝或对端 PID/fingerprint/info path 不同,候选恢复失败,再按已有启动/错误 owner 处理,不升级不匹配身份为可信 runtime。

我阅读了全部 6 个改动文件以及前序 review,检查了当前 head 相比上次作者说明后的变化。最后一轮增加的四项负例分别覆盖错误 PID、被 daemon 拒绝的 token、错误 fingerprint 和其他发布路径;它们补充边界验证,没有放宽恢复条件。此前 review 的 oversized-PID 问题在当前代码中已有上界与异常保护,不应重复作为未修复 finding。git diff --check 在当前 head 通过。

对主干的风险

没有发现需要阻塞的代码问题。最强的反例是损坏记录指向另一个活服务,而恢复逻辑把它错误接纳;代码为此同时保留 loopback 限制、token 认证、PID/fingerprint 一致性以及已发布路径校验。新恢复 ping 只发生在有可用候选信息的损坏记录上,正常有效 locator 的连接路径没有改成每次重建 daemon。永久权限失败也没有被当成普通文件缺失吞掉。

需要继续诚实保留的边界是:静态检查和注入身份的单元用例不能单独证明所有真实进程竞态、跨平台 PID 行为或 socket 故障。新字段也应当一直保持 diagnostic/projection 身份,调用者不应把 recorded_pid_alive=true 单独解释为认证成功。当前代码的正常请求、深浅 readiness 和显式 restart 使用同一 owner,未见新增持久副本或另起恢复循环。

此前在本地隔离环境执行真实 daemon 测试遇到 host permission/运行资源限制,部分测试通过不能代表整个真实进程验证通过;我未把这些环境失败作为当前 PR 的代码问题。依本次要求,本结论主要基于代码,不依据 CI,不继续扩大测试。最有价值但我尚未独立补齐的验证,是当前 head 的真实 daemon schema-corruption → 同 PID 复用 → deep readiness → restart 完整重放,以及对应基线行为对照。

我的整体评价

本次源码 review 未发现 blocking finding。损坏诊断、候选存活与经过认证的 serving identity 被清楚分开,恢复入口同时覆盖请求和关闭流程,前序 oversized-PID finding 在当前 head 已处理。新增负例符合身份边界,代码体量主要来自显式诊断与回归覆盖。可以采用此源码结论;本评论不声称已经完成全部 runtime 验证或可立即合并的资格判断。

English verdict: COMMENT — no blocking code finding at 1940477fb7913811db0315547d02ceb82647af8a. The current implementation distinguishes missing/invalid locators from authenticated runtime identity, shares recovery between request/readiness/restart paths, and fixes the previously reported oversized-PID failure. This is a source-review conclusion; no CI gate or complete independent live daemon/base-head validation is claimed.

@huangruiteng
huangruiteng merged commit 1803e1b into loopx-project:main Oct 22, 2026
37 of 44 checks passed
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.

effect runtime: a corrupt locator spawns a second daemon and doctor reports stopped

5 participants