E2E: drop proper-lockfile, put the login back in the fixtures

The guard that serializes changes to the lock directory is now built the
same way as the lock itself, out of mkdir and an owner directory whose
mtime a heartbeat keeps current. One less dependency for a few lines that
already existed next to it. Taking over a stale lock now tolerates losing
the mkdir race rather than throwing.

The account fixtures authenticate again on first use, through getAccount.
Handing back an unauthenticated TestAccount pushed the call into every
spec that took a fixture and left the ones that didn't make it silently
logged out.

Author: lucatume

packages/calypso-e2e/package.json

@@ -35,7 +35,6 @@
 		"jest-environment-node": "^29.7.0",
 		"mailosaur": "^8.4.0",
 		"playwright": "1.57.0",
-		"proper-lockfile": "^4.1.2",
 		"totp-generator": "^0.0.14",
 		"tslib": "^2.8.1"
 	},
@@ -46,7 +45,6 @@
 		"@automattic/zendesk-client": "workspace:^",
 		"@jest/globals": "^29.7.0",
 		"@types/node": "^24.12.2",
-		"@types/proper-lockfile": "^4.1.4",
 		"nock": "^14.0.15",
 		"typescript": "^6.0.3"
 	},

packages/calypso-e2e/src/lib/test-account.ts

@@ -3,7 +3,6 @@ import fs from 'fs/promises';
 import path from 'path';
 import chalk from 'chalk';
 import { BrowserContext, Page } from 'playwright';
-import { lock } from 'proper-lockfile';
 import { TestAccountName } from '..';
 import { getAccountSiteURL, getCalypsoURL } from '../data-helper';
 import { EmailClient } from '../email-client';
@@ -19,6 +18,7 @@ const LOCK_POLL_MS = 500;
 const LOCK_HEARTBEAT_MS = 10 * 1000;
 const LOCK_STALE_MS = 45 * 1000;
 const LOCK_WAIT_MS = 90 * 1000;
+const LOCK_GUARD_HEARTBEAT_MS = 2 * 1000;
 const LOCK_GUARD_STALE_MS = 10 * 1000;
 const LOCK_GUARD_WAIT_MS = 15 * 1000;
 const LOGIN_ATTEMPTS = 2;
@@ -219,7 +219,14 @@ export class TestAccount {
 				throw error;
 			}
 			await fs.rm( stalePath, { force: true, recursive: true } );
-			await fs.mkdir( lockPath );
+			try {
+				await fs.mkdir( lockPath );
+			} catch ( error ) {
+				if ( hasErrorCode( error, 'EEXIST' ) ) {
+					return null;
+				}
+				throw error;
+			}
 			this.log( 'Took over a stale login lock' );
 			return await this.createOwnedLoginLock();
 		} finally {
@@ -347,19 +354,98 @@ export class TestAccount {
 
 	/** Tries to acquire the guard that serializes lock directory changes. */
 	private async acquireLoginLockGuard(): Promise< ( () => Promise< void > ) | null > {
+		const guardPath = `${ this.getLoginLockPath() }.guard`;
+		const ownerPath = path.join( guardPath, `${ randomUUID() }.owner` );
+
 		try {
-			return await lock( this.getLoginLockPath(), {
-				lockfilePath: `${ this.getLoginLockPath() }.guard`,
-				realpath: false,
-				stale: LOCK_GUARD_STALE_MS,
-				update: 2 * 1000,
-			} );
+			await fs.mkdir( guardPath );
 		} catch ( error ) {
-			if ( hasErrorCode( error, 'ELOCKED' ) ) {
+			if ( ! hasErrorCode( error, 'EEXIST' ) ) {
+				throw error;
+			}
+
+			let newestMtime: number;
+			let owners: string[];
+			try {
+				newestMtime = ( await fs.stat( guardPath ) ).mtimeMs;
+				owners = await fs.readdir( guardPath );
+			} catch ( inspectError ) {
+				if ( hasErrorCode( inspectError, 'ENOENT' ) ) {
+					return null;
+				}
+				throw inspectError;
+			}
+
+			for ( const owner of owners ) {
+				try {
+					newestMtime = Math.max(
+						newestMtime,
+						( await fs.stat( path.join( guardPath, owner ) ) ).mtimeMs
+					);
+				} catch ( inspectError ) {
+					if ( hasErrorCode( inspectError, 'ENOENT' ) ) {
+						return null;
+					}
+					throw inspectError;
+				}
+			}
+
+			if ( Date.now() < newestMtime + LOCK_GUARD_STALE_MS ) {
 				return null;
 			}
+
+			const stalePath = `${ guardPath }.${ randomUUID() }.stale`;
+			try {
+				await fs.rename( guardPath, stalePath );
+			} catch ( renameError ) {
+				if ( hasErrorCode( renameError, 'ENOENT' ) ) {
+					return null;
+				}
+				throw renameError;
+			}
+			await fs.rm( stalePath, { force: true, recursive: true } );
+			return null;
+		}
+
+		try {
+			await fs.mkdir( ownerPath );
+		} catch ( error ) {
+			await fs.rmdir( guardPath ).catch( () => {} );
 			throw error;
 		}
+
+		let heartbeatError: unknown;
+		let heartbeatPromise = Promise.resolve();
+		const heartbeatTimer = setInterval( () => {
+			heartbeatPromise = heartbeatPromise.then( async () => {
+				try {
+					const now = new Date();
+					await fs.utimes( ownerPath, now, now );
+				} catch ( error ) {
+					heartbeatError ??= error;
+				}
+			} );
+		}, LOCK_GUARD_HEARTBEAT_MS );
+		heartbeatTimer.unref();
+
+		return async () => {
+			clearInterval( heartbeatTimer );
+			await heartbeatPromise;
+			if ( heartbeatError ) {
+				throw new Error( `Lost login lock guard for ${ this.accountName }`, {
+					cause: heartbeatError,
+				} );
+			}
+			try {
+				await fs.rmdir( ownerPath );
+			} catch ( error ) {
+				if ( hasErrorCode( error, 'ENOENT' ) ) {
+					throw new Error( `Lost login lock guard for ${ this.accountName }`, { cause: error } );
+				}
+				throw error;
+			}
+			await fs.rmdir( guardPath );
+		};
 	}
 
 	/** Returns this account's login lock directory. */

test/e2e/AGENTS.md

@@ -141,7 +141,7 @@ test( 'Test', async ( { pageLogin, componentSidebar } ) => {
 
 ### Available Fixtures
 
-**Accounts**: one fixture per key of `fixtureAccounts` in [`lib/pw-base.ts`](lib/pw-base.ts), plus `accountGivenByEnvironment` and `accountSMS`. Adding a key there adds the fixture; calling `authenticate()` logs the account in when needed.
+**Accounts**: one fixture per key of `fixtureAccounts` in [`lib/pw-base.ts`](lib/pw-base.ts), plus `accountGivenByEnvironment` and `accountSMS`. Adding a key there adds the fixture; the account is logged in the first time a spec asks for it.
 
 **Pages/Components**: Follow naming conventions:
 

test/e2e/docs/tests_local.md

@@ -70,9 +70,9 @@ See the [list of groups](tests_ci.md#featuretest-groups).
 
 ### Save authentication cookies
 
-The first call to `TestAccount.authenticate()` for an account logs in and saves its cookies under `COOKIES_PATH`, to be re-used until expiry (typically 2 days). Every later authentication reads that file instead of logging in again.
+The first test that needs an account logs in and saves its cookies under `COOKIES_PATH`, to be re-used until expiry (typically 2 days). Every other test and worker reads that file instead of logging in again.
 
-Workers authenticating the same account at the same time race for a lock beside the cookies: the winner logs in, and the rest wait for the cookies it writes. So each account is logged in once per run without being listed anywhere in advance.
+Workers that need the same account at the same time race for a lock beside the cookies: the winner logs in, the rest wait for the cookies it writes. So each account is logged in once per run, whichever spec gets there first, and nothing has to be listed anywhere in advance.
 
 Delete the `cookies` directory to force a fresh login.
 

test/e2e/lib/get-account.ts

@@ -0,0 +1,21 @@
+import { TestAccount, TestAccountName } from '@automattic/calypso-e2e';
+import { Page } from 'playwright';
+
+/**
+ * Retrieves and authenticates a `TestAccount` instance for the specified account name.
+ *
+ * If the account does not have fresh authentication cookies, this function logs in via the login page.
+ * Otherwise, it loads the saved cookies into the browser context.
+ *
+ * @param {Page} page - The Playwright `Page` instance to use for authentication actions.
+ * @param {TestAccountName} accountName - The name of the test account to retrieve.
+ * @returns {Promise< TestAccount >} A promise that resolves to a `TestAccount` instance with valid authentication cookies.
+ */
+export async function getAccount(
+	page: Page,
+	accountName: TestAccountName
+): Promise< TestAccount > {
+	const testAccount = new TestAccount( accountName );
+	await testAccount.authenticate( page );
+	return testAccount;
+}

test/e2e/lib/pw-base.ts

@@ -90,14 +90,15 @@ import {
 	UseADomainIOwnPage,
 	SelectItemsComponent,
 } from '@automattic/calypso-e2e';
-import { test as base, expect } from '@playwright/test';
+import { test as base, expect, type Page } from '@playwright/test';
 import {
 	apiCloseAccount,
 	apiWaitForBearerTokenAcceptance,
 	apiWaitForEmailVerification,
 } from '../specs/shared';
 import { useBlackboxTestKeyForCollect } from './blackbox-test-key';
 import { snoozeAccountRecoveryInterstitial } from './dashboard-helpers';
+import { getAccount } from './get-account';
 
 export type CustomOptions = {
 	/**
@@ -108,7 +109,7 @@ export type CustomOptions = {
 };
 
 /**
- * Test accounts exposed as fixtures of the same name.
+ * Test accounts exposed as a fixture of the same name, logged in on first use.
  *
  * Two accounts are fixtures without belonging here: `accountGivenByEnvironment`, which
  * resolves at run time, and `accountSMS`, whose 2FA code costs a Mailosaur email only a
@@ -125,7 +126,7 @@ export const fixtureAccounts = {
 } as const satisfies Record< string, TestAccountName >;
 
 type AccountFixture = (
-	args: object,
+	args: { page: Page },
 	use: ( account: TestAccount ) => Promise< void >
 ) => Promise< void >;
 
@@ -390,17 +391,20 @@ export const test = base.extend<
 	...( Object.fromEntries(
 		Object.entries( fixtureAccounts ).map( ( [ fixtureName, accountName ] ) => [
 			fixtureName,
-			async ( {}, use ) => {
-				await use( new TestAccount( accountName ) );
+			async ( { page }, use ) => {
+				const testAccount = await getAccount( page, accountName );
+				await use( testAccount );
 			},
 		] )
 	) as Record< keyof typeof fixtureAccounts, AccountFixture > ),
-	accountGivenByEnvironment: async ( {}, use ) => {
+	accountGivenByEnvironment: async ( { page }, use ) => {
 		const accountName = getTestAccountByFeature( envToFeatureKey( envVariables ) );
-		await use( new TestAccount( accountName ) );
+		const testAccount = await getAccount( page, accountName );
+		await use( testAccount );
 	},
-	accountSMS: async ( {}, use ) => {
-		await use( new TestAccount( 'smsUser' ) );
+	accountSMS: async ( { page }, use ) => {
+		const testAccount = await getAccount( page, 'smsUser' );
+		await use( testAccount );
 	},
 	clientEmail: async ( {}, use ) => {
 		const emailClient = new EmailClient();
@@ -677,8 +681,7 @@ export const test = base.extend<
 		}
 	},
 	sitePublicShared: async ( { page, helperData }, use ) => {
-		const account = new TestAccount( 'defaultUser' );
-		await account.authenticate( page );
+		const account = await getAccount( page, 'defaultUser' );
 
 		// createSite is the first line that creates a real resource. From here on
 		// everything is wrapped so the site is deleted no matter what happens next:

yarn.lock

@@ -614,7 +614,6 @@ __metadata:
     "@playwright/browser-chromium": "npm:1.57.0"
     "@playwright/browser-firefox": "npm:1.57.0"
     "@types/node": "npm:^24.12.2"
-    "@types/proper-lockfile": "npm:^4.1.4"
     "@types/totp-generator": "npm:^0.0.8"
     "@wordpress/i18n": "npm:^6.21.0"
     asana-phrase: "npm:^0.0.8"
@@ -625,7 +624,6 @@ __metadata:
     mailosaur: "npm:^8.4.0"
     nock: "npm:^14.0.15"
     playwright: "npm:1.57.0"
-    proper-lockfile: "npm:^4.1.2"
     totp-generator: "npm:^0.0.14"
     tslib: "npm:^2.8.1"
     typescript: "npm:^6.0.3"
@@ -9698,15 +9696,6 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@types/proper-lockfile@npm:^4.1.4":
-  version: 4.1.4
-  resolution: "@types/proper-lockfile@npm:4.1.4"
-  dependencies:
-    "@types/retry": "npm:*"
-  checksum: d597846d6f7860da2470623d3f11b6e5b39864719c3c18b5a5e55f783c5e432fe49dcdcf6341b3903428fdf103f2bdc68eba263ce1ce3cbe8070cbcb54bbf230
-  languageName: node
-  linkType: hard
-
 "@types/qs@npm:*, @types/qs@npm:^6.9.7":
   version: 6.9.7
   resolution: "@types/qs@npm:6.9.7"
@@ -9800,13 +9789,6 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@types/retry@npm:*":
-  version: 0.12.5
-  resolution: "@types/retry@npm:0.12.5"
-  checksum: eaaca483cc62f2f02c0b8486847ee70986ca7f97afd7363037247dbe3e98df8bd56a5b50d58b1e96768a5a1be0307010d86e9991bd458d72e8df88be471bd720
-  languageName: node
-  linkType: hard
-
 "@types/seed-random@npm:^2.2.4":
   version: 2.2.4
   resolution: "@types/seed-random@npm:2.2.4"