Skip to content

fix(cosh-ng): [core] degrade gracefully when SIGINT registration fails - #2648

Open
SunnyQjm wants to merge 1 commit into
alibaba:mainfrom
SunnyQjm:fix/cosh-sigint-register-fallback
Open

fix(cosh-ng): [core] degrade gracefully when SIGINT registration fails#2648
SunnyQjm wants to merge 1 commit into
alibaba:mainfrom
SunnyQjm:fix/cosh-sigint-register-fallback

Conversation

@SunnyQjm

Copy link
Copy Markdown
Collaborator

Summary

修复 24daa1e("await SIGINT without polling")遗留的注册失败路径缺陷。该缺陷最早由 qoderai 在并行 PR #2624 的 P1 评审中识别,分析与验证讨论见该 PR(#2624 已因主体与 24daa1e 重复而关闭,本 PR 携带其唯一幸存增量)。

问题tokio::signal::ctrl_c() 注册失败(受限环境禁 sigaction)时,现行代码在 select! 分支内 exit(1)tokio::select! 默认随机化分支 poll 顺序——注册失败时 run() 分支可能已先启动(session-control 模式甚至已以自身状态码退出),失败行为不确定;且 headless 服务在此类环境下启动即死。

修复:注册失败改为 stderr 告警一次 + future 永不完成。handler 未安装时 SIGINT 保持内核默认终止 disposition——Ctrl-C 仍可终止进程,服务继续运行。消除随机性,可用性优于启动即死。

验证

  • cargo clippy -p cosh-core --all-targets:0 警告
  • cargo test -p cosh-core(alinux3 arm64 容器):失败集与同环境 origin/main 基线一致(4 个 root 权限类已知环境项;另有 1 例 listing_distinguishes_persisted_shell_envelope_sessions 单轮偶发,隔离连跑 5/5 通过,判定 suspected-flaky,与本改动无因果面),零新增失败
  • 正常路径行为不变:注册成功时 SIGINT 优雅退出语义与现状一致(该路径在 perf(cosh-ng): [core] replace SIGINT flag polling with tokio ctrl_c #2624 head 上有过真机实测:空闲常驻进程首个 SIGINT 即退出)
  • 注册失败路径需禁 sigaction 的受限环境,未做运行时构造,由注释与上述语义推理覆盖

focused green(cosh-core crate);全仓 gate 交 CI。

@SunnyQjm
SunnyQjm requested a review from KaiLongZhou as a code owner August 18, 2026 10:04
@github-actions github-actions Bot added the component:cosh-ng src/cosh-ng label Aug 18, 2026

@qoderai qoderai Bot left a comment

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] SIGINT 失败路径目前只能通过推理验证,建议后续考虑引入可测试注入点。
[P2] headless 与 session-control 模式下 SIGINT 预期行为主要存在于注释,建议在模式判定附近补充更明确约束说明。

整体变更方向合理,主路径行为保持一致,退化策略符合描述语义。


🤖 Generated by QoderView workflow run

Comment on lines 191 to 194
@@ -195,8 +189,20 @@ fn is_agent_headless_mode(args: &cli::CliArgs) -> bool {
}

#[cfg(unix)]

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] SIGINT 失败路径行为不可测试

wait_for_sigint 不再返回 Result,测试无法直接模拟注册失败并验证 "警告一次+永不完成" 语义。建议保留一个内部注入点(如 feature-gated hook 或私有 helper)以便构造失败路径,确保该受限环境退化行为在未来修改中不被破坏。


🤖 Generated by QoderFix in Qoder

Comment on lines 189 to 192
@@ -195,8 +189,20 @@ fn is_agent_headless_mode(args: &cli::CliArgs) -> bool {
}

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] headless 模式下 SIGINT 行为差异未在代码层显式体现

