Appdata lockfile #1319

fredrikekelund · 2025-05-05T09:00:52Z

Related issues

Related to pesfPa-1tY-p2 and p1746194187596159-slack-C04GESRBWKW
Fixes https://linear.app/a8c/issue/STU-500

Proposed Changes

Important

I'm currently seeking feedback on the implementation here. I'm aware that the tests are failing, but I don't want to work on fixing those before we decide on a direction for the implementation. The actual CLI and app code should work normally.
Please remember to view the diff with whitespace changes hidden. See this comment for details.

This PR implements a mandatory lockfile mechanism that's used when writing the Studio config file (appdata-v1.json) from the CLI or app code. It fixes race conditions that can lead to data loss, especially when the CLI and the app interact.

This PR evolved from a simpler concept of using function arguments to loadUserData and saveUserData to acquire and release the lock, to a slightly larger refactor that makes acquiring the lock mandatory to write the config file. The current solution is a bigger change, but it's also safer.

This PR now adds a higher-order function (withUserDataWrite) that wraps functions wanting to write to the Studio config file. withUserDataWrite does a couple of things:

Acquires a lock
Injects a parsed instance of userData as the first argument to the callback function
Writes the return value of the callback to the Studio config file
Releases the lock

In the CLI code, we use a slightly more advanced higher-order function that supports the callbacks returning tuples (arrays with a fixed numbers of members). The first member is a userData object that gets written to the Studio config file, the second member is returned to the function caller. This way, we get added flexibility.

Testing Instructions

TBD

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

fredrikekelund · 2025-05-05T09:07:44Z

src/ipc-handlers.ts

