From 333528843d688b33db0604205c3965eb7f7f2e0b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 23 May 2026 16:22:31 -0400 Subject: [PATCH] fix(7): raise concurrency-safety roadmap-analyze budget to 5000ms (Mac flake mitigation) (#157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(7): raise concurrency-safety roadmap-analyze budget 2000ms → 5000ms The 50-phase roadmap-analyze wall-clock test consistently flaked on Mac under realistic load (empirical floor ~2100-3000ms), independently reproduced by three fixers on PRs #3795, #3792, #3799. Extract the magic 2000 to named constant ROADMAP_ANALYZE_BUDGET_MS with an explanatory comment. Raise to 5000ms (2.5x observed worst-case) — large enough to eliminate Mac flakes without masking real regressions. Long-term: convert to behavior-anchored assertion per PR #3803 pattern; tracked as follow-up, out of scope here. Acceptance criteria from #7: > "Replace hardcoded 2000ms with higher budget (e.g., 5000ms) or make > configurable; add comment documenting rationale; confirm no > false-negative on healthy machine." Co-Authored-By: Claude Sonnet 4.6 * fix(7): guard executeForCjs with try/catch fallback in all CJS routers When the synckit bridge throws (worker crash, Atomics failure on Windows/Node 24, or any other transient OS-level failure), the exception propagated uncaught through the sdkHandler closure up to Node.js's unhandled-rejection handler. On Windows, the async stderr write for the rejection message may not flush before process exit, producing an empty- stderr non-zero exit that manifests as 'init failed: Command failed: ...' in workspace.test.cjs. Wrap getExecuteForCjs() in a try/catch in every CJS router that has this pattern (gsd-tools.cjs _dispatchNonFamily, init-, roadmap-, state-, validate-, verify-command-router.cjs). On catch: fall through to the CJS handler, which is the designed safety net for bridge failures and produces identical output. Verified: workspace.test.cjs (26/26), concurrency-safety.test.cjs (31/31), cjs-sdk-bridge-integration.test.cjs (4/4), and bug-3631-router-raw-flag.test.cjs (2/2) all pass locally on Mac. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/bin/gsd-tools.cjs | 33 +++++++++++-------- get-shit-done/bin/lib/init-command-router.cjs | 33 +++++++++++-------- .../bin/lib/roadmap-command-router.cjs | 33 +++++++++++-------- .../bin/lib/state-command-router.cjs | 33 +++++++++++-------- .../bin/lib/validate-command-router.cjs | 33 +++++++++++-------- .../bin/lib/verify-command-router.cjs | 33 +++++++++++-------- tests/concurrency-safety.test.cjs | 8 ++++- 7 files changed, 127 insertions(+), 79 deletions(-) diff --git a/get-shit-done/bin/gsd-tools.cjs b/get-shit-done/bin/gsd-tools.cjs index 785e07ca3..85940a2bb 100755 --- a/get-shit-done/bin/gsd-tools.cjs +++ b/get-shit-done/bin/gsd-tools.cjs @@ -232,19 +232,26 @@ const { tryLoadSdk: _tryLoadSdkBridge, getExecuteForCjs } = require('./lib/cjs-s */ function _dispatchNonFamily({ registryCommand, registryArgs, legacyCommand, legacyArgs, cwd, raw, error, output }) { if (!_tryLoadSdkBridge()) return false; - const result = getExecuteForCjs()({ - registryCommand, - registryArgs, - legacyCommand, - legacyArgs, - // Always request typed JSON from the bridge; CJS `output(data, raw)` handles - // user-facing rendering. Passing `mode: 'raw'` would make the bridge - // pre-render result.data to a JSON string that the CJS output path then - // double-stringifies (returning a JSON string of a JSON string). - mode: 'json', - projectDir: cwd, - workstream: process.env.GSD_WORKSTREAM || undefined, - }); + let result; + try { + result = getExecuteForCjs()({ + registryCommand, + registryArgs, + legacyCommand, + legacyArgs, + // Always request typed JSON from the bridge; CJS `output(data, raw)` handles + // user-facing rendering. Passing `mode: 'raw'` would make the bridge + // pre-render result.data to a JSON string that the CJS output path then + // double-stringifies (returning a JSON string of a JSON string). + mode: 'json', + projectDir: cwd, + workstream: process.env.GSD_WORKSTREAM || undefined, + }); + } catch { + // Bridge threw (e.g. synckit worker crash, Atomics failure on Windows). + // Return false so the caller falls through to the CJS handler. + return false; + } if (!result.ok) { const message = (result.errorDetails && result.errorDetails.message) || `${legacyCommand} (${registryCommand}) failed (${result.errorKind})`; diff --git a/get-shit-done/bin/lib/init-command-router.cjs b/get-shit-done/bin/lib/init-command-router.cjs index ec21ebfd3..1d55770ef 100644 --- a/get-shit-done/bin/lib/init-command-router.cjs +++ b/get-shit-done/bin/lib/init-command-router.cjs @@ -26,19 +26,26 @@ function routeInitCommand({ init, args, cwd, raw, parseNamedArgs, error }) { function sdkHandler(registryCommand, registryArgs, legacyArgs, cjsFallback) { if (!sdkAvailable) return cjsFallback; return () => { - const result = getExecuteForCjs()({ - registryCommand, - registryArgs, - legacyCommand: 'init', - legacyArgs, - // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's - // raw projection (formatQueryRawOutput) and returns the scalar string - // CJS callers used to print. We then bypass output()'s JSON-stringify - // path by passing rawValue (the third positional). With mode:'json', - // output() emits the JSON IR as before. - mode: raw ? 'raw' : 'json', - projectDir: cwd, - }); + let result; + try { + result = getExecuteForCjs()({ + registryCommand, + registryArgs, + legacyCommand: 'init', + legacyArgs, + // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's + // raw projection (formatQueryRawOutput) and returns the scalar string + // CJS callers used to print. We then bypass output()'s JSON-stringify + // path by passing rawValue (the third positional). With mode:'json', + // output() emits the JSON IR as before. + mode: raw ? 'raw' : 'json', + projectDir: cwd, + }); + } catch { + // Bridge threw (e.g. synckit worker crash, Atomics failure on Windows). + // Fall through to CJS handler — the CJS path is the designed safety net. + return cjsFallback(); + } if (!result.ok) { error(result.errorDetails && result.errorDetails.message ? result.errorDetails.message diff --git a/get-shit-done/bin/lib/roadmap-command-router.cjs b/get-shit-done/bin/lib/roadmap-command-router.cjs index 7f8427f3c..c7ab33a28 100644 --- a/get-shit-done/bin/lib/roadmap-command-router.cjs +++ b/get-shit-done/bin/lib/roadmap-command-router.cjs @@ -32,19 +32,26 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }) { function sdkHandler(registryCommand, registryArgs, legacyArgs, cjsFallback) { if (!sdkAvailable) return cjsFallback; return () => { - const result = getExecuteForCjs()({ - registryCommand, - registryArgs, - legacyCommand: 'roadmap', - legacyArgs, - // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's - // raw projection (formatQueryRawOutput) and returns the scalar string - // CJS callers used to print. We then bypass output()'s JSON-stringify - // path by passing rawValue (the third positional). With mode:'json', - // output() emits the JSON IR as before. - mode: raw ? 'raw' : 'json', - projectDir: cwd, - }); + let result; + try { + result = getExecuteForCjs()({ + registryCommand, + registryArgs, + legacyCommand: 'roadmap', + legacyArgs, + // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's + // raw projection (formatQueryRawOutput) and returns the scalar string + // CJS callers used to print. We then bypass output()'s JSON-stringify + // path by passing rawValue (the third positional). With mode:'json', + // output() emits the JSON IR as before. + mode: raw ? 'raw' : 'json', + projectDir: cwd, + }); + } catch { + // Bridge threw (e.g. synckit worker crash, Atomics failure on Windows). + // Fall through to CJS handler — the CJS path is the designed safety net. + return cjsFallback(); + } if (!result.ok) { error(result.errorDetails && result.errorDetails.message ? result.errorDetails.message diff --git a/get-shit-done/bin/lib/state-command-router.cjs b/get-shit-done/bin/lib/state-command-router.cjs index 971b25b76..ff7b3829f 100644 --- a/get-shit-done/bin/lib/state-command-router.cjs +++ b/get-shit-done/bin/lib/state-command-router.cjs @@ -63,19 +63,26 @@ function dispatchViaSdk(registryCommand, registryArgs, legacyArgs, cwd, raw, err // honor the user's --raw flag and let the bridge do default rendering. const bridgeMode = rawFormatter ? 'json' : (raw ? 'raw' : 'json'); - const result = getExecuteForCjs()({ - registryCommand, - registryArgs, - legacyCommand: 'state', - legacyArgs, - mode: bridgeMode, - projectDir: cwd, - // Phase 6 fix: workstream is now threaded through to the native handler. - // GSDTransport no longer forces subprocess for workstream-scoped requests — - // the worker's dispatchNative closure correctly passes workstream to - // registry.dispatch() (Phase 5.1 fix), enabling native workstream dispatch. - workstream: process.env.GSD_WORKSTREAM || undefined, - }); + let result; + try { + result = getExecuteForCjs()({ + registryCommand, + registryArgs, + legacyCommand: 'state', + legacyArgs, + mode: bridgeMode, + projectDir: cwd, + // Phase 6 fix: workstream is now threaded through to the native handler. + // GSDTransport no longer forces subprocess for workstream-scoped requests — + // the worker's dispatchNative closure correctly passes workstream to + // registry.dispatch() (Phase 5.1 fix), enabling native workstream dispatch. + workstream: process.env.GSD_WORKSTREAM || undefined, + }); + } catch { + // Bridge threw (e.g. synckit worker crash, Atomics failure on Windows). + // Return false so the caller falls through to the CJS handler. + return false; + } if (!result.ok) { // Mutation subcommands whose CJS contract is always exit-0: surface the SDK diff --git a/get-shit-done/bin/lib/validate-command-router.cjs b/get-shit-done/bin/lib/validate-command-router.cjs index e38bd8333..60fc61ae4 100644 --- a/get-shit-done/bin/lib/validate-command-router.cjs +++ b/get-shit-done/bin/lib/validate-command-router.cjs @@ -31,19 +31,26 @@ function routeValidateCommand({ verify, args, cwd, raw, parseNamedArgs, output: function sdkHandler(registryCommand, registryArgs, legacyArgs, cjsFallback) { if (!sdkAvailable) return cjsFallback; return () => { - const result = getExecuteForCjs()({ - registryCommand, - registryArgs, - legacyCommand: 'validate', - legacyArgs, - // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's - // raw projection (formatQueryRawOutput) and returns the scalar string - // CJS callers used to print. We then bypass output()'s JSON-stringify - // path by passing rawValue (the third positional). With mode:'json', - // output() emits the JSON IR as before. - mode: raw ? 'raw' : 'json', - projectDir: cwd, - }); + let result; + try { + result = getExecuteForCjs()({ + registryCommand, + registryArgs, + legacyCommand: 'validate', + legacyArgs, + // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's + // raw projection (formatQueryRawOutput) and returns the scalar string + // CJS callers used to print. We then bypass output()'s JSON-stringify + // path by passing rawValue (the third positional). With mode:'json', + // output() emits the JSON IR as before. + mode: raw ? 'raw' : 'json', + projectDir: cwd, + }); + } catch { + // Bridge threw (e.g. synckit worker crash, Atomics failure on Windows). + // Fall through to CJS handler — the CJS path is the designed safety net. + return cjsFallback(); + } if (!result.ok) { error(result.errorDetails && result.errorDetails.message ? result.errorDetails.message diff --git a/get-shit-done/bin/lib/verify-command-router.cjs b/get-shit-done/bin/lib/verify-command-router.cjs index e42809f54..c581b830a 100644 --- a/get-shit-done/bin/lib/verify-command-router.cjs +++ b/get-shit-done/bin/lib/verify-command-router.cjs @@ -26,19 +26,26 @@ function routeVerifyCommand({ verify, args, cwd, raw, error }) { function sdkHandler(registryCommand, registryArgs, legacyArgs, cjsFallback) { if (!sdkAvailable) return cjsFallback; return () => { - const result = getExecuteForCjs()({ - registryCommand, - registryArgs, - legacyCommand: 'verify', - legacyArgs, - // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's - // raw projection (formatQueryRawOutput) and returns the scalar string - // CJS callers used to print. We then bypass output()'s JSON-stringify - // path by passing rawValue (the third positional). With mode:'json', - // output() emits the JSON IR as before. - mode: raw ? 'raw' : 'json', - projectDir: cwd, - }); + let result; + try { + result = getExecuteForCjs()({ + registryCommand, + registryArgs, + legacyCommand: 'verify', + legacyArgs, + // #3631: under --raw, request mode:'raw' so the bridge runs the SDK's + // raw projection (formatQueryRawOutput) and returns the scalar string + // CJS callers used to print. We then bypass output()'s JSON-stringify + // path by passing rawValue (the third positional). With mode:'json', + // output() emits the JSON IR as before. + mode: raw ? 'raw' : 'json', + projectDir: cwd, + }); + } catch { + // Bridge threw (e.g. synckit worker crash, Atomics failure on Windows). + // Fall through to CJS handler — the CJS path is the designed safety net. + return cjsFallback(); + } if (!result.ok) { error(result.errorDetails && result.errorDetails.message ? result.errorDetails.message diff --git a/tests/concurrency-safety.test.cjs b/tests/concurrency-safety.test.cjs index 39a32e899..f17c51532 100644 --- a/tests/concurrency-safety.test.cjs +++ b/tests/concurrency-safety.test.cjs @@ -785,6 +785,12 @@ describe('stress tests with 50+ phases', () => { cleanup(tmpDir); }); + // Wall-clock budget for the 50-phase ROADMAP analyze. + // Empirical floor on Mac under realistic load is ~2100-3000ms; bumped from + // 2000ms to 5000ms after three independent flake reports (see #7). + // Long-term: convert to behavior-anchored assertion per PR #3803 pattern. + const ROADMAP_ANALYZE_BUDGET_MS = 5000; + test('roadmap analyze on 50-phase ROADMAP completes in under 2000ms', () => { create50PhaseProject(tmpDir, 25); @@ -793,7 +799,7 @@ describe('stress tests with 50+ phases', () => { const elapsed = performance.now() - start; assert.ok(result.success, `roadmap analyze should succeed: ${result.error}`); - assert.ok(elapsed < 2000, `Should complete in under 2000ms, took ${elapsed.toFixed(0)}ms`); + assert.ok(elapsed < ROADMAP_ANALYZE_BUDGET_MS, `Should complete in under ${ROADMAP_ANALYZE_BUDGET_MS}ms, took ${elapsed.toFixed(0)}ms`); const output = JSON.parse(result.output); assert.ok(Array.isArray(output.phases), 'Output should contain a phases array');