Skip to content

Commit aa7488b

Browse files
committed
fix: persistent web link with crash-safe writes, optimistic UI, and race-condition guard
- Add persistentWebLink setting and two IPC handlers (persistCurrentToken / clearPersistentToken) - Crash-safe write ordering: flag before token on enable, flag before token on disable - UUID v4 validation of stored tokens in web-server-factory with auto-regeneration fallback - Optimistic UI updates with stale-request sequence counter scoped inside Zustand closure - Toggle disabled state (isPersistPending) with aria-busy during IPC round-trip - clearPersistentToken handler surfaces disk errors via try/catch for renderer rollback - Shared SettingsStoreInterface extracted to stores/types.ts - Comprehensive tests: IPC handlers, factory token scenarios, store race-condition rollbacks Note: silent token regeneration notification is intentionally omitted (out of scope)
1 parent d157630 commit aa7488b

11 files changed

Lines changed: 268 additions & 42 deletions

File tree

src/__tests__/main/ipc/handlers/web.test.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -359,14 +359,22 @@ describe('web handlers', () => {
359359
});
360360

361361
describe('live:persistCurrentToken', () => {
362-
it('should persist the current server token and enable the flag atomically', async () => {
362+
it('should write flag before token for crash safety', async () => {
363363
const handler = registeredHandlers.get('live:persistCurrentToken');
364364
const result = await handler!({});
365365

366366
expect(mockWebServer.getSecurityToken).toHaveBeenCalled();
367-
expect(mockSettingsStore.set).toHaveBeenCalledWith('webAuthToken', 'mock-security-token');
368367
expect(mockSettingsStore.set).toHaveBeenCalledWith('persistentWebLink', true);
368+
expect(mockSettingsStore.set).toHaveBeenCalledWith('webAuthToken', 'mock-security-token');
369369
expect(result).toEqual({ success: true });
370+
371+
// Verify crash-safe write order: flag enabled before token.
372+
// A crash between the two writes leaves persistentWebLink=true with
373+
// a missing token, which the factory handles by generating a fresh UUID.
374+
const setCalls = vi.mocked(mockSettingsStore.set).mock.calls;
375+
const flagIndex = setCalls.findIndex(([key]) => key === 'persistentWebLink');
376+
const tokenIndex = setCalls.findIndex(([key]) => key === 'webAuthToken');
377+
expect(flagIndex).toBeLessThan(tokenIndex);
370378
});
371379

372380
it('should return failure when web server is null', async () => {
@@ -390,13 +398,33 @@ describe('web handlers', () => {
390398
});
391399

392400
describe('live:clearPersistentToken', () => {
393-
it('should clear both settings atomically', async () => {
401+
it('should clear flag before token for crash safety', async () => {
394402
const handler = registeredHandlers.get('live:clearPersistentToken');
395403
const result = await handler!({});
396404

405+
// Verify both writes are made
397406
expect(mockSettingsStore.set).toHaveBeenCalledWith('persistentWebLink', false);
398407
expect(mockSettingsStore.set).toHaveBeenCalledWith('webAuthToken', null);
399408
expect(result).toEqual({ success: true });
409+
410+
// Verify crash-safe write order: flag cleared before token.
411+
// A crash between the two writes must leave persistentWebLink=false
412+
// so the factory ignores the stale token on next startup.
413+
const setCalls = vi.mocked(mockSettingsStore.set).mock.calls;
414+
const flagIndex = setCalls.findIndex(([key]) => key === 'persistentWebLink');
415+
const tokenIndex = setCalls.findIndex(([key]) => key === 'webAuthToken');
416+
expect(flagIndex).toBeLessThan(tokenIndex);
417+
});
418+
419+
it('should return failure when settings write throws', async () => {
420+
mockSettingsStore.set.mockImplementationOnce(() => {
421+
throw new Error('disk full');
422+
});
423+
424+
const handler = registeredHandlers.get('live:clearPersistentToken');
425+
const result = await handler!({});
426+
427+
expect(result).toEqual({ success: false, message: 'disk full' });
400428
});
401429
});
402430

src/__tests__/main/web-server/web-server-factory.test.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ describe('web-server/web-server-factory', () => {
218218
});
219219

220220
it('should use stored token when persistentWebLink is true and token is a valid UUID', () => {
221-
const validUuid = '550e8400-e29b-41d4-a716-446655440000';
221+
const validUuid = '550e8400-e29b-4bd4-a716-446655440000';
222222
vi.mocked(mockSettingsStore.get).mockImplementation((key: string, defaultValue?: any) => {
223223
if (key === 'persistentWebLink') return true;
224224
if (key === 'webAuthToken') return validUuid;
@@ -245,6 +245,15 @@ describe('web-server/web-server-factory', () => {
245245
expect((server as any).securityToken).not.toBe('not-a-valid-uuid');
246246
expect((server as any).securityToken).toBeDefined();
247247
expect(mockSettingsStore.set).toHaveBeenCalledWith('webAuthToken', expect.any(String));
248+
// Token written to settings must match the one given to the server
249+
const storedToken = vi
250+
.mocked(mockSettingsStore.set)
251+
.mock.calls.find(([key]) => key === 'webAuthToken')?.[1];
252+
expect((server as any).securityToken).toBe(storedToken);
253+
// Generated replacement must be a valid UUID v4
254+
expect(storedToken).toMatch(
255+
/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i
256+
);
248257
});
249258

250259
it('should generate and store new token when persistentWebLink is true and no token exists', () => {
@@ -261,6 +270,15 @@ describe('web-server/web-server-factory', () => {
261270
expect((server as any).securityToken).toBeDefined();
262271
expect(typeof (server as any).securityToken).toBe('string');
263272
expect(mockSettingsStore.set).toHaveBeenCalledWith('webAuthToken', expect.any(String));
273+
// Token written to settings must match the one given to the server
274+
const storedToken = vi
275+
.mocked(mockSettingsStore.set)
276+
.mock.calls.find(([key]) => key === 'webAuthToken')?.[1];
277+
expect((server as any).securityToken).toBe(storedToken);
278+
// Generated token must be a valid UUID v4
279+
expect(storedToken).toMatch(
280+
/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i
281+
);
264282
});
265283
});
266284

src/__tests__/renderer/stores/settingsStore.test.ts

Lines changed: 132 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1636,7 +1636,138 @@ describe('settingsStore', () => {
16361636
});
16371637

16381638
// ========================================================================
1639-
// 13. Non-React Access
1639+
// 13. setPersistentWebLink race-condition and rollback tests
1640+
// ========================================================================
1641+
1642+
describe('setPersistentWebLink', () => {
1643+
beforeEach(() => {
1644+
useSettingsStore.setState({ persistentWebLink: false });
1645+
});
1646+
1647+
it('should optimistically set persistentWebLink to true and call persistCurrentToken', async () => {
1648+
const { setPersistentWebLink } = useSettingsStore.getState();
1649+
await setPersistentWebLink(true);
1650+
1651+
expect(useSettingsStore.getState().persistentWebLink).toBe(true);
1652+
expect(window.maestro.live.persistCurrentToken).toHaveBeenCalledOnce();
1653+
});
1654+
1655+
it('should rollback to false on soft IPC failure (result.success === false)', async () => {
1656+
vi.mocked(window.maestro.live.persistCurrentToken).mockResolvedValueOnce({
1657+
success: false,
1658+
message: 'Web server is not running.',
1659+
});
1660+
1661+
const { setPersistentWebLink } = useSettingsStore.getState();
1662+
await setPersistentWebLink(true);
1663+
1664+
expect(useSettingsStore.getState().persistentWebLink).toBe(false);
1665+
});
1666+
1667+
it('should rollback to false on hard IPC failure (thrown exception)', async () => {
1668+
vi.mocked(window.maestro.live.persistCurrentToken).mockRejectedValueOnce(
1669+
new Error('IPC timeout')
1670+
);
1671+
1672+
const { setPersistentWebLink } = useSettingsStore.getState();
1673+
await setPersistentWebLink(true);
1674+
1675+
expect(useSettingsStore.getState().persistentWebLink).toBe(false);
1676+
});
1677+
1678+
it('should call clearPersistentToken when disabling', async () => {
1679+
useSettingsStore.setState({ persistentWebLink: true });
1680+
1681+
const { setPersistentWebLink } = useSettingsStore.getState();
1682+
await setPersistentWebLink(false);
1683+
1684+
expect(useSettingsStore.getState().persistentWebLink).toBe(false);
1685+
expect(window.maestro.live.clearPersistentToken).toHaveBeenCalledOnce();
1686+
});
1687+
1688+
it('should rollback to true on clearPersistentToken hard failure (thrown exception)', async () => {
1689+
useSettingsStore.setState({ persistentWebLink: true });
1690+
vi.mocked(window.maestro.live.clearPersistentToken).mockRejectedValueOnce(
1691+
new Error('IPC timeout')
1692+
);
1693+
1694+
const { setPersistentWebLink } = useSettingsStore.getState();
1695+
await setPersistentWebLink(false);
1696+
1697+
expect(useSettingsStore.getState().persistentWebLink).toBe(true);
1698+
});
1699+
1700+
it('should rollback to true on clearPersistentToken soft failure (result.success === false)', async () => {
1701+
useSettingsStore.setState({ persistentWebLink: true });
1702+
vi.mocked(window.maestro.live.clearPersistentToken).mockResolvedValueOnce({
1703+
success: false,
1704+
message: 'Settings write failed.',
1705+
} as any);
1706+
1707+
const { setPersistentWebLink } = useSettingsStore.getState();
1708+
await setPersistentWebLink(false);
1709+
1710+
expect(useSettingsStore.getState().persistentWebLink).toBe(true);
1711+
});
1712+
1713+
it('should handle rapid double-toggle (enable then disable) correctly', async () => {
1714+
// Simulate enable call that resolves slowly
1715+
let resolveEnable: (value: any) => void;
1716+
const slowEnable = new Promise((resolve) => {
1717+
resolveEnable = resolve;
1718+
});
1719+
vi.mocked(window.maestro.live.persistCurrentToken).mockReturnValueOnce(slowEnable as any);
1720+
1721+
const { setPersistentWebLink } = useSettingsStore.getState();
1722+
1723+
// Start enable (will be in-flight)
1724+
const enablePromise = setPersistentWebLink(true);
1725+
// Immediately disable (supersedes the enable)
1726+
const disablePromise = setPersistentWebLink(false);
1727+
1728+
// Resolve the slow enable after disable was called
1729+
resolveEnable!({ success: true });
1730+
1731+
await enablePromise;
1732+
await disablePromise;
1733+
1734+
// Final state should reflect the last user intent: disabled
1735+
expect(useSettingsStore.getState().persistentWebLink).toBe(false);
1736+
expect(window.maestro.live.clearPersistentToken).toHaveBeenCalled();
1737+
});
1738+
1739+
it('should handle rapid reverse toggle (disable then enable) correctly', async () => {
1740+
// Start with enabled state
1741+
useSettingsStore.setState({ persistentWebLink: true });
1742+
1743+
// Simulate disable call that resolves slowly
1744+
let resolveClear: (value: any) => void;
1745+
const slowClear = new Promise((resolve) => {
1746+
resolveClear = resolve;
1747+
});
1748+
vi.mocked(window.maestro.live.clearPersistentToken).mockReturnValueOnce(slowClear as any);
1749+
1750+
const { setPersistentWebLink } = useSettingsStore.getState();
1751+
1752+
// Start disable (will be in-flight)
1753+
const disablePromise = setPersistentWebLink(false);
1754+
// Immediately re-enable (supersedes the disable)
1755+
const enablePromise = setPersistentWebLink(true);
1756+
1757+
// Resolve the slow clear after enable was called
1758+
resolveClear!({ success: true });
1759+
1760+
await disablePromise;
1761+
await enablePromise;
1762+
1763+
// Final state should reflect the last user intent: enabled
1764+
expect(useSettingsStore.getState().persistentWebLink).toBe(true);
1765+
expect(window.maestro.live.persistCurrentToken).toHaveBeenCalled();
1766+
});
1767+
});
1768+
1769+
// ========================================================================
1770+
// 14. Non-React Access
16401771
// ========================================================================
16411772

16421773
describe('non-React access', () => {

src/__tests__/setup.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,18 @@ const mockMaestro = {
404404
importPlaybook: vi.fn().mockResolvedValue({ success: true, playbook: {}, importedDocs: [] }),
405405
onManifestChanged: vi.fn().mockReturnValue(() => {}),
406406
},
407+
live: {
408+
toggle: vi.fn().mockResolvedValue({ live: false, url: null }),
409+
getStatus: vi.fn().mockResolvedValue({ live: false, url: null }),
410+
getDashboardUrl: vi.fn().mockResolvedValue(null),
411+
getLiveSessions: vi.fn().mockResolvedValue([]),
412+
broadcastActiveSession: vi.fn().mockResolvedValue(undefined),
413+
startServer: vi.fn().mockResolvedValue({ success: true, url: 'http://localhost:3000' }),
414+
stopServer: vi.fn().mockResolvedValue({ success: true }),
415+
persistCurrentToken: vi.fn().mockResolvedValue({ success: true }),
416+
clearPersistentToken: vi.fn().mockResolvedValue({ success: true }),
417+
disableAll: vi.fn().mockResolvedValue({ success: true, count: 0 }),
418+
},
407419
web: {
408420
broadcastAutoRunState: vi.fn(),
409421
broadcastSessionState: vi.fn(),

src/main/ipc/handlers/web.ts

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
* - live:broadcastActiveSession: Broadcast active session change
1414
* - live:startServer: Start the web server
1515
* - live:stopServer: Stop the web server
16+
* - live:persistCurrentToken: Persist the running server's token and enable persistent web link
17+
* - live:clearPersistentToken: Clear the persisted token and disable persistent web link
1618
* - live:disableAll: Disable all live sessions and stop server
1719
* - webserver:getUrl: Get the web server URL
1820
* - webserver:getConnectedClients: Get connected client count
@@ -261,27 +263,35 @@ export function registerWebHandlers(deps: WebHandlerDependencies): void {
261263
}
262264
});
263265

264-
// Persist the current web server's security token and enable persistent web link
265-
// Writes both webAuthToken and persistentWebLink atomically on the main side
266-
// to prevent inconsistent disk state if the app is force-quit mid-operation
266+
// Persist the current web server's security token and enable persistent web link.
267+
// Flag is written first: a crash between the two writes leaves
268+
// persistentWebLink=true with a missing/stale token, which the factory
269+
// handles by generating and persisting a fresh UUID on next startup.
267270
ipcMain.handle('live:persistCurrentToken', async () => {
268271
const webServer = getWebServer();
269272
if (!webServer || !webServer.isActive()) {
270273
return { success: false, message: 'Web server is not running.' };
271274
}
272275
const currentToken = webServer.getSecurityToken();
273-
settingsStore.set('webAuthToken', currentToken);
274276
settingsStore.set('persistentWebLink', true);
277+
settingsStore.set('webAuthToken', currentToken);
275278
logger.info('Persisted current web server token and enabled persistent web link', 'WebServer');
276279
return { success: true };
277280
});
278281

279-
// Clear persistent web link token and disable the flag atomically on the main side
282+
// Clear persistent web link token and disable the flag on the main side.
283+
// Flag is cleared first: a crash between the two writes leaves
284+
// persistentWebLink=false with a stale token, which the factory ignores.
280285
ipcMain.handle('live:clearPersistentToken', async () => {
281-
settingsStore.set('persistentWebLink', false);
282-
settingsStore.set('webAuthToken', null);
283-
logger.info('Cleared persistent web link token and disabled flag', 'WebServer');
284-
return { success: true };
286+
try {
287+
settingsStore.set('persistentWebLink', false);
288+
settingsStore.set('webAuthToken', null);
289+
logger.info('Cleared persistent web link token and disabled flag', 'WebServer');
290+
return { success: true };
291+
} catch (error: any) {
292+
logger.error(`Failed to clear persistent token: ${error.message}`, 'WebServer');
293+
return { success: false, message: error.message };
294+
}
285295
});
286296

287297
// Disable all live sessions and stop the server

src/main/stores/types.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,5 +159,9 @@ export interface AgentSessionOriginsData {
159159
/** Generic read/write store interface for settings */
160160
export interface SettingsStoreInterface {
161161
get<T>(key: string, defaultValue?: T): T;
162-
set(key: string, value: any): void;
162+
/** Type-safe set for known settings keys */
163+
set<K extends keyof MaestroSettings>(key: K, value: MaestroSettings[K]): void;
164+
/** Fallback for dynamic keys — used by the generic settings:set IPC handler
165+
* in persistence.ts which accepts arbitrary key/value pairs from the renderer */
166+
set(key: string, value: unknown): void;
163167
}

src/main/web-server/WebServer.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,13 +117,10 @@ export class WebServer {
117117
});
118118

119119
// Use provided token (persistent mode) or generate a new one (ephemeral mode)
120-
if (securityToken && securityToken.length > 0) {
120+
if (securityToken) {
121121
this.securityToken = securityToken;
122122
logger.debug('Using persistent security token', LOG_CONTEXT);
123123
} else {
124-
if (securityToken === '') {
125-
logger.warn('Empty security token provided, generating ephemeral token', LOG_CONTEXT);
126-
}
127124
this.securityToken = randomUUID();
128125
logger.debug('Security token generated', LOG_CONTEXT);
129126
}

src/main/web-server/web-server-factory.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,12 @@ import { getHistoryManager } from '../history-manager';
1111
import { logger } from '../utils/logger';
1212
import { isWebContentsAvailable } from '../utils/safe-send';
1313
import type { ProcessManager } from '../process-manager';
14-
import type { StoredSession } from '../stores/types';
14+
import type { StoredSession, SettingsStoreInterface as SettingsStore } from '../stores/types';
1515
import type { Group } from '../../shared/types';
1616

17-
import type { SettingsStoreInterface as SettingsStore } from '../stores/types';
17+
/** UUID v4 format regex for validating stored security tokens.
18+
* Enforces version nibble (4) and variant bits ([89ab]). */
19+
const UUID_V4_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
1820

1921
/** Store interface for sessions */
2022
interface SessionsStore {
@@ -63,8 +65,7 @@ export function createWebServerFactory(deps: WebServerFactoryDependencies) {
6365
if (persistentWebLink) {
6466
const storedToken = settingsStore.get<string | null>('webAuthToken', null);
6567
// Validate stored token is a proper UUID before trusting it
66-
const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
67-
if (storedToken && UUID_REGEX.test(storedToken)) {
68+
if (storedToken && UUID_V4_REGEX.test(storedToken)) {
6869
securityToken = storedToken;
6970
} else {
7071
if (storedToken) {

0 commit comments

Comments
 (0)