@@ -153,7 +153,7 @@ export async function createSite(
 	enableHttps?: boolean,
 	siteId?: string
 ): Promise< SiteDetails[] > {
-	const userData = await loadUserData();
+	const userData = await loadUserData( true );


The createSite function can probably take more than 1000ms to execute, so maybe we need to hold the lockfile for longer in this case.

bcotrim

I've run some smoke tests on the app and everything seems to be working correctly.

I would suggest changing the current approach of passing a lock flag to something more like the withLock function. I think that approach would help guarantee a few things:

Writes are always performed with the lock held
Error handling is more robust, ensuring the file is always unlocked on exceptions or when the process completes

This could help reduce the risk of accidental misuse and make the locking logic easier to maintain.

For example:

function withAppdataLock(fn) {
  return async (...args) => {
    await lock(LOCKFILE_PATH, { wait: 1000, stale: 1000 });
    try {
      const data = await readAppdata();
      const updated = await fn(data, ...args);
      await saveAppdata(updated);
      return updated;
    } finally {
      await unlock(LOCKFILE_PATH);
    }
  };
}

What do you think about this approach?

fredrikekelund · 2025-05-14T11:21:30Z

I would suggest changing the current approach of passing a lock flag to something more like the withLock function

I like your idea, @bcotrim, but I'm not convinced it's flexible enough for our existing codebase.

The appealing aspects with your suggestion, as I see it, are:

It guarantees that locks are held and released in withAppdataLock functions
It looks clean.

I've refactored ipc-handlers.ts to use that approach. For some functions, the difference from before is negligible. For other functions that previously both updated the Studio config file and returned a different value, I had to come up with workarounds. In several cases, I think the end result was better than before (the deleteSite function returning an array of all remaining sites is perhaps not the best approach, IMO).

Still, there is one remaining case in src where the withAppdataLock approach would be more cumbersome:

studio/src/lib/windows-helpers.ts

Lines 38 to 64 in 4d0cf79

    
           switch ( response ) { 
        
           	case buttons.indexOf( AUTOMATIC_UPDATE ): 
        
           		// Update Windows Defender configuration 
        
           		await saveUserData( { 
        
           			...userData, 
        
           			promptWindowsSpeedUpResult: 'yes', 
        
           		} ); 
        
           		try { 
        
           			await excludeProcessInWindowsDefender(); 
        
           		} catch ( _error ) { 
        
           			const mainWindow = await getMainWindow(); 
        
           			await dialog.showMessageBox( mainWindow, { 
        
           				type: 'error', 
        
           				title: __( 'Something went wrong' ), 
        
           				message: __( 
        
           					'The configuration couldn\'t be changed to speed up site creation.\n\nTo initiate this process again, please go to "Help > How can I make Studio faster?" in the application menu.' 
        
           				), 
        
           			} ); 
        
           		} 
        
           		break; 
        
           	case buttons.indexOf( NOT_INTERESTED ): 
        
           		// Skip it, user is not interested 
        
           		await saveUserData( { 
        
           			...userData, 
        
           			promptWindowsSpeedUpResult: 'no', 
        
           		} ); 
        
           		break;

For the withAppdataLock approach to be really effective, we shouldn't have to export the saveUserData function from src/storage/user-data.ts, and I've yet to think of a practical way to refactor src/lib/windows-helpers.ts to accommodate this approach.

More importantly, the CLI code also has several functions that update the Studio config file and return a different value. Here, for example:

studio/cli/lib/snapshots.ts

Lines 39 to 55 in 4d0cf79

    
           export async function updateSnapshotInAppdata( 
        
           	atomicSiteId: number, 
        
           	siteFolder: string 
        
           ): Promise< Snapshot > { 
        
           	const site = await getOrCreateSiteByFolder( siteFolder ); 
        
           	const userData = await readAppdata( true ); 
        
           	const snapshot = userData.snapshots.find( ( s ) => s.atomicSiteId === atomicSiteId ); 
        
           	if ( ! snapshot ) { 
        
           		throw new LoggerError( __( 'Failed to find existing preview site in appdata' ) ); 
        
           	} 
        
           	snapshot.localSiteId = site.id; 
        
           	snapshot.date = Date.now(); 
        
           	await saveAppdata( userData, true ); 
        
           	return snapshot; 
        
           }

I don't think the updateSnapshotInAppdata function is really calling for a refactor for any other reason, and following the withAppdataLock approach introduces relatively cumbersome hoops that we need to jump through.

bcotrim · 2025-05-14T20:42:34Z

src/storage/user-data.ts

+
+type MaybePromise< T > = T | Promise< T >;
+
+export function withAppdataLock< Args extends unknown[] >(


Could we do something like this

export function withAppdataLock< Args extends unknown[], ReturnType = void >( fn: ( userData: UserData, ...args: Args ) => MaybePromise< [ UserData, ReturnType ] >

This would allow for the flexibility we need, while keeping everything clean.
What do you think?

fredrikekelund · 2025-05-15T12:22:57Z

I added an updateAppdata function to src/storage/user-data.ts to solve the problem in src/lib/windows-helpers.ts. That function writes only primitive values with a low chance of conflicts and can only be used to write known values (i.e. not values that are derived from the existing ones in the config file, like modifications to the sites array, for example.)

For the CLI code, I first took a shot at a generator-based approach where we yield updates to the config file. That felt overly complex, though, so I ended up implementing your suggestion, @bcotrim. withAppdataWrite callbacks can now return tuples, where the second value (if there is one) is passed back to the caller. I was hesitant at first, thinking this would not be easy to understand, but I've since come around, and I think it's OK now.

I haven't touched the tests, so it's expected that they are still failing. The CLI and the app should (hopefully) be fully functional, though.

I'd appreciate another round of feedback, @bcotrim!

fredrikekelund · 2025-05-15T12:58:17Z

Tagging you for feedback, @nightnei, since Bernardo's AFK

fredrikekelund · 2025-05-16T08:47:18Z

If the tuple pattern feels weird, we can also make the callbacks return a schema like so:

{
	userData?: UserData;
	return?: unknown;
}

fredrikekelund · 2025-05-19T09:14:09Z

src/hooks/use-site-details.tsx

@@ -227,7 +224,7 @@ export function SiteDetailsProvider( { children }: SiteDetailsProviderProps ) {
 					customDomain,
 					enableHttps
 				);
-				const newSite = data.find( ( site ) => site.path === path );
+				const newSite = data.sites.find( ( site ) => site.path === path );


Not 100% happy about the createSite function returning the entire user config object… It might be worthwhile to update withUserDataWrite function to also support returning values to the caller (the same way that withAppdataWrite works).

Let's change the createSite IPC function to only return the newly created site.

nightnei

I was reviewing, and more and more I was wondering - would it be simpler if we don't introduce HOF and make fewer changes with just modifying saveUserData? Since now code looks much more complicated, especially with nuances like updateAppdata

And yeah, tuple pattern looks weird, I think idea with object is great 👍

nightnei · 2025-05-19T14:26:03Z

cli/lib/utils.ts

 export function normalizeHostname( hostname: string ): string {
 	return hostname
 		.trim()
 		.toLowerCase()
 		.replace( /^https?:\/\//, '' )
 		.replace( /\/$/, '' );
 }
+
+export function lock( path: string, options: lockfile.Options ) {


lock looks very general. what about lockFileAsync & unlockFileAsync? adding Async mostly due to distinguish between library name lockfile and court custom functions in codebase.

nightnei · 2025-05-19T14:31:23Z

cli/lib/appdata.ts

@@ -111,6 +112,40 @@ export async function saveAppdata( userData: UserData ): Promise< void > {
 	}
 }

+const LOCKFILE_PATH = path.join( getAppdataDirectory(), 'appdata-v1.lock' );


NIT but I would go with appdata-v1.json.lock, IMO it looks more clear that it lock specifically for appdata-v1.json.

Suggested change

const LOCKFILE_PATH = path.join( getAppdataDirectory(), 'appdata-v1.lock' );

const LOCKFILE_PATH = path.join( getAppdataDirectory(), 'appdata-v1.json.lock' );

fredrikekelund · 2025-05-20T06:53:30Z

I was reviewing, and more and more I was wondering - would it be simpler if we don't introduce HOF and make fewer changes with just modifying saveUserData?

It would indeed be simpler, but it would also be less secure, because there's no guarantee that we obtain a look before writing the config file.

This is the solution I explored originally, though. If you want to see what that looked like, you can go to https://github.com/Automattic/studio/pull/1319/files/559b01c9717d46ea236666bec13583e2b7865ae8

nightnei · 2025-05-20T10:02:14Z

I reviewed the old approach and IMO it was cool. Of course, providing true everywhere looked really not nice and just duplicating. But I think that previous solution could be improved with moving loadUserData inside saveUserData and additionally we could pass callback as an argument to saveUserData and we would kill two birds with one stone. Actually, it's just mixing the latest changes in this PR with those changes before HOF:

~~const data = await loadUserData( true );~~
~~data.devToolsOpen = false;~~
~~await saveUserData( data, true );~~

await saveUserData( currentData => ({
	 ... currentData,
	 devToolsOpen: false
 }));
 
 const saveUserData = (cb) => {
 	lock;
 	data = read;
 	cb(data);
 	unlock;
 }

As a result:

We don't do big refactoring
YOLO members are already used to the current behaviour of saveUserData, so the behaviour literally is not changed
Code is easier to read
And last but not least - we are considering moving to real DB or something like that in the future, so in the future I believe that it will be easier to move to DB with this approach, since we will just adjust encapsulated saveUserData with replacing file lock mechanism to DB locking

HOF is really nice idea, but IMO not in this case, it looks overcomplicated.

WDYT?

bcotrim · 2025-05-23T11:49:12Z

The thought of using an ESLint rule struck me, too. Your screenshot looks promising, @bcotrim. Thinking out loud: if we choose a linter-based approach, the rules should go like this:

saveUserData and updateAppdata can only be called if:
1. We update with data that's not derived from the existing file (i.e., we don't modify the `sites` array, for example), OR

2. We need to have acquired a lock first
This would indeed lead to simpler code, even if I'm unsure how difficult it would be to encode those exact rules in ESLint. @bcotrim, could you share the code behind your experiment? Maybe branch off this PR.

I've cleaned the code a bit and adjusted the condition to the behavior you mentioned.
#1430

fredrikekelund · 2025-05-26T13:12:15Z

I worked on fixing the tests for this PR, but realized that there's a fundamental challenge in the proposed architecture: if a withAppdataWrite function calls another withAppdataWrite function, the lock mechanism does not work as intended. In these cases, the second callback waits until the lock of the first callback has been released, and the first callback unlocks the lock after completing, despite no longer holding the lock (in the worst case, another function could already have acquired the lock).

I propose taking a step back and going with the simpler architecture that I originally suggested (see https://github.com/Automattic/studio/pull/1319/files/559b01c9717d46ea236666bec13583e2b7865ae8). This would be a pragmatic way of fixing the issue at hand without spending too much more time on this.

We can potentially improve that solution later by adding an ESLint rule. I think manual code reviews would go a long way, though.

nightnei · 2025-05-26T19:07:24Z

I propose taking a step back and going with the simpler architecture that I originally suggested (see Automattic/studio/pull/1319/files/559b01c9717d46ea236666bec13583e2b7865ae8). This would be a pragmatic way of fixing the issue at hand without spending too much more time on this.

Let's do it.
And taking into account that it's a temporary solution (AFAIU), then I think we should not worry a lot about making this solution perfect. Just keep it simple and clear.

bcotrim · 2025-05-27T12:40:45Z

@nightnei @fredrikekelund This PR should be ready for review. As we discussed I changed this back to a simpler approach without the HOF.
I would still recommend reviewing/merging this one first #1430 as the rule really makes it complete, to enforce the lock/unlock mechanism.

Let me know if you have any comments or feedback.

nightnei · 2025-05-27T13:17:43Z

This PR should be ready for review. As we discussed I changed this back to a simpler approach without the HOF.

@bcotrim The current code LGTM

I would still recommend reviewing/merging this one first #1430 as the rule really makes it complete, to enforce the lock/unlock mechanism.

Let me try to update the code with my suggestion above. If it works well, then we don't need ESLint rule and code will be even more simple and w/o duplications of calling loadUserData

nightnei · 2025-05-27T14:31:13Z

Let me try to update the code with my #1319 (comment). If it works well, then we don't need ESLint rule and code will be even more simple and w/o duplications of calling loadUserData

So, the idea is this - to avoid copy-paste, avoid the eslint rule and make a reliable solution. I see that updating it in some places will not be very easy, so we can try to proceed with changes in my PR and update all calls of saveUserData or we can proceed with current changes in this PR as the first step and then we will see - should we add eslint rule or maybe we will soon consider using DB or something else.

~~Folks, what are your thoughts about it?~~

UPDATE: I am ok with the reverted latest changes in this PR 👍
Regarding ESLint rule - it also makes sense to make these changes complete, so if Fredrik also ok with it, then let's merge, do final review/test and let's merge it.

fredrikekelund

This is looking really good overall 👍 I feel confident that this is the right path forward.

I left some comments with varying degrees of importance. On a more birds-eye-view note, I also recommend adding something like this to cli/index.ts:

process.on( 'uncaughtExceptionMonitor', async () => {
	await unlockAppdata();
} );

We should do the same for the Studio app and add an equivalent handler in src/index.ts.

fredrikekelund · 2025-05-28T07:55:09Z

cli/lib/appdata.ts

@@ -163,20 +173,29 @@ export function getNewSitePartial( siteFolder: string ): z.infer< typeof siteSch
 	return newSite;
 }

-export async function getOrCreateSiteByFolder(
+const createNewSite = async ( siteFolder: string ): Promise< z.infer< typeof siteSchema > > => {


Suggested change

const createNewSite = async ( siteFolder: string ): Promise< z.infer< typeof siteSchema > > => {

const createNewSite = async ( siteFolder: string ): Promise< SiteData > => {

Nit, but there's a dedicated type for this now.

fredrikekelund · 2025-05-28T07:55:19Z

cli/lib/appdata.ts

 	siteFolder: string
-): Promise< z.infer< typeof siteSchema > > {
-	let site;
+): Promise< z.infer< typeof siteSchema > > => {


Suggested change

): Promise< z.infer< typeof siteSchema > > => {

): Promise< SiteData > => {

fredrikekelund · 2025-05-28T07:55:44Z

cli/lib/appdata.ts

+		const site = await getSiteByFolder( siteFolder );
+		return site;


Suggested change

const site = await getSiteByFolder( siteFolder );

return site;

return await getSiteByFolder( siteFolder );

Nit, suggestion

fredrikekelund · 2025-05-28T08:02:51Z

cli/lib/tests/appdata.test.ts

-	describe( 'saveAppdata', () => {
-		it( 'should save the userData to the appdata file', async () => {
-			const mockUserData = {
-				version: 1,
-				newSites: [],
-				sites: [],
-				snapshots: [],
-			};
-
-			await saveAppdata( mockUserData );
-
-			expect( writeFile ).toHaveBeenCalledWith(
-				expect.any( String ),
-				JSON.stringify( mockUserData, null, 2 ) + '\n',
-				{ encoding: 'utf8' }
-			);
-		} );
-
-		it( 'should throw LoggerError if there is an error saving the file', async () => {
-			const mockUserData = {
-				version: 1,
-				newSites: [],
-				sites: [],
-				snapshots: [],
-			};
-
-			( writeFile as jest.Mock ).mockRejectedValue( new Error( 'Write error' ) );
-
-			await expect( saveAppdata( mockUserData ) ).rejects.toThrow(
-				'Failed to save Studio config file'
-			);
-		} );
-
-		it( 'should add version 1 if version is not provided', async () => {
-			const mockUserData = {
-				newSites: [],
-				sites: [],
-				snapshots: [],
-			};
-
-			await saveAppdata( mockUserData );
-
-			expect( writeFile ).toHaveBeenCalled();
-			const savedData = JSON.parse( ( writeFile as jest.Mock ).mock.calls[ 0 ][ 1 ] );
-			expect( savedData.version ).toBe( 1 );
-		} );
-	} );
-


I believe we could bring back these tests, no? I removed them when we stopped exporting saveAppdata

fredrikekelund · 2025-05-28T08:03:28Z

common/lib/lockfile.ts

@@ -0,0 +1,25 @@
+import lockfile from 'lockfile';
+
+export function lockFileAsync( LOCKFILE_PATH: string, options: lockfile.Options ) {


Suggested change

export function lockFileAsync( LOCKFILE_PATH: string, options: lockfile.Options ) {

export function lockFileAsync( lockfilePath: string, options: lockfile.Options ) {

Let's not capitalize this, since this is no longer a constant.

fredrikekelund · 2025-05-28T08:12:52Z

src/ipc-handlers.ts

+			Sentry.captureException( error );
+		}
+		const newSites = userData.sites.filter( ( site ) => site.id !== id );
+		return { ...userData, sites: newSites };


Suggested change

return { ...userData, sites: newSites };

We call getIpcApi().getSiteDetails() in the renderer process now to query the sites after this function completes, so let's remove the return value.

fredrikekelund · 2025-05-28T08:16:05Z

src/ipc-handlers.ts

 export async function handleNewSite( event: IpcMainInvokeEvent, newSite: NewSiteDetails ) {
-	await createSite( event, newSite.path, undefined, undefined, undefined, undefined, newSite.id );
+	try {
+		await lockAppdata();
+		const userData = await loadUserData();
+		const updatedUserData = await createSite(
+			event,
+			newSite.path,
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			newSite.id
+		);

-	const userData = await loadUserData();
-	await saveUserData( {
-		...userData,
-		newSites: userData.newSites?.filter( ( s ) => s.id !== newSite.id ),
-	} );
+		const newSites = userData.newSites?.filter( ( s ) => s.id !== newSite.id );
+		await saveUserData( { ...updatedUserData, newSites } );
+	} finally {
+		await unlockAppdata();
+	}
 }


There's a logic problem here similar to what I described on Monday #1319 (comment):

We acquire a lock

We call createSite

createSite acquires a lock (it waits for the existing lock to go stale after 1000ms)

createSite returns

We update the Studio config file without a lock

We release the lock (despite no longer actually holding it)

We could fix this by acquiring the lock after createSite returns, and then calling loadUserData again to get a fresh copy of the config file.

fredrikekelund · 2025-05-28T08:16:15Z

src/lib/bump-stats/index.ts

+			console.log( 'bumpStat', group, stat, bumpInDev );
+


Suggested change

console.log( 'bumpStat', group, stat, bumpInDev );

fredrikekelund · 2025-05-28T08:30:59Z

src/storage/user-data.ts

+const LOCKFILE_PATH = nodePath.join( getResourcesPath(), LOCKFILE_NAME );
+
+export async function lockAppdata() {
+	return lockFileAsync( LOCKFILE_PATH, { wait: 1000, stale: 1000 } );


I'm not sure what the right configuration is, but I have a feeling that we should be more strict with this configuration.

Firstly, I believe it's redundant to specify the wait option if we also have stale. The current configuration is saying "proceed anyway after 1000ms" AND "throw an error if the lock isn't released within 1000ms". AFAICT, the lockfile module ignores the wait option when stale is also specified.

I don't think our implementation is robust enough to use the wait option exclusively. I recommend proceeding by removing wait and increasing the value of stale.

fredrikekelund · 2025-05-28T08:32:24Z

src/ipc-handlers.ts

-	userData.sites.push( server.details );
-	sortSites( userData.sites );
-	await saveUserData( userData );
+		userData.sites.push( server.details );


Suggested change

userData.sites.push( server.details );

await lockAppdata();

const userData = await loadUserData();

userData.sites.push( server.details );

I recommend acquiring the lock here. SiteServer.create can take a long time to resolve. Holding the lock for such a long time is risky, since it may go stale.

bcotrim · 2025-05-28T13:36:37Z

On a more birds-eye-view note, I also recommend adding something like this to cli/index.ts:
process.on( 'uncaughtExceptionMonitor', async () => {
	await unlockAppdata();
} );
We should do the same for the Studio app and add an equivalent handler in src/index.ts.

We always wrap the unlock call in a finally and are using the stale option from lockfile so I don't think this is necessary. Is there any scenario I'm missing?

fredrikekelund · 2025-05-28T14:16:28Z

Is there any scenario I'm missing?

I was thinking of a scenario where an unexpected exception is thrown from somewhere other than the function holding the lock. This could theoretically happen if we're juggling simultaneous async operations.

fredrikekelund · 2025-05-30T12:06:36Z

I realize that the uncaughtExceptionMonitor could potentially lead to issues where the current process unlocks the lockfile despite not holding the lock to begin with… Maybe it's better to avoid it for the time being. The other changes here still go a very long way towards fixing the concurrent write problem.

fredrikekelund

Moving the try { clause in the createSite function is the only relatively high-priority change I see remaining. Aside from that, this PR LGTM, and I've confirmed that it fixes the issue I reported to begin with 👍

Apparently, I can't approve the PR because I'm also the PR author, but feel free to add your own approval in my stead, @bcotrim

fredrikekelund · 2025-05-30T11:23:05Z

src/ipc-handlers.ts

@@ -174,124 +179,134 @@ export async function createSite(
 		// Form validation should've prevented a non-empty directory from being selected
 		throw new Error( 'The selected directory is not empty nor an existing WordPress site.' );
 	}
+	try {


Let's move the opening try { clause closer to the lockAppdata call. If an error is thrown before we acquire the lock, that could lead to incorrect behavior.

fredrikekelund · 2025-05-30T11:24:45Z

src/ipc-handlers.ts

-		site.id === updatedSite.id ? updatedSite : site
-	);
-	userData.sites = updatedSites;
+export async function updateSite( event: IpcMainInvokeEvent, updatedSite: SiteDetails ) {


Suggested change

export async function updateSite( event: IpcMainInvokeEvent, updatedSite: SiteDetails ) {

export async function updateSite( event: IpcMainInvokeEvent, updatedSite: SiteDetails ): Promise< void > {

This is most definitely optional, but I find it helpful to add the explicit return types back here to clarify the intention. I removed the return types to accommodate the withUserDataWrite type definition.

fredrikekelund · 2025-05-30T11:25:12Z

src/ipc-handlers.ts

@@ -161,8 +167,7 @@ export async function createSite(
 	customDomain?: string,
 	enableHttps?: boolean,
 	siteId?: string
-): Promise< SiteDetails[] > {
-	const userData = await loadUserData();
+) {


Suggested change

) {

): Promise< SiteDetails > {

Per my other comment about return types.

fredrikekelund · 2025-05-30T11:28:46Z

src/lib/oauth.ts

 async function storeToken( token: StoredToken ) {
-	try {
-		const userData = await loadUserData();
-		await saveUserData( {
-			...userData,
-			authToken: token,
-		} );
-	} catch ( error ) {
-		console.error( 'Failed to store token', error );
-	}
+	await updateAppdata( { authToken: token } );
 }


Given how succinct this function is now and that it isn't exported, I'd consider inlining it in onOpenUrlCallback

fredrikekelund · 2025-05-30T11:29:39Z

src/lib/oauth.ts

 export async function clearAuthenticationToken() {
-	try {
-		const userData = await loadUserData();
-		await saveUserData( {
-			...userData,
-			authToken: undefined,
-		} );
-	} catch ( error ) {
-		return;
-	}
+	await updateAppdata( { authToken: undefined } );
 }


Optional, but we could also inline this in the clearAuthenticationToken function in ipc-handlers

fredrikekelund · 2025-05-30T11:41:11Z

src/storage/tests/user-data.test.ts

-		test( 'saves user data correctly', async () => {
-			await saveUserData( mockedUserData as UserData );
-			expect( atomically.writeFile ).toHaveBeenCalledWith(
-				normalize( '/path/to/app/appData/App Name/appdata-v1.json' ),
-				JSON.stringify(
-					{
-						version: 1,
-						sites: mockedUserData.sites?.map( ( site ) => ( {
-							...site,
-							themeDetails: defaultThemeDetails,
-						} ) ),
-						snapshots: [],
-					},
-					null,
-					2
-				) + '\n',
-				'utf-8'
-			);
-		} );


We could bring back this test case, too. Not critical, though.

fredrikekelund · 2025-05-30T12:03:11Z

cli/lib/tests/appdata.test.ts

The changes to this file do not need to be reverted, but the tests still pass without them.

fredrikekelund added 3 commits May 5, 2025 10:52

[WIP] Appdata lockfile

ffe30c1

Tweak

75ebfa3

More locks

559b01c

fredrikekelund requested a review from bcotrim May 5, 2025 09:00

fredrikekelund self-assigned this May 5, 2025

fredrikekelund commented May 5, 2025

View reviewed changes

bcotrim reviewed May 5, 2025

View reviewed changes

fredrikekelund added 4 commits May 14, 2025 11:23

Merge branch 'trunk' into f26d/lockfile-poc

147aeff

withAppdataLock

4d0cf79

More withAppdataLock

e8585b9

Even more withAppdataLock

fb22226

Fixes

d42cd66

bcotrim reviewed May 14, 2025

View reviewed changes

fredrikekelund added 5 commits May 15, 2025 10:46

Tweaks

4a7bec7

Generator

349037e

Tweaks

a06da61

Return a tuple

19b526e

Moving and commenting

f22c892

Remove withLock

fb79af8

fredrikekelund requested a review from nightnei May 15, 2025 12:58

Merge branch 'trunk' into f26d/lockfile-poc

5433872

fredrikekelund requested a review from a team May 16, 2025 08:15

fredrikekelund commented May 19, 2025

View reviewed changes

nightnei reviewed May 19, 2025

View reviewed changes

bcotrim mentioned this pull request May 23, 2025

ESLint rule for user data lockfile #1430

Open

1 task

fredrikekelund assigned bcotrim May 26, 2025

fredrikekelund and others added 2 commits May 26, 2025 15:22

Fixing more tests

ef8a1b1

lockfile in cli files

446559f

bcotrim force-pushed the f26d/lockfile-poc branch 2 times, most recently from 6708e55 to 446559f Compare May 27, 2025 07:53

bcotrim and others added 6 commits May 27, 2025 09:25

fix CLI unit test

cc227e2

adjust lock function

65fe249

adjust studio app code

f228cba

fix unit tests

e9aaf06

Merge branch 'trunk' into f26d/lockfile-poc

b98aa68

fix bump stats lockfile

1f41116

bcotrim changed the title ~~[WIP] Appdata lockfile~~ Appdata lockfile May 27, 2025

bcotrim marked this pull request as ready for review May 27, 2025 12:36

nightnei mentioned this pull request May 27, 2025

refactor: isolate locking appdata #1443

Open

fredrikekelund commented May 28, 2025

View reviewed changes

address pr feedback

1fce729

bcotrim and others added 2 commits May 28, 2025 15:57

Merge branch 'trunk' into f26d/lockfile-poc

b99fed8

fix oauth unit test

8287a21

fredrikekelund commented May 30, 2025

View reviewed changes

address pr feedback

bebbe90


		type MaybePromise< T > = T \| Promise< T >;

		export function withAppdataLock< Args extends unknown[] >(

	const LOCKFILE_PATH = path.join( getAppdataDirectory(), 'appdata-v1.lock' );
	const LOCKFILE_PATH = path.join( getAppdataDirectory(), 'appdata-v1.json.lock' );

	const createNewSite = async ( siteFolder: string ): Promise< z.infer< typeof siteSchema > > => {
	const createNewSite = async ( siteFolder: string ): Promise< SiteData > => {

	): Promise< z.infer< typeof siteSchema > > => {
	): Promise< SiteData > => {

		const site = await getSiteByFolder( siteFolder );
		return site;

	const site = await getSiteByFolder( siteFolder );
	return site;
	return await getSiteByFolder( siteFolder );

		@@ -0,0 +1,25 @@
		import lockfile from 'lockfile';

		export function lockFileAsync( LOCKFILE_PATH: string, options: lockfile.Options ) {

	export function lockFileAsync( LOCKFILE_PATH: string, options: lockfile.Options ) {
	export function lockFileAsync( lockfilePath: string, options: lockfile.Options ) {

	export async function updateSite( event: IpcMainInvokeEvent, updatedSite: SiteDetails ) {
	export async function updateSite( event: IpcMainInvokeEvent, updatedSite: SiteDetails ): Promise< void > {

Appdata lockfile #1319

Are you sure you want to change the base?

Appdata lockfile #1319

Conversation

fredrikekelund commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcotrim left a comment

Choose a reason for hiding this comment

Uh oh!

fredrikekelund commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrikekelund commented May 15, 2025

Uh oh!

fredrikekelund commented May 15, 2025

Uh oh!

fredrikekelund commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightnei left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrikekelund commented May 20, 2025

Uh oh!

nightnei commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcotrim commented May 23, 2025

Uh oh!

fredrikekelund commented May 26, 2025

Uh oh!

nightnei commented May 26, 2025

Uh oh!

bcotrim commented May 27, 2025

Uh oh!

nightnei commented May 27, 2025

Uh oh!

nightnei commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcotrim commented May 28, 2025

fredrikekelund commented May 5, 2025 •

edited

Loading

nightnei left a comment •

edited

Loading

nightnei commented May 20, 2025 •

edited

Loading

nightnei commented May 27, 2025 •

edited

Loading