Skip to content

fix: keep release panic=unwind so filter catch_unwind fails open - #2401

Closed
amitgaur-zengineer wants to merge 2 commits into
rtk-ai:developfrom
amitgaur-zengineer:fix/release-panic-unwind-filter-failopen
Closed

amitgaur-zengineer wants to merge 2 commits into
rtk-ai:developfrom
amitgaur-zengineer:fix/release-panic-unwind-filter-failopen

Conversation

@amitgaur-zengineer

@amitgaur-zengineer amitgaur-zengineer commented Jun 11, 2026 •

Copy link
Copy Markdown

Summary

  • [profile.release] sets panic = "abort", which makes std::panic::catch_unwind a no-op — so the filter fail-open recovery in core::stream and cmds::system::pipe_cmd ("filter panicked — passing through raw output") is dead code in release builds. Any panic inside a filter aborts the whole process with SIGABRT instead of degrading to raw passthrough (rtk crashes (SIGABRT) on gcloud compute instances list --format=… — poisoned Once cascade #2400; seen in the wild on gcloud … list --format=…).
  • Fix: set [profile.release] panic = "unwind" so the existing catch_unwind actually fails open. Adds a manifest-level regression guard (the normal suite can't catch this — cargo test always builds with unwind, which is why it shipped).
  • Also: run_fallback applied the matched TOML filter (core::toml_filter::apply_filter) with no catch_unwind, unlike the streaming/pipe paths. Wrapped it in a filter_or_passthrough helper so a filter panic there also fails open (and the default panic hook prints the location, which helps diagnose the underlying filter bug).

Test plan

  • cargo fmt --all && cargo clippy --all-targets && cargo test — fmt clean, clippy clean, 2153 passed / 0 failed (stable 1.96.0)
  • cargo test --test release_profile_panic — passes; verified it FAILS when panic is flipped back to abort
  • cargo test filter_or_passthrough — new fallback fail-open tests (panic → passthrough; normal → filtered value)
  • cargo test test_panicking_filter_returns_passthrough — existing fail-open test still passes

Trade-off: panic = "unwind" slightly increases binary size (landing pads); it does not affect startup time. Correctness + the fail-open behavior the codebase already implements/tests outweigh the size delta.

Closes #2400

panic = "abort" in [profile.release] made std::panic::catch_unwind a no-op
in shipped binaries, so a panic inside any filter (core::stream,
cmds::system::pipe_cmd) aborted the whole process with SIGABRT instead of
degrading to raw passthrough. Tests did not catch it because cargo test
builds with unwind. Add a manifest-level regression guard.

Closes rtk-ai#2400
@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.
You have signed the CLA already but the status is still pending? Let us recheck it.

run_fallback applied the matched TOML filter via core::toml_filter::apply_filter
with no catch_unwind, unlike the streaming and pipe paths. Wrap it in a
filter_or_passthrough helper so a filter panic degrades to raw passthrough
(and prints the panic location) instead of propagating out of the fallback.

Refs rtk-ai#2400
kylehgc added a commit to kylehgc/rtk that referenced this pull request Jul 23, 2026
- Conflict list: drop rtk-ai#2579/rtk-ai#2590/rtk-ai#2615 (already flagged 07-17), add
  rtk-ai#2401/rtk-ai#2453 and the chunk-7 conflict cluster; note top-pick rtk-ai#2401 is
  a hand-port (issue #54).
- Next-wave: restore recovered candidate rtk-ai#2256 (gh prompt-injection
  scan) dropped in distillation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@HetCreep

HetCreep commented Aug 4, 2026

Copy link
Copy Markdown

The diagnosis here matches what I found independently in an audit of v0.44.2 (700bdde), and this PR already covers the parts I would have raised — including that cargo test always builds with unwind, which is why it shipped. Two things I did not see covered, offered in case they belong in this PR or a follow-up.

Provenance: AI-assisted source audit, human-directed; the two points below are read from source at 700bdde.

1. A fourth filter path still has no guard: run_captured_filter

This PR wraps run_fallback's TOML filter, joining the existing guards in core::stream and cmds::system::pipe_cmd. But the buffered command path appears to be unguarded:

src/core/runner.rs:130 — run_filtered → run_captured_filter calls filter_fn(text_to_filter, exit_code) directly, with no catch_unwind.

That is the path a large number of the src/cmds/** filters take. With panic = "unwind" restored, a panic there still unwinds out of main rather than failing open — the child has already run, so its side effects happened, but its output is discarded, nothing is teed, and no tracking row is written.

2. The core::stream guard may not be reachable in production at all

Worth double-checking before relying on it as one of the existing fail-open sites: the catch_unwind at src/core/stream.rs:450 sits inside the FilterMode::Buffered arm, and the only constructor of FilterMode::Buffered I could find is at src/core/stream.rs:729, inside #[cfg(test)] mod tests. Every production call site I traced (runner.rs:107,188,209, uv_cmd.rs:69, search.rs:459, git.rs:1337) passes Passthrough, Streaming, or CaptureOnly.

If that is right, then even after this PR the streaming/buffered guard would be protecting a variant that production never constructs, and pipe_cmd plus the new filter_or_passthrough would be the only live ones. I may have missed a constructor — but if not, it changes what "the existing fail-open recovery" covers.

Concrete case this would still hit

src/cmds/js/prisma_cmd.rs:319-320 computes let end = line[pos..].find(char::is_whitespace).unwrap_or(20); then slices line[pos..pos+end]. When the token is last and shorter than 20 bytes, the slice runs past the end. I reproduced the panic with a verbatim extract:

panicked at prisma_cmd.rs: end byte index 59 is out of bounds for string of length 47

filter_migrate_status is routed from src/main.rs:2198 through the buffered path, so under this PR it would unwind out of an unguarded frame. (Line 252 of that same file uses the correct unwrap_or(line.len() - pos).)

Not asking to widen this PR — it is coherent as it stands, and the profile fix is the load-bearing part. Just flagging what I believe remains open after it lands.

@amitgaur-zengineer

Copy link
Copy Markdown
Author

Closing stale PR — superseded or no longer planned. Happy to reopen if this work is picked back up.

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.

rtk crashes (SIGABRT) on gcloud compute instances list --format=… — poisoned Once cascade

3 participants