Implement code to guard against toggling the incorrect line

[claremacrae]

Expected Behavior

When I toggle a task using the Tasks plugin, I expect the task that is completed to match the text of the task I toggled.

Current Behavior

Very, very occasionally, I find that the task completed is not the correct task.

I do not yet have a reproduction, but it is the exact same behaviour as is observed in #688, except it is never reproducible.

I'm starting this issue as a place-holder, to capture more ideas, and information on it.

Steps to Reproduce

Not yet known. I believe it is a timing issue.

Context (Environment)

Unknown.

Possible Causes

• Sometimes Tasks handles a change after the 2 second file-saving interval. It is possible by then that more than one line has changed, and the line/task numbers in the Task may no longer be correct
• I think I have seen code in Tasks that reads a file from the cache, which may be out-of-date. Once Tasks has read all files, perhaps it should read the file directly, instead of the cached copy?
• Perhaps multiple nearby tasks in a file where changed within a 2 second interval?
• Perhaps the heading information in the task became stale in the 2 second interval?

Possible Solution

• I keep coming back to the idea that when Tasks completes a task, it should do some kind of safety check that the line it is completing has a description that is sufficiently similar to the one in the Task being completed.
• In the initial implementation, it might just pop up a message and say 'I couldn't find that task. Please try to complete it again.
• In a later implementation, it could search the file to see if there was a unique line that matched. This might provide a workaround for both When a block-referenced task is made complete, it is completed after rewriting the contents of another task. #688 and the failure to find tasks within callouts in Live Preview - as discovered in fix: find tasks in blockquotes and Obsidian callouts #953 (comment)

[BluBloos]

I keep coming back to the idea that when Tasks completes a task, it should do some kind of safety check that the line it is completing has a description that is sufficiently similar to the one in the Task being completed.

If the safety check is to ensure the description of the task is sufficiently similar I'm afraid that only partially solves this issue.

currently, afaik (based on reading tryRepetitive in File.ts) task UIDs = (file path + section + section index)

so it seems that the description doesn't have anything to do with what uniquely identifies a task and indeed there may be two unique tasks with identical descriptions.

"Compare task desc" could then only be a heuristic check that goes as, "we are reasonably sure that this task is the same one but not entirely".

---

My current thoughts on a potential solutions are:

1

Ensure that the following sequence of operations occur atomically: 1. read file, 2. create new Task(), 3. modify Task, 4. write new one.

with the new Ob API of vault.process(...) I think that would be a good direction.

2

Change the UID of tasks to actually be originalMarkdown with the current UID as a perf opt for finding it.

---

As for (2.) I think this is particularly risky and goes against the goal to reduce breaking changes. As per Hyrums Law the undocumented but implicit contract of the Tasks plugin is the Task UID that I mentioned above (unless I missed something in the docs).

...

I'm going to continue to investigate further...!!!

[claremacrae]

I have limited capacity to parse the above at the moment, but a few thoughts on first reading:

If the safety check is to ensure the description of the task is sufficiently similar I'm afraid that only partially solves this issue.

Absolutely agree. But it is a low hanging and rather important fruit.

Incidentally, the underlying file reading and writing may change significantly if we move to using datacore as the underlying mechanism. Therefore, I personally am focussing my time on other areas, until we know for sure if we can adopt datacore.

I saw the discussion about vault.process() today and it looks good.

I believe that Tasks in some places uses a cached read before updating a task, and I feel a non-cached read would be a safer thing to.

[BluBloos]

Absolutely agree. But it is a low-hanging and rather important fruit.

Agreed!

[BluBloos]

I'll take a stab at a PR for this. I have some ideas brewing ...

the scope of the PR would be:

1. before writing to a line in a file, check that the line is the exact same as "before". Before is what was used to create the Task.

2. a stab at the timing issues ... seeing if vault.process can help, with a potential dive into what the caching code impl is

there would be an additional test (maybe) to ensure the invertibility of line -> Task. So that Task -> line produces the original line.

[BluBloos]

as for using vault.process it seems I get a Property 'process' does not exist on type 'Vault' so I suspect that some sort of Obsidian dependency needs to be updated?

you can see my branch where I've already got it implemented/works locally: https://github.com/BluBloos/obsidian-tasks/tree/data-integrity

[claremacrae]

as for using vault.process it seems I get a Property 'process' does not exist on type 'Vault' so I suspect that some sort of Obsidian dependency needs to be updated?

I don't know what version vault.process was released in.

