Skip to content

fix(skillfs): ensure SLS ops log written even on SIGPIPE panic#1507

Open
zhangtaibo wants to merge 1 commit into
alibaba:mainfrom
zhangtaibo:fix/skillfs-sls-panic-guard
Open

fix(skillfs): ensure SLS ops log written even on SIGPIPE panic#1507
zhangtaibo wants to merge 1 commit into
alibaba:mainfrom
zhangtaibo:fix/skillfs-sls-panic-guard

Conversation

@zhangtaibo

Copy link
Copy Markdown
Contributor

问题

Fixes #1506

skillfs CLI subcommands list and classify do not write SLS ops log entries when stdout is piped to a consumer that closes early (e.g. head -5). The log_command() call runs after all println! output, but println! panics on broken pipe (SIGPIPE/EPIPE) when the downstream reader has already exited, causing the process to unwind before reaching log_command().

This is a regression from the original SLS ops logging fix (commit 1a07b20, issue #1306). The log_command calls were added to all subcommands, but they are unreachable when the process panics from a broken stdout pipe.

修复

Add an RAII SlsLogGuard that calls sls_ops::log_command() in its Drop implementation. The guard is created before the command runs and dropped on all exit paths — including panic unwinds from broken pipe. This ensures the SLS entry is always written regardless of stdout pipe state.

+struct SlsLogGuard {
+    ops_name: String,
+    start: std::time::Instant,
+    err_reason: std::cell::Cell<Option<String>>,
+    completed: std::cell::Cell<bool>,
+}
+
+impl Drop for SlsLogGuard {
+    fn drop(&mut self) {
+        let reason = if self.completed.get() {
+            self.err_reason.take()
+        } else {
+            Some("panic".to_string())
+        };
+        sls_ops::log_command(&self.ops_name, self.start, reason);
+    }
+}

Each command handler replaces its manual log_command call with guard creation:

let _guard = SlsLogGuard::new("list");
let result = cmd_list(source, enabled_only).await;
_guard.set_reason(err_reason(&result));
result

验证

# ECS fresh clone + apply patch + build
cd /root && rm -rf anolisa && git clone https://github.com/alibaba/anolisa.git && cd anolisa
git apply skillfs-sls-panic-guard.patch
bash scripts/rpm-build.sh skillfs
# → exit 0, RPM 2.0M

# Install patched RPM and test
rpm -U --force scripts/rpmbuild/RPMS/x86_64/skillfs-0.3.3-1.alnx4.x86_64.rpm
touch /var/log/anolisa/sls/ops/skillfs.jsonl
> /var/log/anolisa/sls/ops/skillfs.jsonl

# Before fix: list piped to head -5 → 0 bytes (no SLS entry)
# After fix: list piped to head -5 → 180 bytes (SLS entry written!)
skillfs list /usr/share/anolisa/runtime/skills 2>&1 | head -5
stat -c '%s' /var/log/anolisa/sls/ops/skillfs.jsonl
# 180

# All 3 parametrized tests pass:
pytest tests/test_sls_ops_emission.py::test_skillfs_cli_emits_sls_entry -v
# 3 passed in 10.50s

Co-Authored-By: Claude noreply@anthropic.com

skillfs CLI list/classify subcommands don't write SLS ops log entries
when stdout is piped to a consumer that closes early (e.g. head -5).
println! panics on broken pipe, killing the process before
log_command() runs. Add SlsLogGuard RAII guard that calls
log_command() in Drop, ensuring SLS entry is written on all exit
paths including panic unwinds.

Fixes alibaba#1506

Signed-off-by: zhangtaibo <zhangtaobo.ztb@alibaba-inc.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


zhangtaibo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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