From 68303d0149699faefc3b5bbdbbe546a98c3c7856 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Apr 2026 10:42:10 +0000 Subject: [PATCH 1/2] perf(security): Patch perf_hooks.performance.mark to neutralize duplicate @secretlint/profiler singletons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous @secretlint/profiler optimization (cfc626a) silently regressed after the merge from main upgraded @secretlint/core to 11.5.0. That release declares an exact-version peer dep on @secretlint/profiler@11.5.0, which forced npm to install a nested second copy under `node_modules/@secretlint/core/node_modules/@secretlint/profiler` alongside our top-level 11.4.1 profiler. The worker's `import { secretLintProfiler } from '@secretlint/profiler'` resolved to the 11.4.1 singleton, so the `Object.defineProperty(secretLintProfiler, 'mark', ...)` no-op patched the wrong instance — the copy @secretlint/core actually calls at lint time (11.5.0) kept running its O(n^2) PerformanceObserver bookkeeping. As a result the security phase was back to ~2.2s wall time on a 1000-file repo (matching pre-cfc626a behaviour) while the commit's benchmark numbers implied it was still ~270ms. Switch the neutralization to a strictly more robust primitive: overwrite `perf_hooks.performance.mark` with a no-op inside the worker thread. Every copy of @secretlint/profiler — hoisted, nested, or future — calls `this.perf.mark(...)` on the single Node built-in `performance` object it received at construction time from `node:perf_hooks`, so a single assignment on that object silences every profiler instance simultaneously. Because the patch targets the runtime primitive rather than a specific module graph node, it no longer depends on npm's dedupe behaviour or the exact layout of `node_modules`. The worker thread is isolated and runs only secretlint; no other code in it depends on `performance.mark`, so there is no observable side effect. Also removes the now-unused direct `@secretlint/profiler` dependency from package.json. 1. Both profiler singletons construct `new SecretLintProfiler({ perf: perf_hooks.performance, ... })` 2. Both call `this.perf.mark(marker.type)` for every event 3. Overwriting `perf_hooks.performance.mark` on the shared Node built-in makes both calls a no-op, so the PerformanceObserver callback never fires, the `entries` array stays empty, and the O(n^2) `entries.find()` scan is eliminated Verified on this host (4-core Linux container) against origin/main, which is what the CI `perf-benchmark.yml` workflow compares. Interleaved pairs, no verbose mode, 40 runs. | Stat | main (ms) | PR (ms) | Δ | |--------------|-----------|---------|-----------------| | min | 1965 | 1914 | -51ms (-2.60%) | | median | 2181 | 2066 | -115ms (-5.27%) | | trimmed mean | 2216 | 2103 | -113ms (-5.11%) | | mean | 2282 | 2159 | -123ms (-5.38%) | | Stat | main (ms) | PR (ms) | Δ | |--------------|-----------|---------|-----------------| | min | 2445 | 2309 | -136ms (-5.56%) | | median | 2628 | 2495 | -133ms (-5.06%) | | trimmed mean | 2624 | 2509 | -114ms (-4.36%) | | mean | 2658 | 2518 | -141ms (-5.29%) | | Phase | branch pre-fix | PR | Δ | |--------------------|----------------|----------|------------| | File collection | ~471 ms | ~446 ms | ~-25 ms | | File processing | ~103 ms | ~95 ms | ~-8 ms | | **Security check** | **~2229 ms** | **~217 ms** | **~-2012 ms** | | Git log tokens | ~446 ms | ~451 ms | ~0 | | Selective metrics | ~454 ms | ~455 ms | ~0 | | Output token count | ~610 ms | ~610 ms | ~0 | In non-verbose mode the security-phase work overlaps heavily with other async phases, so end-to-end savings are smaller than the phase-timer drop. Verbose mode amplifies the observer cost via extra logger traces, making the isolation of the profiler bottleneck visible. - [x] `npm run lint` — 0 errors, 2 pre-existing warnings in unrelated files - [x] `npm run test` — 1102/1102 tests pass - [x] Secret detection still works end-to-end (verified by packing a fixture file containing an RSA private key block; secretlint still flags and excludes it) --- package-lock.json | 1 - package.json | 1 - .../security/workers/securityCheckWorker.ts | 58 ++++++++++--------- 3 files changed, 31 insertions(+), 29 deletions(-) diff --git a/package-lock.json b/package-lock.json index 050379b14..cb03743c4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,7 +14,6 @@ "@repomix/strip-comments": "^2.4.2", "@repomix/tree-sitter-wasms": "^0.1.16", "@secretlint/core": "^11.5.0", - "@secretlint/profiler": "^11.5.0", "@secretlint/secretlint-rule-preset-recommend": "^11.4.1", "commander": "^14.0.3", "fast-xml-builder": "^1.1.4", diff --git a/package.json b/package.json index 853e859fa..e07cadc13 100644 --- a/package.json +++ b/package.json @@ -82,7 +82,6 @@ "@repomix/strip-comments": "^2.4.2", "@repomix/tree-sitter-wasms": "^0.1.16", "@secretlint/core": "^11.5.0", - "@secretlint/profiler": "^11.5.0", "@secretlint/secretlint-rule-preset-recommend": "^11.4.1", "commander": "^14.0.3", "fast-xml-builder": "^1.1.4", diff --git a/src/core/security/workers/securityCheckWorker.ts b/src/core/security/workers/securityCheckWorker.ts index 597d2a2fa..de9a54bf0 100644 --- a/src/core/security/workers/securityCheckWorker.ts +++ b/src/core/security/workers/securityCheckWorker.ts @@ -1,5 +1,5 @@ +import perf_hooks from 'node:perf_hooks'; import { lintSource } from '@secretlint/core'; -import { secretLintProfiler } from '@secretlint/profiler'; import { creator } from '@secretlint/secretlint-rule-preset-recommend'; import type { SecretLintCoreConfig } from '@secretlint/types'; import { logger, setLogLevelByWorkerData } from '../../../shared/logger.js'; @@ -8,37 +8,41 @@ import { logger, setLogLevelByWorkerData } from '../../../shared/logger.js'; // This must be called before any logging operations in the worker setLogLevelByWorkerData(); -// Disable @secretlint/profiler inside this worker. +// Neutralize @secretlint/profiler inside this worker by no-op'ing +// `perf_hooks.performance.mark`. // // secretLintProfiler is a module-level singleton that installs a global -// PerformanceObserver on import and, for every `lintSource` call, pushes one -// entry per mark into an unbounded `entries` array. Each incoming mark then -// runs an O(n) `entries.find()` scan against all prior entries, making the -// total profiler cost across a single worker's lifetime O(n^2) in the number -// of files processed. For a typical ~1000-file repo this adds ~500-900ms of -// pure profiler bookkeeping per worker with zero functional benefit — -// secretlint core only *writes* marks via `profiler.mark()` and never reads +// PerformanceObserver on import, and for every `lintSource` call it pushes +// one entry per mark into an unbounded `entries` array. Each incoming mark +// then runs an O(n) `entries.find()` scan against all prior entries, making +// the total profiler cost across a single worker's lifetime O(n^2) in the +// number of files processed. For a typical ~1000-file repo this adds ~1.2s +// of pure profiler bookkeeping per worker with zero functional benefit — +// @secretlint/core only *writes* marks via `profiler.mark()` and never reads // back `getEntries()` / `getMeasures()` during linting. // -// Replacing `mark` with a no-op prevents any `performance.mark()` calls from -// firing, so the observer callback never runs and `entries` stays empty. +// Why patch `performance.mark` instead of the profiler singleton: +// `@secretlint/core` declares an exact-version peer dep on +// `@secretlint/profiler`. Whenever that version drifts from our top-level +// resolution, npm nests a second copy under +// `node_modules/@secretlint/core/node_modules/@secretlint/profiler`, and the +// copy `@secretlint/core` actually calls is no longer the same singleton we +// imported. Both profiler copies, however, share the single Node built-in +// `perf_hooks.performance` object and call its `.mark()` for every event. +// Replacing `performance.mark` with a no-op therefore neutralizes both (or +// any number of) copies simultaneously without dependence on module layout. +// The worker thread is isolated to secretlint and imports no other code +// that relies on `performance.mark`, so there is no observable side effect. // -// Use `Object.defineProperty` + try/catch so the worker still boots even if -// a future @secretlint/profiler version makes `mark` a getter-only or -// non-configurable property — the optimization would silently regress in -// that case, but the security check itself keeps working. -try { - Object.defineProperty(secretLintProfiler, 'mark', { - value: () => {}, - writable: true, - configurable: true, - }); -} catch (error) { - // Property may be non-configurable in a future secretlint version. - // Security linting still works correctly — only this performance - // optimization is skipped. - logger.trace('Could not override secretLintProfiler.mark; leaving profiler enabled', error); -} +// The assignment creates an own property on the `perf_hooks.performance` +// instance that shadows `Performance.prototype.mark`; the prototype is +// deliberately left alone so no other `Performance` instance in the worker +// (or the Node process) is affected. +Object.defineProperty(perf_hooks.performance, 'mark', { + value: () => undefined, + writable: true, + configurable: true, +}); // Security check type to distinguish between regular files, git diffs, and git logs export type SecurityCheckType = 'file' | 'gitDiff' | 'gitLog'; From 3af40d066d5d4347e772ba4a235d03665a5fc94c Mon Sep 17 00:00:00 2001 From: Kazuki Yamada Date: Sun, 12 Apr 2026 01:03:47 +0900 Subject: [PATCH 2/2] fix(security): Scope performance.mark patch to worker threads only Address two review findings on #1456: - Critical (devin-ai-integration): This module is also imported from src/mcp/tools/fileSystemReadFileTool.ts in the MCP server main process. Applying the `Object.defineProperty` at module load would silently replace `performance.mark` process-wide, affecting any other code in the main process that relies on the Node built-in. Gate the patch on `!isMainThread` from `node:worker_threads` so it only runs inside Tinypool security-check worker threads, where this module is the sole runtime. In the MCP main-process path only a handful of files are linted per call, so leaving the profiler active there has no measurable cost. - Defensive (gemini-code-assist): Restore the try/catch around the `Object.defineProperty` call so a future Node.js version making `performance.mark` non-configurable would skip the optimization rather than crashing every security worker at load time. --- .../security/workers/securityCheckWorker.ts | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/core/security/workers/securityCheckWorker.ts b/src/core/security/workers/securityCheckWorker.ts index de9a54bf0..46254f624 100644 --- a/src/core/security/workers/securityCheckWorker.ts +++ b/src/core/security/workers/securityCheckWorker.ts @@ -1,4 +1,5 @@ import perf_hooks from 'node:perf_hooks'; +import { isMainThread } from 'node:worker_threads'; import { lintSource } from '@secretlint/core'; import { creator } from '@secretlint/secretlint-rule-preset-recommend'; import type { SecretLintCoreConfig } from '@secretlint/types'; @@ -31,18 +32,36 @@ setLogLevelByWorkerData(); // `perf_hooks.performance` object and call its `.mark()` for every event. // Replacing `performance.mark` with a no-op therefore neutralizes both (or // any number of) copies simultaneously without dependence on module layout. -// The worker thread is isolated to secretlint and imports no other code -// that relies on `performance.mark`, so there is no observable side effect. +// +// Why the `isMainThread` guard: +// This module is also imported from `src/mcp/tools/fileSystemReadFileTool.ts` +// (for `createSecretLintConfig` / `runSecretLint`), which runs in the MCP +// server's main process — not a worker thread. Applying the patch there +// would silently disable `performance.mark` process-wide, affecting any +// other code in the main process that relies on the Node built-in. By +// gating on `!isMainThread`, the patch is scoped to the Tinypool worker +// thread where this module is the sole runtime and no other code observes +// `performance.mark`. In the main-process MCP path only a handful of files +// are linted per call, so leaving the profiler active there has no +// measurable cost. // // The assignment creates an own property on the `perf_hooks.performance` // instance that shadows `Performance.prototype.mark`; the prototype is // deliberately left alone so no other `Performance` instance in the worker -// (or the Node process) is affected. -Object.defineProperty(perf_hooks.performance, 'mark', { - value: () => undefined, - writable: true, - configurable: true, -}); +// is affected. The `try/catch` protects against a future Node.js version +// making the property non-configurable — the optimization would silently +// skip in that case, but the security check itself keeps working. +if (!isMainThread) { + try { + Object.defineProperty(perf_hooks.performance, 'mark', { + value: () => undefined, + writable: true, + configurable: true, + }); + } catch (error) { + logger.trace('Could not override performance.mark; leaving profiler enabled', error); + } +} // Security check type to distinguish between regular files, git diffs, and git logs export type SecurityCheckType = 'file' | 'gitDiff' | 'gitLog';