But yes, I am behind on updating dependencies in general - the smoke-testing takes time - but specifically there hasn't been any urgency to update Obsidian/CodeMirror deps as we weren't needing any of the newer features.

[BluBloos]

I don't know what version vault.process was released in.

Re: vault.process I've investigated and found it to be released in 1.1.0 of Ob API

[claremacrae]

Re: vault.process I've investigated and found it to be released in 1.1.0 of Ob API

Excellent. If I am reading the release notes correctly, that is still only available for insiders...

https://forum.obsidian.md/c/announcements/13

[BluBloos]

interesting ... I did test locally calling this func, but I'm not an insider so I'll take another look ...

[claremacrae]

interesting ... I did test locally calling this func, but I'm not an insider so I'll take another look ...

Now I'm really doubting myself!

Narrator: She was correct to doubt herself. 🤣

[claremacrae]

The last release announced in the #releases channel in Discord is:

https://discord.com/channels/686053708261228577/694235607810965636/1055894252569231421

[claremacrae]

🤦 I am very sorry for misleading you - I conflated 1.1.0 and 1.1.10... 😊

You are absolutely correct...

So 1.1.0 was released to all users on 20 December 2022.

[claremacrae]

Just to note that with the check you added in #1663, @BluBloos, I have actually once been able to manually trigger the conflict in my own vault.

I need to do some more experimenting, but I am slightly hopeful that with a lot of exploratory testing I might be able to identify the cause of the bug...

I'm not at all sure about the following but....

I wonder about a first release of the above anti-corruption fix, using the current Obsidian dependency... So that anyone who is stuck on an old Obsidian still gets their data protected.

And then a later release with the Obsidian minimum version updated to 1.1.0 for even more robust editing for users able to update...

I would welcome your thoughts...

[BluBloos]

I have actually once been able to manually trigger the conflict in my own vault.

I need to do some more experimenting, but I am slightly hopeful that with a lot of exploratory testing I might be able to identify the cause of the bug...

Just to make sure I understand, a timing issue occurred and the guard didn't get hit? And then some line got overwritten that wasn't supposed to? I'm not sure exactly what you repro on your end ...

[claremacrae]

Just to make sure I understand, a timing issue occurred and the guard didn't get hit? And then some line got overwritten that wasn't supposed to? I'm not sure exactly what you repro on your end ...

No - A timing issue occurred and the guard did get hit! 🎉 ❤️

And I got useful information in the (slightly reformatted) error message.

In maybe 200 to 400 very rapid completions of recurring tasks, spread across multiple sections, I have received the error message twice, preventing overwriting of the line.

I am super happy that I can now detect the issue, enabling me to try some systematic rapid testing and see if I can spot any patterns.

[claremacrae]

Reproduction 1 - delete some lines before the task

in vault (optionally with sync enabled)

• Setup
  • Left pane: Source mode
  • Right page: Reading mode
  • Sync enabled - not needed, see below
• Create a section with, say, 20 tasks in
• Delete a load of tasks before the last task in the section
• Then quickly toggle the last one
• It finds the wrong line, triggering the check added in fix: Prevent rare overwrite of wrong line in Reading mode & results blocks #1663

The cause is that Obsidian will only save-and-re-parse the edited file automatically every 2 seconds, so if you toggle what was the 20th task in a section, but there are no longer 20 tasks in the section, it will pick the wrong one.

I think Sync may compound the problem, as sometimes Obsidian reports that a file has been changed outside of Obsidian when I was only editing it in Obsidian, and my suspicion is that Sync's merging results in an edited file that was uploaded to the server being re-downloaded and sometimes being seen as an externally modified file.

in sample vault in Tasks

• Above occurs when in vanilla vault, and no sync

Workaround

• Hit Save (Cmd+S) after deleting lines, and before toggling the line

This forces Obsidian to quickly re-parse the file, and usually then the task index is correct by the time you manually toggle the last task.

---

FWIW, attached is the content of the Markdown file I was experimenting with when I found the above repro: sdfafa.md

[claremacrae]

My current thinking is approximately something like this:

The problem:

• Obsidian has a 2-second cycle for auto-saving edits
• The current retry mechanism in replaceTaskWithTasks() attempts to handle cases where the markdown file has not yet been parsed
• But I think it does not behave well when the file was previously parsed but has since been modified
• Types of edits to the file might include:
  • Pasting in before the task a large chunk of markdown that may add extra sections or extra tasks
  • Deleting multiple sections
  • Deleting tasks before the task being toggled