注释说明受限环境下服务保持运行、SIGINT 仍用内核默认终止进程,但 headless/session-control 模式下的差异只存在于文字说明。建议在 is_agent_headless_mode 附近补充更明确的行为约束注释,帮助后续维护者理解不同模式下 SIGINT 的预期语义。


🤖 Generated by QoderFix in Qoder

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

Copy link
Copy Markdown

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: 8753de17d1

ℹ️ 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".

Comment on lines +193 to +195
// If handler registration fails (restricted environments), fall back to
// the kernel default SIGINT disposition: warn once and never resolve,
// so the process keeps serving and Ctrl-C still terminates it. Exiting

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 Preserve termination when SIGINT is inherited as ignored

When handler registration fails under the restricted environments targeted here and the launcher has inherited SIGINT as ignored—an explicitly supported scenario in crates/cosh-core/tests/sigint.rs—the failed registration does not restore the kernel default because ignored dispositions survive exec. This pending future therefore leaves SIGINT ignored indefinitely, so neither Ctrl-C nor kill -INT terminates the headless process despite the comment and warning claiming otherwise. Avoid waiting forever unless termination is actually guaranteed, or retain an actionable failure path for this case.

AGENTS.md reference: AGENTS.md:L347-L349

Useful? React with 👍 / 👎.

@SunnyQjm

SunnyQjm commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #2648
head_sha: 8753de1
reviewed_at: 2026-08-18T10:48:09Z

Findings

未发现 blocking package/module/public API 组织问题。

  • [P2] src/cosh-ng/crates/cosh-core/src/main.rs:199 退化路径假设 SIGINT 处于内核默认 disposition,但忽略态(SIG_IGN)可跨 exec 继承:若 launcher 已忽略 SIGINT,注册失败不会恢复默认动作,此后 Ctrl-C 与 kill -INT 均无法终止 headless 进程,与新增注释「Ctrl-C still terminates it」的承诺不符。建议失败路径显式将 SIGINT 重置为 SIG_DFL,或收窄注释/告警措辞。
  • [P2] src/cosh-ng/crates/cosh-core/src/main.rs:192 「告警一次 + 永不完成」语义无注入点可测,仅靠注释与推理覆盖;建议抽出 crate-private 可单测 helper,防止后续改动悄悄破坏退化路径。
  • [P3] src/cosh-ng/crates/cosh-core/src/main.rs:189 headless 与 session-control 模式下 SIGINT 预期行为只存在于新注释中,is_agent_headless_mode 判定处无交叉引用,建议补一条注释指向该退化约定。

组织规则核对

  • 变更仅落在 cosh-core/src/main.rs(Cargo 约定的 binary 入口文件),未触碰 cosh-shell,不涉及与纯代码组织修复混提交的问题。
  • 未新增 crate、未新增 root implementation 文件、未改 lib.rs public surface、无 forbidden dependency、无 self-crate path、无大文件越线。
  • main.rs 增长约 12 行,均为信号处理职责内注释与退化逻辑,仍属 bootstrap 装配范畴,未引入新职责。

Open Questions

  • 是否值得在失败路径主动 signal(SIGINT, SIG_DFL),以覆盖继承忽略态的受限环境场景?

Validation

  • Passed(作者自报):cargo clippy -p cosh-core --all-targets 0 警告;crate 测试无新增失败。
  • CI:Build cosh-ng releaseTest cosh-ngTest cosh-ng fast checks 均 SUCCESS。
  • Not run:注册失败路径的真实受限环境运行时验证(作者已声明);本评审未执行任何本地命令,行号基于 diff 推算。

When tokio::signal::ctrl_c() fails to register (restricted environments
that deny sigaction), the current code exits with status 1 from inside
tokio::select!. Because select! polls branches in random order, the
run() branch may already have started by then -- in session-control
mode it may even have exited with its own status -- so the failure
behavior is nondeterministic, and headless serving dies at startup in
such environments.

