Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adopt atomic write for user data #180695

Closed
Maheraj1 opened this issue Apr 24, 2023 · 15 comments · Fixed by #192141
Closed

Adopt atomic write for user data #180695

Maheraj1 opened this issue Apr 24, 2023 · 15 comments · Fixed by #192141
Assignees
@bpasero @sandy081
Labels
author-verification-requested Issues potentially verifiable by issue author debt Code quality issues file-io File I/O insiders-released Patch has been released in VS Code Insiders user-profiles User profile management
Milestone
September 2023

Comments

@Maheraj1
Copy link

Does this issue occur when all extensions are disabled?: Yes

I was working on one of my projects when there was a power outage, I noticed that my profiles were gone. I recreated them and again a few days ago this happened again. This happened 2 times to me within a period of 2 weeks.

  • VS Code Version: 1.77.3
  • OS Version: MacOS 13.3.1

Steps to Reproduce:

  1. Open a folder with a non-default profile
  2. Remove the plug of your machine
  3. Bootup your machine
  4. Open vs code
  5. The profile doesn't exist
@jrieken jrieken assigned sandy081 and unassigned jrieken Apr 24, 2023
@sandy081
Copy link
Member

@Maheraj1 When this happens can you please check if the profiles folders do exist in the following location?

  • Run command Developer: Open User Data Folder in VS Code
  • Navigate to User / profiles folder

Can you please share storage.json file in User / globalStorage folder

@bpasero I store profiles information in the storage file. It could happen that this file is truncated?

@sandy081 sandy081 added info-needed Issue requires more information from poster user-profiles User profile management labels May 10, 2023
@bpasero
Copy link
Member

bpasero commented May 10, 2023 •
edited

@sandy081 yes, this call (and any other of these) can lead to a corrupt state file if for example the process terminates after the truncate but before the write:

vscode/src/vs/platform/state/node/stateService.ts

Lines 140 to 146 in 66e8ef9

// Write to disk
try {
await this.fileService.writeFile(this.storagePath, VSBuffer.fromString(serializedDatabase));
this.lastSavedStorageContents = serializedDatabase;
} catch (error) {
this.logService.error(error);
}

For a more robust state location, we have the SQLite backend.

@sandy081
Copy link
Member

Unfortunately, I cannot use the StorageService because, profiles are initialised before storage service is ready and storage service is not available in CLI and Server.

@bpasero
Copy link
Member

bpasero commented May 11, 2023

I think the state service should write to a file in the same folder (storage.json.backup) and then rename to the target to reduce the chance of this to happen.

@bpasero
Copy link
Member

bpasero commented May 11, 2023

Do we have confirmation this happened here? Because I still see the "info-needed" label.

