Re: BUG: Zsh loses history entries since 2015

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Tue, Apr 15, 2025 at 2:58 AM Michael Stapelberg
<stapelberg+zsh@xxxxxxxxxx> wrote:
>
> > > To be clear: the zsh processes that are killed in my scenario do not
> > > have any pending history to save
> >
> > How do you know this?  Because if this could be programmatically
>
> 1. I can observe this from the bpftrace logs I collect:
>    non-interrupted zsh exits read + write the same number of history bytes.
> 2. I use INC_APPEND_HISTORY and do not leave half-entered commands
>    in my various shell sessions. Hence, I know there is no pending history.

OK, INC_APPEND_HISTORY is key here.

> > determined at time of exit, the entire history operation could be
> > optimized away.
>
> I don’t think it can, unless we’re okay with skipping the size and duplicate
> enforcement, which only happens at exit time?

Those also happen during the incremental append if the number of
entries has reached 20% larger than SAVEHIST.

So, we could just let the "20% rule" always control whether this
happens, and skip the write-back at exit.  Not going to happen for
this upcoming release, though, and probably would need an option
setting.

> > I think there's an opportunity to do a little more error-handling
> > specifically when readhistfile() has failed during savehistfile().
>
> Thanks, I can confirm that your patch fixes the issue.

Great!  This is going to have the effect of leaving the file a bit
oversized sometimes (with INC_APPEND) or lose current shell history
(without).

> > When savehistfile() is doing straightforward write-back, it only stops
> > if fprintf() or fputc() returns -1.  It feels like there's a potential
> > race condition there
>
> You mean receiving a SIGHUP while in fprintf() or fputc()?

Any signal that prevents the underlying system call from finishing.
SIGHUP in particular, unless being blocked/deferred, is just going to
cause the shell to exit.

> So, should we handle EINTR?

We do.  That's what ERRFLAG_INT signifies.

> > I further suspect there's a logic error at the end of the
> > readhistline() loop in readhistfile(), proposed fix in the patch.
>
> Which logic error do you mean specifically? (It’s not clear from the patch.)

The first hunk of diff that moves the "return" outside the loop body.
The original code catches interrupts if they happen during the body,
but not if they happen during the loop condition.