feat(anolisa): type system service lifecycle - #2724
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
[P1] teardown 路径对非 missing stop 失败的 typed 语义存在分叉，可能误把实际故障归类为“已停止”，影响告警定位。
[P2] systemd_cli_error 在 NotFound 分支返回 Runtime error 的语义与上层基于 typed NotFound 的“已不存在”处理可能重复。
[P2] FakeSystemdRunner 新测试未覆盖 disable/daemon-reload 失败场景，容易遗漏 CLI warning 文案回归。
🤖 Generated by Qoder • View workflow run
| cmd: &str, | ||
| systemd: &Systemd<R>, | ||
| warnings: &mut Vec<String>, | ||
| ) { | ||
| match systemd.stop_unit(SERVICE_NAME) { |
There was a problem hiding this comment.
[P1] teardown 非 missing stop 失败时 status 探测与诊断不完全对齐
stop_and_disable_service 中对 SystemdError::NonZeroExit 时追加一次 unit_status 探测，仅在第二次查询返回 NotFound 才按“已停止”处理，而 Systemd::stop_unit 自身在非 0 退出时保持 NonZeroExit，不区分 missing。对同一 stop 调用的 typed 语义存在分叉，可能导致 teardown 在某些本地化诊断下错误地将实际故障归类为“已停止”，影响后续告警定位。建议统一依赖上层 typed evidence，而不是按 stderr 内容或额外 show 调用推断状态。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
Not a real issue. This path does not parse the localized stop diagnostic. stop_unit preserves the effect failure as NonZeroExit; teardown only treats the unit as already stopped when a separate machine-readable unit_status observation returns typed NotFound. A loaded unit, status spawn failure, or status non-zero exit keeps and renders the original stop failure. The missing and non-missing fake-runner cases cover both branches, including arbitrary diagnostic text. The separation is intentional because systemctl stop itself does not provide typed missing-unit evidence.
| command: cmd.to_string(), | ||
| reason: format!("systemctl {} failed: {stderr}", args.join(" ")), | ||
| }); | ||
| fn systemd_cli_error(cmd: &str, args: &[&str], error: SystemdError) -> CliError { |
There was a problem hiding this comment.
[P2] systemd_cli_error 对 NotFound 分支与上层 missing 单元语义可能重复
systemd_cli_error 在 SystemdError::NotFound 时构造 CliError::Runtime 文本为 service not found，而上层 teardown 已使用 typed NotFound 作为“服务已不存在”信号并转为友好 warning。二者叠加可能在未来调用处产生既记录 warning 又返回 runtime error 的重复路径。建议在需要“已不存在”语义的调用点避免再将 NotFound 包装为硬错误，或明确区分仅用于 CLI 输出的分支。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
Not a real issue. Teardown handles typed NotFound before calling systemd_cli_error, so one execution cannot both emit the already-stopped warning and return a runtime error. The NotFound arm keeps the error mapping exhaustive for non-idempotent callers, where a missing target should remain a hard error; the current fixed non-empty setup/disable targets do not reach it. Removing that mapping would incorrectly make all future missing-target contexts benign.
| } | ||
| } | ||
|
|
||
| match systemd.disable_unit_file(SERVICE_NAME) { |
There was a problem hiding this comment.
[P2] 新增 FakeSystemdRunner 测试未覆盖 disable/daemon-reload 错误路径
systemd 生命周期 fake 测试目前验证了非升级/升级顺序以及 stop/enable 正常与部分失败信息，但 teardown 中 disable_unit_file 和 daemon_reload 的失败分支仅依赖 warning 收集逻辑，未通过 fake runner 做具体 stderr 映射验证。该缺口可能让 future 修改破坏 CLI 诊断文本而不被测试捕获。建议补充至少一例 disable 与 daemon-reload 非 0 退出的 fake 调用，断言 warnings 中的字符串保持预期格式。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
Fixed in 86120059. Added teardown_preserves_disable_and_reload_failure_warnings, which scripts a successful stop followed by non-zero disable and daemon-reload calls, asserts both exact CLI warning strings in order, and proves reload still runs after disable fails. Focused verification now passes 16/16 CLI system tests and 12/12 platform systemd tests; changed-crate Clippy and workspace docs also pass.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
- Route service mutations through the typed systemd process boundary. - Preserve command order while removing diagnostic-string state checks. - Keep filesystem, socket, and other host probes outside this slice. Signed-off-by: 空澈 <kongche.jbw@alibaba-inc.com>
e7f1638 to
8612005
Compare
ikunkun-sys
left a comment
ikunkun-sys
left a comment
There was a problem hiding this comment.
Reviewed 86120059. Found one non-blocking P2; no P0/P1 findings.
| None => layout.libexec_dir.join("anolisa-system-helper"), | ||
| }; | ||
| let unit_path = layout.systemd_unit_dir.join(UNIT_FILENAME); | ||
| let systemd = Systemd::system(); |
There was a problem hiding this comment.
[P2] 保留 systemctl 诊断的调用方 locale
这里改用 Systemd::system() 后，setup/teardown 的 systemctl 进程会经过 SystemCommandRunner；该 runner 在 command.rs:53 无条件设置 LC_ALL=C。旧的 Command::new("systemctl") 继承调用方 locale，而 systemd_cli_error 又会把 failure.stderr 原样放进用户可见的 setup 错误和 teardown warning，因此本次重构会把非 C locale 下的诊断改成英文，违背 #2718 的“human output 不变”约束。可复现：同一个 bus error 在 LC_ALL=de_DE.utf8 下包含 Die Operation ist nicht erlaubt，在 LC_ALL=C 下则为 Operation not permitted。请避免让 runner 的 rpm/dnf locale 策略改变这些 systemd 路径的可见输出。
ikunkun-sys
left a comment
ikunkun-sys
left a comment
There was a problem hiding this comment.
Requesting changes for the unresolved user-visible locale regression documented in #2724 (comment). The current head remains 86120059, and the finding is still current and unresolved.
2 participants
Why
anolisa system setupandsystem teardownstill spawnedsystemctldirectly even after the typed systemd process boundary was introduced. This left privileged service sequencing dependent on host commands in tests and made teardown infer a missing unit from diagnostic strings.What changed
Systemd<CommandRunner>boundary with the narrow daemon-reload, enable, start, restart, stop, and disable operations required by the system command.Related issue
closes #2718
User / Agent impact
No CLI arguments, human output, warning text, JSON schema, or exit codes change. System setup and teardown now use deterministic typed service evidence internally.
Risk and compatibility
The source-level API addition is limited to the internal
anolisa-platformworkspace crate. Privileged setup and teardown keep their existing systemctl argv and ordering; no state, protocol, or configuration migration is introduced.Validation
Validated on macOS arm64 from
src/anolisa/:cargo fmt --all -- --checkcargo check --lockedcargo test -p anolisa-platform systemd::tests --locked— 12 passedcargo test -p anolisa-cli commands::system::tests --locked— 16 passedcargo clippy -p anolisa-platform --all-targets --locked -- -D warningscargo clippy -p anolisa-cli --all-targets --locked --no-deps -- -D warningscargo doc --workspace --no-depsThe local full workspace Clippy gate remains blocked by pre-existing dead-code warnings in
anolisa-core/src/capability.rs. Linux CI is the authoritative full-workspace and live-systemd platform evidence.Documentation and rollback
No documentation change is required because public command behavior is unchanged. Rollback is a direct revert of this commit; no persisted state or configuration migration is involved.
Ship Note
cargo test -p anolisa-platform systemd::tests --lockedandcargo test -p anolisa-cli commands::system::tests --locked.