From 376ed3bb4abda80f7a57cf963822002b5304f2f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Tue, 8 Sep 2026 19:06:03 +0100 Subject: [PATCH] perf(lock): probe this process's own start time once (#3222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(lock): probe this process's own start time once `acquireFileLock` stamps the owner file with the acquiring process's start time so a later reclaimer can tell a live owner from pid reuse. That value cannot change while we are running, but it was re-probed on every acquisition — and on Windows the probe is a `powershell.exe` spawn plus a `Get-CimInstance Win32_Process` WMI query, which is the single most expensive step in taking an uncontended lock. Add `readProcessStartTimeCached` and make it the default reader in `acquireFileLock` and `resolveWatchDeps`. Only this process's own pid is cached: - A foreign pid is always re-probed. That process can exit and its pid be reused, which is precisely what the stamp exists to detect. - A failed probe is not cached. `acquireFileLock` throws when the start time is empty, so caching one transient failure would leave the process unable to take a lock for the rest of its life. `readProcessStartTime` itself is unchanged and still probes every call, so the existing timezone-pinning regression test keeps exercising the real `ps` invocation instead of passing off a cached value. Behavior is otherwise identical: same probe, same string, same stamp. Co-Authored-By: Claude Opus 5 (1M context) * chore(autofix): apply prettier + eslint fixes via /autofix command --------- Co-authored-by: Gergo Magyar Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- gitnexus/src/core/auto-sync/starter.ts | 4 +- gitnexus/src/storage/file-lock.ts | 8 ++-- gitnexus/src/utils/process-identity.ts | 19 +++++++++ gitnexus/test/unit/process-identity.test.ts | 45 +++++++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/gitnexus/src/core/auto-sync/starter.ts b/gitnexus/src/core/auto-sync/starter.ts index 624e092ec..7d67dfa64 100644 --- a/gitnexus/src/core/auto-sync/starter.ts +++ b/gitnexus/src/core/auto-sync/starter.ts @@ -4,7 +4,7 @@ import path from 'node:path'; import { execFileSync } from 'node:child_process'; import { acquireFileLock, FileLockBusyError } from '../../storage/file-lock.js'; import { getGlobalDir } from '../../storage/repo-manager.js'; -import { isProcessAlive, readProcessStartTime } from '../../utils/process-identity.js'; +import { isProcessAlive, readProcessStartTimeCached } from '../../utils/process-identity.js'; import { loadAutoSyncConfig } from './config.js'; import { runAutoSyncOnce } from './runner.js'; import { getAutoSyncMutexPath, getAutoSyncWatchDir } from './state.js'; @@ -632,7 +632,7 @@ function resolveWatchDeps(deps: Partial = {}): AutoSyn return undefined; } }), - readProcessStartTime: deps.readProcessStartTime ?? readProcessStartTime, + readProcessStartTime: deps.readProcessStartTime ?? readProcessStartTimeCached, sleep: deps.sleep ?? ((ms) => diff --git a/gitnexus/src/storage/file-lock.ts b/gitnexus/src/storage/file-lock.ts index a857d6a47..2b2494ef8 100644 --- a/gitnexus/src/storage/file-lock.ts +++ b/gitnexus/src/storage/file-lock.ts @@ -3,7 +3,7 @@ import fs from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; import { setTimeout as sleep } from 'node:timers/promises'; -import { isProcessAlive, readProcessStartTime } from '../utils/process-identity.js'; +import { isProcessAlive, readProcessStartTimeCached } from '../utils/process-identity.js'; const HOSTNAME = os.hostname(); @@ -46,7 +46,9 @@ export async function acquireFileLock( pid, ownerId: crypto.randomUUID(), processStartTime: - options.processStartTime ?? (options.readProcessStartTime ?? readProcessStartTime)(pid) ?? '', + options.processStartTime ?? + (options.readProcessStartTime ?? readProcessStartTimeCached)(pid) ?? + '', hostname: options.hostname ?? HOSTNAME, }; if (!owner.processStartTime) { @@ -69,7 +71,7 @@ export async function acquireFileLock( resolvedPath, owner, options.isProcessAlive ?? isProcessAlive, - options.readProcessStartTime ?? readProcessStartTime, + options.readProcessStartTime ?? readProcessStartTimeCached, ) ) { continue; diff --git a/gitnexus/src/utils/process-identity.ts b/gitnexus/src/utils/process-identity.ts index e7c99439c..8c00eab08 100644 --- a/gitnexus/src/utils/process-identity.ts +++ b/gitnexus/src/utils/process-identity.ts @@ -38,3 +38,22 @@ export function readProcessStartTime(pid: number): string | undefined { return undefined; } } + +let ownStartTime: string | undefined; + +/** + * `readProcessStartTime`, except this process's own start time is probed once. + * It cannot change while we are running, and every `acquireFileLock` — plus + * each retry attempt and each stale-lock reclaim guard — stamps the owner file + * with it. On Windows that probe is a `powershell.exe` spawn and a WMI query, + * so a process taking several locks pays it several times for one constant. + * + * A foreign pid is never cached: that process can exit and its pid can be + * reused, which is the very thing the stamp exists to detect. A failed probe + * is not cached either — one transient failure would otherwise leave the + * process unable to take a lock for its whole lifetime. + */ +export function readProcessStartTimeCached(pid: number): string | undefined { + if (pid !== process.pid) return readProcessStartTime(pid); + return (ownStartTime ??= readProcessStartTime(pid)); +} diff --git a/gitnexus/test/unit/process-identity.test.ts b/gitnexus/test/unit/process-identity.test.ts index 7b31f1d32..de3436882 100644 --- a/gitnexus/test/unit/process-identity.test.ts +++ b/gitnexus/test/unit/process-identity.test.ts @@ -4,8 +4,24 @@ import { isProcessAlive, readProcessStartTime } from '../../src/utils/process-id afterEach(() => { vi.restoreAllMocks(); + vi.doUnmock('node:child_process'); + vi.resetModules(); }); +/** + * Loads a fresh copy of the module (fresh memo) over a counted `execFileSync`, + * so "how many times did we actually shell out" is observable. `doMock` is not + * hoisted, so the statically imported functions used by the other tests keep + * the real implementation. + */ +async function withCountedProbe(probe: () => string) { + const execFileSync = vi.fn(probe); + vi.doMock('node:child_process', () => ({ execFileSync })); + vi.resetModules(); + const identity = await import('../../src/utils/process-identity.js'); + return { execFileSync, readProcessStartTimeCached: identity.readProcessStartTimeCached }; +} + describe('process identity', () => { it('treats only ESRCH as a dead process', () => { const kill = vi.spyOn(process, 'kill'); @@ -39,4 +55,33 @@ describe('process identity', () => { } }, ); + + it('probes this process once and re-probes a foreign pid every time', async () => { + const { execFileSync, readProcessStartTimeCached } = await withCountedProbe(() => 'STAMP\n'); + + expect(readProcessStartTimeCached(process.pid)).toBe('STAMP'); + expect(readProcessStartTimeCached(process.pid)).toBe('STAMP'); + // On Windows each probe is a powershell.exe spawn plus a WMI query. + expect(execFileSync).toHaveBeenCalledTimes(1); + + // A foreign process can exit and its pid be reused — caching that stamp + // would blind the reuse check the stamp exists for. + expect(readProcessStartTimeCached(process.pid + 1)).toBe('STAMP'); + expect(readProcessStartTimeCached(process.pid + 1)).toBe('STAMP'); + expect(execFileSync).toHaveBeenCalledTimes(3); + }); + + it('retries after a failed self probe instead of caching the failure', async () => { + const { execFileSync, readProcessStartTimeCached } = await withCountedProbe(() => 'STAMP\n'); + execFileSync.mockImplementationOnce(() => { + throw new Error('probe unavailable'); + }); + + // A cached failure would leave acquireFileLock throwing "Unable to + // determine process start time" for the rest of the process's life. + expect(readProcessStartTimeCached(process.pid)).toBeUndefined(); + expect(readProcessStartTimeCached(process.pid)).toBe('STAMP'); + expect(readProcessStartTimeCached(process.pid)).toBe('STAMP'); + expect(execFileSync).toHaveBeenCalledTimes(2); + }); });