• When the file has since been modified, all bets are off as to whether a pre-existing task's position information in the file is valid at all
• Does the anti-corruption fix cause replaceTaskWithTasks() to bail out after the first attempt - or the first attempt that reaches that far?

Potential band-aids:

I am struggling to express this clearly, but I think the ultimate fix for this is going to be something like:

• If we failed to find the exactly-matching task line based on section number and task number in section inside the file after 10 attempts...
  • Then read each of the tasks in the given section index, and if there is an exactly matching mark-down source line, replace that
• If we still haven't found an exactly matching mark-down source line,
  • Then read the whole file and repeat the search
• If we still haven't found an exactly matching mark-down source line after searching the whole file,
  • Write out the error message that is currently written out on first failure to match

[claremacrae]

Whatever we do, it's gonna need some pretty thought thorough unit/automated tests...

[claremacrae]

Or does vault.process() make all my ramblings above go away? 🤣

[claremacrae]

Reproduction 2 - change level of indentation

• Setup
  • Left pane: Source mode
  • Right page: Reading mode
  • Sync enabled
• Indent a task line (in left pane)
• Then quickly complete it (in right pane)

With original guard code:

Tasks: Unable to find task in file .../sdfafa.md.
Expected task:     - [ ] #task  XXX Throw the trash away 3 🔁 every day 📅 2023-02-23. Found task line: - [ ] #task  XXX Throw the trash away 3 🔁 every day 📅 2023-02-23.

With lightly-reformatted error message in not-yet-pushed code - still using Task.identicalTo():

Tasks: Unable to find task in file:
.../sdfafa.md
Expected task:
    - [ ] #task  XXX Throw the trash away 3 🔁 every day 📅 2023-02-23
Found task line:
- [ ] #task  XXX Throw the trash away 3 🔁 every day 📅 2023-02-23

[claremacrae]

Note to self: Try these same reproduction steps in dataview

[claremacrae]

Reproduction 3 - edit the line then complete it

• Setup
  • Left pane: Source mode
  • Right page: Reading mode
  • Sync enabled
• edit some text in a task line (in left pane)
• Then quickly complete it (in right pane)

Output:

Tasks: Unable to find task in file:
.../sdfafa.md
Expected task:
- [ ] #task  XXX Throw the tradsfsdfsdfsfdssh away 3 🔁 every day 📅 2023-02-23
Found task line:
- [ ] #task  XXX Throw the trash away 3 🔁 every day 📅 2023-02-23

[claremacrae]

Thoughts - 2023-02-18 - 1

The weird thing with all of these is that:

• when I edit a task line in Source mode in one pane
  • the task is immediately updated in the matching Reading pane
• But if immediately toggle the task by clicking its checkbox in the Reading pane
  • I get the task mismatch error
• But if, between the edit and the toggle, I Save the file manually (Cmd+S/Ctrl+S) the file is re-parsed in time for the Task to correctly be toggled

Also:

• when I edit a task line in Source mode in one pane
• and I immediately toggle it with Tasks' 'Toggle task done' command in the Source pane
  • The task is updated correctly

Observations when testing on Desktop

So for me in this testing, the problem seems to fundamentally be the 'up to 2 seconds' between:

• the task line being edited in the Source pane
• the corresponding Task object being updated in the Reading pane up to 2 seconds later

When I usually see this in the wild - usually on Mobile

When I have generally had the problem for real, it is:

• when I am using Tasks on my iPhone
• and the synced vault is not open on any other machine
• and I am in Reading mode, with no other panes open
• and I toggle a few tasks relatively quickly
• then, rarely, the wrong task is completed

[claremacrae]

Thoughts - 2023-02-18 - 2

• When I edit a file in Live Preview
• And try to catch it out with quick actions
• It all works correctly

So the problem scenarios seem to be:

• Editing a task in one pane, and interacting with it in a different Reading view before the 2-second save-and-reparse happens
• Acting on a synced file in Reading view several times in quick succession, before the round-trip sync-and-re-parse occurs

[BluBloos]

Hi clare, I am able to repro these same cases!

[claremacrae]

Hi clare, I am able to repro these same cases!

Thanks for testing. @BluBloos.

This has been bugging me for ages - then your message made the issue visible and I have made so much progress in understanding the various things going on since then - at the risk of repeating myself, very many thanks indeed!

[BluBloos]

pushed a PR that fixes this :)

(I've adjusted the name also from "Data integrity 2" - that was a mistake)

[claremacrae]

This was fixed in #1663 - thanks to BluBloos.