Warn once and never resolve the signal branch instead: with no handler
installed the kernel default SIGINT disposition stays in effect, so
Ctrl-C still terminates the process while the service keeps running.

Identified by the qoderai P1 review on the parallel PR alibaba#2624 (see the
verification discussion there).

Fixes: 24daa1e ("fix(cosh-ng): await SIGINT without polling")
@SunnyQjm
SunnyQjm force-pushed the fix/cosh-sigint-register-fallback branch from 8753de1 to 6df45a4 Compare August 18, 2026 11:34
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

三条评审意见处理完毕(head 已更新为 6df45a4):

@chatgpt-codex-connector [P2](inherited-ignored × 注册失败):意见成立,已采纳。 核实:ignored disposition 穿越 exec(tests/sigint.rs 钉住的 launcher 场景),注册失败时不会恢复 default——原文案"内核默认 disposition"在该格子不成立,Ctrl-C 与 kill -INT 均不可终止。处置按建议的"retain an actionable failure path"方向:无法安全恢复 SIG_DFL(sigaction 族在注册已失败的环境不保证可用,且 forbid(unsafe_code) 下无安全 API、signal-hook 已随 24daa1e 移除),故维持 pending 但把注释重写为完整的 (注册结果 × 继承 disposition) 2×2 行为合同,并将告警文案改为如实陈述 + 可行动指引("the inherited SIGINT disposition stays in effect (stop with SIGTERM if Ctrl-C does not terminate the process)")。

@QoderAI [P2](headless/session-control 语义应在代码层显式):部分采纳。 已在 run_until_sigint 处补注释声明 SIGINT 等待包裹全部运行模式、合同见 wait_for_sigint;未在 is_agent_headless_mode 处堆注释——模式判定与信号语义正交,该函数不拥有信号语义,就近引用比复制描述更可维护。

@QoderAI [P2](失败路径可测试注入点):不采纳,理由如下。 该路径唯一逻辑是"打印一行 + pending";引入注入点需把 tokio::signal::ctrl_c 参数化为可替换抽象,给 binary bootstrap 层(standard §4 职责范畴)增加常驻抽象,而注入 mock 验证的只是 mock 自身的 Err 分支,无法证实真实受限环境的行为。语义由上述 2×2 合同注释 + tests/sigint.rs(注册成功侧)钉住;若该路径未来出现真实缺陷,再升级为注入点。

验证:clippy 0 警告;cargo test -p cosh-core 失败集与同环境 origin/main 基线一致(4 项已知环境项),零新增。本轮为注释与告警文案变更,无控制流变化。

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@SunnyQjm

SunnyQjm commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #2648
head_sha: 6df45a4
reviewed_at: 2026-08-20T03:27:41Z

Findings

未发现 blocking package/module/public API 组织问题。

  • [P3] src/cosh-ng/crates/cosh-core/src/main.rs:211(按 diff 推算)退化告警走 eprintln!,成功路径走 tracing::info!(约 main.rs:124),两分支日志通道不一致;只采集 tracing 输出的部署环境会看不到该告警。上轮已提示、当前 head 仍未变,属轻微一致性建议,不阻断。(反向考虑:受限环境下 stderr 可能比 tracing 更可靠,若作者有意为之可直接关闭此条。)

行为核对

  • 注册成功路径语义不变:ctrl_c().await 返回 Ok 时不进入 if let Errselect! 分支照常记录优雅退出日志。
  • 失败路径由 select!exit(1) 改为告警一次 + std::future::pending::<()>(),消除了随机 poll 顺序竞态;run() 完成时 pending future 被 drop,取消安全、无泄漏。
  • 2×2(注册结果 × 继承 disposition)契约注释与 POSIX 语义一致:注册成功覆盖继承 disposition(tests/sigint.rs 钉住 inherited-ignored 场景);失败时继承 disposition 生效,inherited-ignored 组合需 SIGTERM,告警文案如实给出可行动指引。
  • 未引入 unsafe,符合 forbid(unsafe_code);放弃恢复 SIG_DFL 的理由(sigaction 族在受限环境不保证可用)在注释中说明充分。
  • wait_for_sigint 返回类型从 io::Result<()> 收窄为 (),唯一 caller 为 run_until_sigint,属内部简化,无 API 面影响。

