Skip to content

fix(sync): skip records that fail to decrypt or decode instead of failing the whole store - #3569

Merged
ellie merged 3 commits into
mainfrom
push-llwrwytqoupy
Jul 7, 2026
Merged

fix(sync): skip records that fail to decrypt or decode instead of failing the whole store#3569
ellie merged 3 commits into
mainfrom
push-llwrwytqoupy

Conversation

@ellie

@ellie ellie commented Jul 7, 2026

Copy link
Copy Markdown
Member

Previously, we chose to fail loudly when there was a key mismatch in a users store. However, in the past, we did not check the key at login, so users could easily end up in a state where they inserted an invalid key, and then broke their store.

This PR ensures that even with a partially invalid store, the user can still sync their data.

This does not mean that users will be able to introduce new invalid keys, and simply makes things better for older users (pre key-checking). We still need to figure out a more straightforward way for a user to repair their entire record store if they have somehow mixed up keys, other than the purge + push/pull --force steps.

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@ellie

ellie commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@greptile-apps plz sir may i have one review 🙏

@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Reviewed PR #3569 which converts hard decrypt/decode failures across all record stores into skip-and-warn behavior. Found one P1 issue: sync::build() uses eprintln! for store-level failures while the rest of the PR uses tracing::warn!, and this function can be called during interactive shell sessions.

Confidence Score: 4/5

Safe to merge; the only concern is that build-level failures in sync.rs still use eprintln! and can surface in interactive sessions.

The store-level changes are correct and well-tested. The one gap is sync.rs, which uses eprintln! for build failures while the same function is called from interactive history sessions.

crates/atuin/src/sync.rs — eprintln! should be tracing::warn! to match the rest of the PR and avoid terminal pollution in interactive sessions.

Important Files Changed

Filename Overview
crates/atuin-client/src/history/store.rs Converts decrypt/decode failures from hard errors to skips with tracing::warn; adds test for corrupt-record resilience; incremental_build follows the same pattern correctly using the outer-loop id variable.
crates/atuin/src/sync.rs build() now swallows per-store failures and logs them with eprintln!, but individual-record errors inside each store use tracing::warn — inconsistent, and eprintln! can surface in interactive sessions triggered from history.rs.
crates/atuin-dotfiles/src/store.rs aliases() now skips corrupt records with tracing::warn; test added mirrors the pattern in history store.
crates/atuin-dotfiles/src/store/var.rs vars() applies same skip-and-warn pattern; consistent with alias store changes.
crates/atuin-kv/src/store.rs KV store build now skips undecryptable records; skipping a newer Delete record leaves an older Create visible, but this is the accepted trade-off for a mixed-key store.
crates/atuin-scripts/src/store.rs scripts() applies the same pattern; consistent with other stores.

Reviews (2): Last reviewed commit: "review fixes" | Re-trigger Greptile

Comment thread crates/atuin-client/src/history/store.rs
@ellie
ellie marked this pull request as ready for review July 7, 2026 06:33
@ellie
ellie requested a review from BinaryMuse July 7, 2026 06:34

@BinaryMuse BinaryMuse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable. Do we have a good way to surface to the user that something went wrong other than them looking in the logs?

@ellie
ellie merged commit 8eee5c8 into main Jul 7, 2026
25 of 26 checks passed
@ellie
ellie deleted the push-llwrwytqoupy branch July 7, 2026 21:50
taylordotfish pushed a commit that referenced this pull request Jul 9, 2026
### Bug Fixes

- *(ai)* Dispatch skills registered in the slash command registry
([#3593](#3593))
- *(ci)* Fossier install in scan workflow
([#3485](#3485))
- *(i18n)* Fix typos in Russian localization
([#3575](#3575))
- *(nu)* Use `char -u 1b` for ESC in OSC 133 sequences
([#3530](#3530))
- *(nu)* Suppress error when `ATUIN_HISTORY_ID` is missing in
`pre_prompt` hook
([#3587](#3587))
- *(pi)* Observe tool events instead of registering a bash tool
([#3557](#3557))
- *(pty-proxy)* Set `$SHELL` to the spawned shell
([#3548](#3548))
- *(search)* Fix terminal clearing with latest Ratatui
([#3578](#3578))
- *(sync)* Skip records that fail to decrypt or decode instead of
failing the whole store
([#3569](#3569))
- Atuin hangs when attempting to spawn daemon from Ctrl+R invocation
([#3502](#3502))
- Capture session ID from stream headers rather than final event
([#3531](#3531))
- Doctor resiliency fo runknown platforms + openbsd warning
([#3551](#3551))
- Double input on arrow keys in AI setup prompt on Windows
([#3552](#3552))
- Exclude AI agent commands from zsh-autosuggestions
([#3567](#3567))
- Silence shellcheck SC2016 on literal `$all-user` author filter
- Respect `store_failed` when using daemon
([#3571](#3571))


### Documentation

- Highlight `Ctrl-r` keybinding on docs page
([#3489](#3489))
- Document store purge workflow
([#3544](#3544))
- Fix command example typo in documentation
([#3536](#3536))
- Make commented-out lines in `config.toml` match real defaults
([#3583](#3583))
- Add fish shell cleanup step to uninstall instructions
([#3582](#3582))


### Features

- *(doctor)* Add whether daemon is enabled to `doctor` output
([#3572](#3572))
- *(pty-proxy)* Add `--shell` flag to override the spawned shell
([#3327](#3327))
- Setup fossier to stop bot slop prs
([#3482](#3482))
- Capture command output + expose to new `atuin_output` tool
([#3510](#3510))
- Cache user contexts on load until `/reload`
([#3525](#3525))
- Create database integration tests for atuin-server
([#3514](#3514))
- Add `/model` slash command for changing models
([#3576](#3576))
- Add mcp server for history tools and expand search filters
([#3581](#3581))
- Add status bar with model and usage information
([#3591](#3591))


### Miscellaneous Tasks

- *(rustdoc)* Fix Rustdoc warnings
([#3585](#3585))
- *(warnings)* Fix compile warnings with latest dependencies
([#3586](#3586))
- Vouch for all existing contributors
([#3486](#3486))
- Update GitHub app token format
- Update to Rust 1.96.1
([#3568](#3568))
- Adopt `derive_more` to reduce boilerplate across the codebase
([#3573](#3573))


### Performance

- *(search)* Scan history by recency until N unique
([#3553](#3553))
- Add `synchronous(Normal)` + `optimize_on_close` to record store SQLite
([#3577](#3577))
- Remove unnecessary clones in a hot path
([#3580](#3580))


### Refactor

- Implement `From<sqlx::Error>` and clean up `fix_error`
([#3484](#3484))
- Pull `fn into_utc` into `atuin-server-database` crate
([#3487](#3487))
@davidolrik

Copy link
Copy Markdown
Contributor

We still need to figure out a more straightforward way for a user to repair their entire record store if they have somehow mixed up keys, other than the purge + push/pull --force steps.

@ellie: I have a solution for this in #3429, which adds atuin store repair. - It has just been rebased on top of main

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.

3 participants