Skip to content

fix(skillfs): preserve SLS logs on panic#1508

Open
kongche-jbw wants to merge 1 commit into
alibaba:mainfrom
kongche-jbw:fix/skillfs/sls-broken-pipe
Open

fix(skillfs): preserve SLS logs on panic#1508
kongche-jbw wants to merge 1 commit into
alibaba:mainfrom
kongche-jbw:fix/skillfs/sls-broken-pipe

Conversation

@kongche-jbw

Copy link
Copy Markdown
Collaborator

Description

SkillFS list, classify, validate, and mount can lose their SLS ops
records when a downstream pipe closes before log_command is reached. This
change arms a single RAII SlsOpsGuard immediately after CLI parsing, before
tracing emits output, and finishes it eagerly on normal or error returns.
Panic unwinding records err_reason="panic", while validate preserves its
record before process::exit. Deterministic reader-less pipe tests cover both
stdout-only and merged stdout/stderr failure paths.

Related Issue

closes #1506

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional change)
  • Performance improvement
  • CI/CD or build changes

Scope

  • cosh (copilot-shell)
  • cosh-ng (cosh-ng)
  • sec-core (agent-sec-core)
  • skill (os-skills)
  • sight (agentsight)
  • tokenless (tokenless)
  • ckpt (ws-ckpt)
  • memory (agent-memory)
  • anolisa (anolisa-cli)
  • skillfs (SkillFS)
  • Multiple / Project-wide

Checklist

  • I have read the Contributing Guide
  • My code follows the project's code style
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly
  • For cosh: Lint passes, type check passes, and tests pass
  • For cosh-ng: cargo clippy --all-targets -- -D warnings and cargo fmt --check pass
  • For sec-core (Rust): cargo clippy -- -D warnings and cargo fmt --check pass
  • For sec-core (Python): Ruff format and pytest pass
  • For skill: Skill directory structure is valid and shell scripts pass syntax check
  • For sight: cargo clippy -- -D warnings and cargo fmt --check pass
  • For tokenless: cargo clippy -- -D warnings and cargo fmt --check pass
  • For memory (Linux only): cargo clippy --all-targets -- -D warnings, cargo fmt --check, and cargo test pass
  • For anolisa: cargo clippy --all-targets --locked -- -D warnings, cargo fmt --all --check, and cargo test --locked pass
  • For skillfs: cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace pass
  • Lock files are up to date (package-lock.json / Cargo.lock)

Testing

Executed on Linux with Rust 1.86.0 and the system FUSE 3 development package:

cargo +1.86.0 test -p skillfs --test sls_ops_cli_tests
# 13 passed, 0 failed

cargo +1.86.0 fmt --all -- --check
cargo +1.86.0 clippy --workspace --all-targets -- -D warnings
cargo +1.86.0 test --workspace
cargo +1.86.0 doc --workspace --no-deps
scripts/test.sh

The FUSE smoke suite performed real mounts and verified unmounting, managed
supervisor recovery, orphan cleanup, and fast-failure cleanup. No FUSE checks
were skipped.

Additional Notes

  • The guard covers panic unwinding; abort and SIGKILL cannot run destructors.
  • Stop and Supervise remain internal commands without SLS ops records.
  • No dependencies, lock files, public APIs, or user documentation changed.
  • Existing unrelated rustdoc warnings remain unchanged.

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

  • SlsOpsGuard 未覆盖 logging 初始化阶段的潜在 broken pipe panic,可能仍遗漏早期 SLS 记录。
  • validate 子命令通过 finish_sls 使用 guard 的方式缺乏类型约束,未来改动易引入重复记录。
  • 回归测试中使用 unsafe libc 管道操作时错误处理和 fd 清理不够严谨,建议收紧安全边界。

🤖 Generated by QoderView workflow run

Comment thread src/skillfs/crates/skillfs-cli/tests/sls_ops_cli_tests.rs
- Arm an RAII SlsOpsGuard in main before logging init, so a broken
  stdout/stderr pipe closing before the first write still logs one record.
- finish() writes eagerly and disarms, keeping validate's pre-exit record
  that a Drop-only guard would lose to process::exit skipping destructors.
- Covers list/classify/validate/mount and panic unwinding only, not
  abort/SIGKILL, which never run drops.

Signed-off-by: kongche-jbw <kongche.jbw@alibaba-inc.com>
@kongche-jbw kongche-jbw force-pushed the fix/skillfs/sls-broken-pipe branch from c07e455 to ceccf7b Compare July 16, 2026 02:54
@kongche-jbw

Copy link
Copy Markdown
Collaborator Author

已在 ceccf7b 中处理文件描述符清理问题。

pipe() 成功后会立即将两个文件描述符包装为 OwnedFd,dup()
成功后也会立即包装复制出的文件描述符。因此,后续发生 panic
或 spawn 失败时,所有已创建的文件描述符都会通过 RAII 自动关闭。
读取端现在通过 drop() 显式关闭,系统调用失败信息也补充了
last_os_error()。

这里没有增加 EINTR 重试或引入 nix 依赖:该逻辑仅用于测试,
系统调用失败应直接使测试失败;OwnedFd 已保证失败路径不会泄漏
文件描述符。

已使用 Rust 1.86.0 重新验证:

  • cargo test -p skillfs --test sls_ops_cli_tests:13 项通过
  • cargo clippy --workspace --all-targets -- -D warnings:通过
  • cargo test --workspace:通过
  • scripts/test.sh:通过

/// Reproduce `skillfs <cmd> 2>&1 | <reader that closes immediately>` (the
/// issue's own repro). Creates a pipe and closes its read end *before*
/// spawning, so the child inherits a reader-less pipe on both stdout and
/// stderr. Its very first write — the logging line emitted in `main` before

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.

此处(以及 SlsOpsGuard 文档注释 main.rs:542-543)的论证与实际行为略有出入。

注释称"child 的第一次写入 —— main 中在命令体之前发出的日志行 —— 命中 EPIPE 并 panic"。但 main 里的 info!(pid, "starting skillfs CLI") 是经由 tracing 的 fmt subscriber 写到 stderr 的,而 tracing 的 writer 会吞掉写入错误(EPIPE),并不会 panic。因此合并输出场景下真正触发 panic 的并不是这行日志。

实际情况:

  • list/classify/validate:panic 发生在命令体里的第一个 println!
  • mount:panic 发生在 cmd_mount 里挂载前的 eprintln! 提示横幅(main.rs:1826-1889),恰好在 FUSE 挂载调用之前——这也是该用例"不残留挂载"性质的实际来源。

测试断言(1 条记录、err_reason="panic")依然正确,但"logging line ... hits EPIPE and panics"的措辞会误导读者,且 mount 用例"不残留挂载"其实隐式依赖"横幅在挂载之前"这一顺序。建议把注释改为描述实际的 panic 位置,或直接删掉对具体 panic 点的断言性描述。

在 logging 初始化前 arm guard 这一做法本身是对的(对未来新增的 stdout 直写是保险),此点无需改动。

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[nightly][skillfs] CLI subcommands list/classify don't write SLS ops log when stdout piped (SIGPIPE panic kills process before log_command)

2 participants