组织规则核对

  • 变更仅落在 cosh-core/src/main.rs(Cargo 约定 binary 入口),单 crate、单关注点,未触碰 cosh-shell,不存在跨 crate 行为修改与组织修复混提交(standard §1)。
  • 未新增 crate、未新增 root implementation 文件、未改 lib.rs public surface、无 forbidden dependency、无 cosh_shell:: self-crate path。
  • main.rs 净增约 21 行,均为信号退化逻辑与契约注释,属 bootstrap/信号装配职责(standard §4);全文行数按 diff 推算仍低于 300 行目标(估算值,行数以 CI gate 实测为准)。

剩余风险

  • 失败退化路径无注入点可单测,仅靠契约注释与语义推理覆盖(前轮 P2,作者已给出不采纳理由:唯一逻辑是"打印一行 + pending",注入 mock 无法证实真实受限环境行为);后续改动 wait_for_sigint 时缺乏测试防护。
  • inherited-ignored + 注册失败组合下进程只能靠 SIGTERM 终止,属显式设计决策,已如实写入注释与告警文案。
  • 注册失败路径需禁 sigaction 的受限环境,作者声明未做运行时构造。

Validation

  • Passed(作者自报):cargo clippy -p cosh-core --all-targets 0 警告;cargo test -p cosh-core 与同环境 origin/main 基线一致,零新增失败。
  • CI:Build cosh-ng releaseTest cosh-ngTest cosh-ng fast checks 均 COMPLETED SUCCESS;首轮 raw_cli PTY 失败经本地 7/7 复跑与同 head rerun 转绿,作者判定 suspected-flaky 且与改动面无交集,分类合理。
  • Not run:注册失败路径的受限环境运行时验证(作者已声明);本评审未执行任何本地命令,行号与行数均基于 diff 推算。

@KaiLongZhou

Copy link
Copy Markdown
Collaborator

Code Review Check: ✅ Approved

Reviewer audit of code-reviewer's analysis — all findings verified against source code.

Summary

This is a precise, minimal fix that correctly resolves the SIGINT registration failure race condition. The code-reviewer's analysis is accurate with no significant omissions.

Verified points

  1. Race elimination ✅ — Old code's exit(1) in select!'s Err branch raced with run() due to randomized poll order. pending() correctly eliminates this.
  2. pending() semantics ✅ — Zero-sized type, never resolves, no resource leak on drop. Correct for disabling the signal branch.
  3. forbid(unsafe_code) compatible ✅ — No unsafe introduced; SIG_DFL restoration correctly ruled out.
  4. Disposition coverage ✅ — All 4 (registration × disposition) combinations documented in comments.
  5. Test compatibility ✅ — tests/sigint.rs covers registration-success path (unchanged); failure path reasonably deferred to code reasoning.
  6. Cancellation safety ✅ — Dropping Pending or ctrl_c() future on run() completion is safe.

Note

cosh-ng CI checks (Test cosh-ng, Test cosh-ng fast checks, Build cosh-ng release) were still IN_PROGRESS at review time. Merge should wait for CI green.

@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

CI 失败分类(回应 auto-review P2 的合并前核对要求):

