fix(shell): fix incorrect timing of child shells

[akinomyoga]

When a child shell session is started from another shell session (let us call this parent session hereafter), the environment variable ATUIN_HISTORY_ID set by the parent session causes Atuin's precmd hook of the child session to be unexpectedly performed before the first call of Atuin's preexec hook.

For example, let us consider the case to run bash inside the terminal to start a new session.

[parent]$ bash
# 1. [atuin history start] is called for "bash" by the preexec in the parent session.
# 2. The command "bash" is run.
# 3. [atuin history end] is called for "bash" by the precmd in the child session (This is the UNEXPECTED one).
[child]$ echo some-commands
[child]$ exit
# 4. the preexec hook for "exit" is called in the child session
# 5. The command "exit" is run, and the child session is terminated.
# 6. [atuin history end] is called for "bash" by the precmd in the parent session. (This is the EXPECTED one but has no effect because of 3)

In this patch, we clear ATUIN_HISTORY_ID (possibly set by the parent session) on the startup of the session. The variable ATUIN_HISTORY_ID doesn't seem to be an environment variable in the fish integration, but we clear it also in the fish integration for consistency.

Possible drawback

With this solution, when the user runs source .bashrc (or eval "$(atuin init bash)") in the command line, the ATUIN_HSITORY_ID for source .bashrc is lost and the timing for that command is not recorded. However, the same happens for the exit command. In the sense that source .bashrc terminates the existing ATUIN_SESSION (and creates another), it is similar to exit. For this reason, we might not need to care about it more than exit.

Other possible solutions

However, another possible option would be not to make ATUIN_HISTORY_ID the environment variable from the beginning. As far as I search in the codebase, there does not seem to be a code that reads the environment variable ATUIN_HISTORY_ID from other processes. So I do not see the necessity to make the variable an environment variable.

Or, with the current main, we are effectively measuring the startup time of the child shell. Maybe we can keep the current code if this is the intended one, but then, this seems inconsistent: In this way, only the startup times of the child sessions are measured, but the startup time of the parent session (the login shell started by the terminal) is not measured. Also, this does not work in the current integration with the fish shell because ATUIN_HISTORY_ID doesn't seem to be an environment variable in the fish integration.

Review request

In this patch, I touch the integrations with all the shells, but I'm only familiar with Bash. If possible, I'd like reviews from the contributors to the integrations with the other shells: @ellie (for zsh, etc.), @conradludgate (for fish, etc.), @stevenxxiu (for nu), and @sophiajt (for env in nu)

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
atuin-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 6, 2024 1:18pm

[ellie]

I'll have a proper dig into this one soon - might be worth pinging @arcuru to check the fish stuff 🙏

[ellie]

Looks like a reasonable fix to me, thank you!

As far as I search in the codebase, there does not seem to be a code that reads the environment variable ATUIN_HISTORY_ID from other processes.

Out of scope for this PR, but you're correct. It's only used within the shell integration, and not read from other processes

[akinomyoga]

Thank you!

[arcuru]

Sorry I didn't see this ping until now.

The fish change doesn't hurt, but is unnecessary (EDIT: see below). In #1370 I fixed this issue from happening in fish by changing the ATUIN_HISTORY_ID variable to not be exported to child processes. set -gx to set -g.

EDIT: Nevermind, the change to fish is necessary. If you, for example, run a fish shell from bash it would inherit that variable from the bash setup, so you'd probably be hitting all the same issues.

[sophiajt]

I'm out for a bit, but I'll tag in someone from the Nushell team.

[IanManske]

Just tested the changes for nushell, and it looks like the fix works as intended! (3. is no longer triggered.)