Data loss reported - Githubissues

lishid commented 3 years ago

https://forum.obsidian.md/t/data-loss-0-11-3-zero-byte-md-file-in-vault/13538/10

This is caused by the code at: https://github.com/delashum/obsidian-checklist-plugin/blob/9ddf1f41dd64b4569c3a759567fef59befde0841/src/_utils.ts#L236

cachedData is undefined when the file has not been read this session. Don't use private APIs like this.

Recommended fix: Use vault.read if you're going to be making changes and writing it back to the file.

liamcain commented 3 years ago

If you're writing back to disk, you should always use vault.read to avoid data loss.

lishid commented 3 years ago

I have temporarily delisted this plugin and issued a deprecation flag for all available versions.

Please let me know when it's resolved so we can add it back.

delashum commented 3 years ago

I'm really sorry to hear that happened. Looking into a fix right now.

What is the difference between vault.read and vault.cachedRead?

delashum commented 3 years ago

My understanding is that the main issue here was using (file as any).cachedData as a source of truth for what was in the file as it can come back as undefined if that file isn't loaded into the cache. I am now loading all files using vault.cachedRead, and I use the results from that to write updates. Is that sufficient? Or do I need to use vault.read instead?

lishid commented 3 years ago

Always use vault.read if you intend to write the file back to disk. vault.cachedRead may return stale data, for example, if the user modified the file on disk, but it's a network drive so we didn't get any notification that the file has changed.

delashum commented 3 years ago

Do those two commits seem sufficient? Tested locally and works fine

lishid commented 3 years ago

Let me do a quick review here.

https://github.com/delashum/obsidian-checklist-plugin/blob/a247a5cd300e71a82a3ca3716d495cb2d0cfadb5/src/_utils.ts#L73 Here you should probably use the proper API to get the file - there's one for getting the abstract file by path, just need to check if the result is instanceof TFile.

Actually, why aren't you reading the file contents when toggling the item? Seems like you're working off a full copy of the file contents that you've saved sometimes earlier (which may be stale).

I recommend reading the file right before writing it, and processing that version. If you think the file may/will have changed between then two, then depending on what you'd want to do, you can either work off the new version, or maybe just don't touch the file.

You've set your plugin to isDesktopOnly = false, but you're using the NodeJS os package: https://github.com/delashum/obsidian-checklist-plugin/blob/a247a5cd300e71a82a3ca3716d495cb2d0cfadb5/src/_utils.ts#L1 Should probably use a user-agent test for isMacOS instead.

https://github.com/delashum/obsidian-checklist-plugin/blob/9ddf1f41dd64b4569c3a759567fef59befde0841/src/svelte/TextChunk.svelte#L9 Don't use this. (You're using it more than once) It's not exposed for a good reason. Pass it in from your plugin/view. You can access it when you inherit ItemView using this.app.

More of a long term/bigger improvement down the line: https://github.com/delashum/obsidian-checklist-plugin/blob/a247a5cd300e71a82a3ca3716d495cb2d0cfadb5/src/_utils.ts#L18 Consider avoiding re-parse every single file on every rerender? Our API can give specific file changes and you could use to incrementally update your parsed files (delete/rename from vault, new/change from metadataCache).

lishid commented 3 years ago

So the specific case of data loss I believe is caused by a race-condition that you aren't testing for generally: when a file is changed locally, its cachedData is wiped when Obsidian receives the OS notification. Between then and the metadata cache updates with the new file contents, you'll be left with a stale copy of the old data. Before your patch, this will wipe the file as cachedData was emptied.

liamcain commented 3 years ago

Also, just to add on: when you're parsing the tasks to display them in the view, you're better off keeping it as vault.cachedRead since it will be notably faster. So I don't think you want the change in a247a5c.

BUT as @lishid mentioned, treat that fileInfo.content as stale and re-read the particular file when toggling the task.

delashum commented 3 years ago

The reason I decided to not re-read the file before writing is that I am tracking each checklist-item by what line it's on. And if I read a file right before writing and the todo is now on a different line than what's in my cache the update won't be possible. I will think about another way to approach this though. There is definitely a race condition since there is a delay between updates and the this.app.metadataCache.on("resolve",...) firing.

Since there isn't really any documentation I have little insight into what the different between TFile and TAbstractFile. I am only using app.vault.getFiles().find((f) => f.path === item.filePath) because app.vaule.getAbstractFile() didn't seem to provide what I was looking for. I would definitely love to hear how I can get a specific file though.

I am curious what's wrong with (window as any).app. Seems like it's pointing to the exact same object as this.app coming from the View. Btw, I got my inspiration to pull app from window from your repo @liamcain obsidian-calendar-plugin.

Good suggestion on using user-agent. I'll get that fixed

To your last suggestion, I would love to only re-parse updated files! It is very unclear to me due to the lack of documentation on how to listen for those sort of changes, so i opted to just re-parse everything each time.

lishid commented 3 years ago

The reason I decided to not re-read the file before writing is that I am tracking each checklist-item by what line it's on. And if I read a file right before writing and the todo is now on a different line than what's in my cache the update won't be possible. I will think about another way to approach this though. There is definitely a race condition since there is a delay between updates and the this.app.metadataCache.on("resolve",...) firing.

Yeah I think for this, you should probably compare the read file with the one you have cached, and bail with a new Notice saying the underlying file has changed, try again in a few seconds. If you have a way to know what the user did and map it to the new file (probably not for your case I'm guessing?) you can attempt to still perform the action on the new file contents.

Since there isn't really any documentation I have little insight into what the different between TFile and TAbstractFile. I am only using app.vault.getFiles().find((f) => f.path === item.filePath) because app.vaule.getAbstractFile() didn't seem to provide what I was looking for. I would definitely love to hear how I can get a specific file though.

Do this:

let file = vault.getAbstractFileByPath(path);
if (file instanceof TFile) {
  //... your code here ...
}

I am curious what's wrong with (window as any).app . Seems like it's pointing to the exact same object as this.app coming from the View. Btw, I got my inspiration to pull app from window from your repo @liamcain obsidian-calendar-plugin.

😠 @liamcain

window.app is used as a debugging pointer for testing things from console. It may be renamed or removed in the future.

To your last suggestion, I would love to only re-parse updated files! It is very unclear to me due to the lack of documentation on how to listen for those sort of changes, so i opted to just re-parse everything each time.

The ones we're internally using for incrementally generating search results and backlinks, is something like the following:

vault.on('delete') => remove the file vault.on('rename') => move the results for the old file to the new file, knowing the contents probably didn't change metadataCache.on('changed') (or 'resolve' if you need the cache) => parse/update the file. This replaces the new and changed triggers from vault.

delashum commented 3 years ago

Awesome, thanks for the helpful responses. This gives me a lot more information to work with. I apologize again for losing one of your users data, that sucks.

I should be able to find some time this week to make all these changes.

Also, I would be happy to help write some documentation on the Plug-in API if that's something you are interested in happening. There are quite a few things that were really unclear to me that I think could help prevent this sort of thing going forward.

lishid commented 3 years ago

No worries! Awesome work and appreciate the quick response. In the end it's not your fault, we should have more actively pushed our users to backup their data. Which we will!

Liam is working on some FAQ on the docs, which might be of interest to you. Would be great if you can participate once you have more time.