结论:suspected-flaky(PTY 时序类),与本 PR 无因果,rerun 已绿。

  • 首轮失败集(run 32132420274 第 1 轮):failed_command::raw_cli_build_failure_respects_analysis_mode_matrixevidence_request::raw_cli_terminal_output_read_misroute_records_details_audit——均为 cosh-shell raw_cli PTY 集成测试,与本 PR 改动面(cosh-core 信号等待路径,纯注释/文案/错误分支)无交集。
  • 本地复现尝试:alinux3 arm64 容器内同 head 对这两个用例连跑 3 轮(每轮 2 用例)+ 1 次单跑,7/7 全部通过,无法复现。
  • 同 head rerun:Test cosh-ng 转 success,当前 run 整体 conclusion=success。
  • 背景:raw_cli PTY timing 测试历史上有 flake 记录;且 main 于昨日合入 relay 事件驱动重构(28cbd7f4),该套件时序特征刚发生变化,首轮失败更可能与此相关,建议在 main 侧观察其复发率。

@KaiLongZhou

Copy link
Copy Markdown
Collaborator

Code Review: PR #2648 — fix(cosh-ng): [core] degrade gracefully when SIGINT registration fails

变更概述

修改 cosh-core/src/main.rs 中的 wait_for_sigint()run_until_sigint(),将 SIGINT handler 注册失败时的行为从 exit(1) 改为 stderr 告警 + std::future::pending()(永不完成)。

审查结论:✅ LGTM

正确性分析:

  1. 竞态消除:旧代码在 select! 分支内 exit(1),但 tokio::select! 随机 poll 顺序意味着 run() 分支可能已先启动(session-control 模式甚至已以自身状态码退出)。改为 pending() 后,select! 自然等待 run() 完成,消除了不确定性。

  2. std::future::pending::<()>() 选择正确:这是安全的、稳定的 API,语义清晰——future 永不 resolve,select! 只等 run() 分支。类型标注 ::<()> 明确,无需额外 turbofish 推断。

  3. 信号处置语义完整:注释详细覆盖了 registration outcome × inherited disposition 的四种组合,且解释了为什么不尝试恢复 SIG_DFL(需要额外 sigaction 调用、受限环境可能再次失败、forbid(unsafe_code) 约束下不可表达)。

  4. 错误信息实用eprintln! 消息告知用户 "stop with SIGTERM if Ctrl-C does not terminate the process",覆盖了 inherited-ignored 场景下的操作指引。

  5. forbid(unsafe_code) 合规:未引入任何 unsafe 代码。

  6. 类型变更合理wait_for_sigint()-> std::io::Result<()> 改为 -> (),错误处理内聚到函数内部,调用方 select! 分支简化为 _ = wait_for_sigint(),更干净。

测试覆盖:

  • tests/sigint.rs 中的 sigint_exits_when_inherited_as_ignored 验证了正常注册路径(inherited-ignored 场景下 tokio 仍能安装 handler 并优雅退出)。
  • 注册失败路径需要禁 sigaction 的受限环境,无法在常规 CI 中构造,由注释和语义推理覆盖——这是合理的。

无问题发现。 变更范围小、逻辑清晰、注释充分,是一个高质量的缺陷修复。


@code-reviewer-leader 请继续委派 code-reviewer-check 进行审核。

@KaiLongZhou

Copy link
Copy Markdown
Collaborator

Code Review Check: ✅ LGTM

Verified code-reviewer's analysis against actual code — all 6 points confirmed correct:

  1. Race eliminationtokio::select! random poll order is documented behavior; old exit(1) could clobber run()'s exit code
  2. std::future::pending::<()>() — stable API, correct type, no resource leak on drop
  3. Signal disposition 4-combination analysis — matches POSIX semantics; SIG_DFL restoration reasoning sound
  4. forbid(unsafe_code) compliance — confirmed at main.rs:1, no unsafe introduced
  5. Type change-> std::io::Result<()> to -> () is a clean simplification
  6. Test coveragesigint_exits_when_inherited_as_ignored covers normal path; failure path requires restricted environment (untestable in CI, adequately covered by comments)

No P0/P1 omissions found. Change scope is small (1 file, +28/-10), logic is clear, comments are thorough.

Verdict: Approve — no blocking issues.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants