Intermittent history overwrite due to setInterval race condition

[pjeby]

Steps to reproduce

1. From an open note, open a new note, then another (in the same pane)
2. Go back in history

Expected result

The second note is displayed

Actual result

Sometimes, the first note is displayed – i.e., the third note replaced the second note in history. This most often happens when I open today’s daily note for the first time, though not every day. (It’s also possible that it happens other times that I’m not noticing: the daily note scenario is one where I commonly navigate forward/back a lot and know exactly what note should appear when I go backward.)

It happens just often enough to be vexing, and has been happening since the 0.10.x series, but I never reported it because I could never figure out how to reproduce it on purpose.

Now, however, I have been able to figure out what is happening by tracing through the code in the debugger, so I can at least report the cause, as I will describe below.

Environment

• Operating system: Windows
• Obsidian version: 0.12.3

---

Additional information

Here’s what’s happening, as far as I can tell:

1. In the middle of setViewState(), after the view has updated its current file but before updateStateResult() is invoked, the 1-second setInterval for updateHistory fires, requesting a history save for the active leaf.
2. updateHistory calls recordHistory with a flag indicating the current state should be replaced – which causes the presence of note 2 to be deleted from history, replaced with an entry for note 3
3. setViewState resumes execution, and eventually gets around to calling updateStateResult(), which calls recordHistory with a flag indicating a new state should be added to the history for note 3
4. recordHistory() detects that the state to be added to history is identical to the current state in history (saved in step 2), and does nothing. The net result is that note 3 has replaced note 2 in the history, leaving a history of [note 1, note 3] instead of [note 1, note 2, note 3] as should be the case.

I’m planning to add a workaround for this in the pane-relief plugin, by turning a replaceState into a pushState if an attempt is made to change the current file for the active leaf. But it would probably be a good idea for this to be fixed in Obsidian as well.

ISTM that one way to do that would be to perform a pushState at the beginning of setViewState, before any await can happen. The state to be pushed is known, after all, as is whether the push is needed. At the end of the operation, a request to replaceState could then be issued, which would fix any discrepancy between the state passed to setViewState and the view’s final state. And there’d be no way for the setInterval of updateHistory to interfere, since it would just update the already-pushed state, and the final replaceState would be a no-op, as it is now.

[pjeby]

A bit of a follow-up.

I added the workaround code in pane-relief and included a debugger breakpoint so I could check and see if I could catch the behavior “in the act” as it were. I was able to successfully reproduce the behavior three or four times by clicking on a dozen dates using the Calendar plugin, while the inspector was open.

It turns out that if you want to reproduce this, the best thing to do is perform an operation that takes a while in synchronous code before yielding, as that seems to make it more likely the 1 second interval will have elapsed before updateStateResult has a chance to run. Having the inspector open slowed things down a bit, and apparently something the Calendar plugin does helps slow things down as well.

Anyway, the tl;dr is that I have now confirmed my prior theory as to how this is happening, because the code that does the faulty replaceState is definitely being called directly from the interval event, not from inside any other function. And users affected by this issue can use pane-relief 0.0.5 and up as a workaround until the race condition is addressed in core.

[rpcm]

Seeing issues like this makes me wish parts of the codebase were open so community members could fix these. Although I understand the reasoning behind keeping it proprietary for now.

[Licat]

Thanks for the detailed report and analysis. I’ve been able to repro and I have a fix for this in the upcoming insider release.