bpasero added a commit that referenced this issue May 12, 2023
@bpasero
files - add new option atomic for writeFile and adopt for state (fix
5539513 
 #180695)
@VSCodeTriageBot
Copy link
Collaborator

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.

Happy Coding!

@VSCodeTriageBot VSCodeTriageBot closed this as not planned Won't fix, can't repro, duplicate, stale May 19, 2023
@sandy081
Copy link
Member

@bpasero Reopening this and I think the fix you are working on is actually helpful that prevents loosing user data.

@sandy081 sandy081 reopened this May 23, 2023
@sandy081 sandy081 added bug Issue identified by VS Code Team member as probable bug debt Code quality issues and removed info-needed Issue requires more information from poster debt Code quality issues labels May 23, 2023
@sandy081 sandy081 added this to the June 2023 milestone May 23, 2023
@sandy081 sandy081 added the feature-request Request for new features or functionality label May 23, 2023
@sandy081 sandy081 changed the title Profiles auto deleted after power outage Support atomic write and adopt it for user data May 23, 2023
@bpasero bpasero added file-io File I/O and removed bug Issue identified by VS Code Team member as probable bug labels May 23, 2023
@bpasero bpasero changed the title Support atomic write and adopt it for user data Adopt atomic write for user data May 23, 2023
@bpasero bpasero removed their assignment May 23, 2023
@bpasero
Copy link
Member

bpasero commented May 23, 2023

I have #182974 for the providing part.

@sandy081 sandy081 mentioned this issue Jul 5, 2023
Merged
sandy081 added a commit that referenced this issue Jul 5, 2023
@sandy081
#180695 - adopt atomic write for userdata
1198bd4
@sandy081 sandy081 mentioned this issue Jul 5, 2023
Merged
sandy081 added a commit that referenced this issue Jul 5, 2023
@sandy081
#180695 adopt atomic delete for extensions
a4fcb9e
sandy081 added a commit that referenced this issue Jul 5, 2023
@sandy081
adopt atomic delete for extensions (#187054)
e195ed8 
#180695 adopt atomic delete for extensions
@sandy081 sandy081 removed the feature-request Request for new features or functionality label Jul 5, 2023
sandy081 added a commit that referenced this issue Jul 5, 2023
@sandy081
adopt atomic write for userdata (#187038)
1157e96 
* #180695 - adopt atomic write for userdata

* check if fsp has atomic write

* fix tests
@sandy081 sandy081 closed this as completed Jul 6, 2023
@joyceerhl joyceerhl mentioned this issue Jul 10, 2023
Closed
@sandy081 sandy081 reopened this Jul 11, 2023
@sandy081
Copy link
Member

Reopened because of #187245

Atomic write in windows is failing sometimes and I think this is because some other processes (defender 🤔 ) might be holding the file that has to be renamed.

I have seen similar issues in extensions when I implemented atomic installation of extensions long time back, ie. extracting the zip to a temp file in the same folder and renaming it. I have to retry renaming multiple times to avoid this.

Check this issue - nodejs/node#29481

This is common in windows and it seems the workaround is to retry.

@sandy081 sandy081 modified the milestones: July 2023, August 2023 Jul 11, 2023
@bpasero
Copy link
Member

bpasero commented Jul 21, 2023

This is a good catch and I had to learn that graceful-fs does not retry rename calls if the target already exists:

https://github.com/isaacs/node-graceful-fs/blob/234379906b7d2f4c9cfeb412d2516f42b0fb4953/polyfills.js#L108-L111

I think this was a decision sometime in the project to not wait for up to 60 seconds for an unrecoverable error to bubble up.

But in our case with explicit atomic write support, we need to implement this retry logic from our usage I think.

@bpasero bpasero mentioned this issue Jul 26, 2023
Merged
@bpasero
Copy link
Member

bpasero commented Aug 23, 2023

Fyi #188899 landed and will retry rename even if a file at destination already exists.

@sandy081 sandy081 modified the milestones: August 2023, September 2023 Aug 23, 2023
@sandy081
Copy link
Member

I will adopt this in debt week of September.

@sandy081 sandy081 mentioned this issue Aug 28, 2023
Closed
@sandy081 sandy081 mentioned this issue Sep 4, 2023
Merged
sandy081 added a commit that referenced this issue Sep 4, 2023
@sandy081
fix #180695
2a4e8f7
sandy081 added a commit that referenced this issue Sep 4, 2023
@sandy081
fix #180695 (#192141)
83c8a64
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Sep 4, 2023
@bpasero bpasero added the author-verification-requested Issues potentially verifiable by issue author label Sep 8, 2023
@VSCodeTriageBot
Copy link
Collaborator

This bug has been fixed in the latest release of VS Code Insiders!

@Maheraj1, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version e073d67 of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bpasero bpasero

@sandy081 sandy081

Labels
author-verification-requested Issues potentially verifiable by issue author debt Code quality issues file-io File I/O insiders-released Patch has been released in VS Code Insiders user-profiles User profile management
Projects
None yet
Milestone
September 2023
Development

Successfully merging a pull request may close this issue.

fix #180695 microsoft/vscode
5 participants
@bpasero @jrieken @sandy081 @Maheraj1 @VSCodeTriageBot