fix: honour store_failed for negative exit codes and daemon mode#3495
Open
jerry7991 wants to merge 3 commits into
Open
fix: honour store_failed for negative exit codes and daemon mode#3495jerry7991 wants to merge 3 commits into
jerry7991 wants to merge 3 commits into
Conversation
The `store_failed = false` setting was leaking two classes of failed commands into history. 1. Commands killed by a signal record a negative exit code (PR atuinsh#821), but `handle_end` only filtered when `exit > 0`, so signal kills like Ctrl+C-interrupted runs were kept. 2. The daemon's `end_history` RPC never consulted `store_failed` at all, so when `daemon.enabled = true` every failed command was persisted regardless of the user's preference. Switch the local check to `exit != 0` and add the same gate inside the daemon's `end_history` handler (it already has access to live settings via `DaemonHandle::settings`). Cover both paths with unit tests for exit code 0, positive, and negative.
Contributor
Fossier: Manual Review Requested
Score BreakdownTotal Score: 67.1/100 | Confidence: 100% | Outcome: REVIEW
|
Contributor
Greptile SummaryThis PR makes
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "fix: honour store_failed for negative ex..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
store_failed = falsewas still keeping two classes of failed commands in history:crates/atuin/src/command/client/history.rs:511usedexit > 0, so anything that came in as a negative exit (e.g. Ctrl+C-interrupted runs, which atuin already supports recording per fix: record negative exit codes #821) slipped past the filter.EndHistoryRPC handler never consultedstore_failed, so users runningdaemon.enabled = truehad every non-zero exit persisted regardless of their setting.Changes
handle_end: change the predicate toexit != 0so negative exit codes are also dropped.atuin-daemonhistory component: checkhandle.settings().await.store_failedinsideend_history; when it'sfalseand the exit is non-zero, remove the entry from the running map and return early without writing to the local DB or pushing to the record store.(store_failed, exit)combinations: failed+disabled, signal-killed+disabled, success+disabled, failed+enabled.store_failedconfig doc to mention signal kills and daemon mode.Why
Reported by a user who had
store_failed = falsebut still saw failed commands appearing inatuin search. The behaviour was due to the two gaps above. The local-path bug was introduced in #2284 when the original!=was tightened to>on the (incorrect) assumption that exit codes are always 0–255; the daemon-path gap was there from the original daemon implementation.Test plan
cargo test -p atuin --bin atuin handle_end— 4 new tests passcargo test -p atuin --bin atuin— full atuin suite green (218 passed)cargo test -p atuin-daemon— daemon lifecycle suite green (5 passed)cargo clippy -p atuin -p atuin-daemon --no-deps— cleancargo fmt --check -p atuin -p atuin-daemon— clean