mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-11 22:53:04 +00:00
perf(lock): probe this process's own start time once (#3222)
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
* 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) <noreply@anthropic.com> * chore(autofix): apply prettier + eslint fixes via /autofix command --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This commit is contained in:
parent
18cbeb907c
commit
376ed3bb4a
4 changed files with 71 additions and 5 deletions
|
|
@ -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<AutoSyncWatchControlDeps> = {}): AutoSyn
|
|||
return undefined;
|
||||
}
|
||||
}),
|
||||
readProcessStartTime: deps.readProcessStartTime ?? readProcessStartTime,
|
||||
readProcessStartTime: deps.readProcessStartTime ?? readProcessStartTimeCached,
|
||||
sleep:
|
||||
deps.sleep ??
|
||||
((ms) =>
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue