From f6c980db81c210d013bedb4e94cf681393fc1ec8 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Wed, 18 Mar 2026 18:27:05 -0500 Subject: [PATCH] fix(gsd): prevent concurrent dispatch during skip chains (#1272) (#1283) --- package-lock.json | 6 +- src/resources/extensions/gsd/auto.ts | 54 ++++++-- .../gsd/tests/auto-reentrancy-guard.test.ts | 127 ++++++++++++++++++ 3 files changed, 173 insertions(+), 14 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/auto-reentrancy-guard.test.ts diff --git a/package-lock.json b/package-lock.json index d59cdeed6..fa2e3d9a3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "gsd-pi", - "version": "2.28.0", + "version": "2.31.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "gsd-pi", - "version": "2.28.0", + "version": "2.31.2", "hasInstallScript": true, "license": "MIT", "workspaces": [ @@ -9101,7 +9101,7 @@ }, "packages/pi-coding-agent": { "name": "@gsd/pi-coding-agent", - "version": "0.57.1", + "version": "2.31.2", "dependencies": { "@mariozechner/jiti": "^2.6.2", "@silvia-odwyer/photon-node": "^0.3.4", diff --git a/src/resources/extensions/gsd/auto.ts b/src/resources/extensions/gsd/auto.ts index 3721052a6..dcb4881d8 100644 --- a/src/resources/extensions/gsd/auto.ts +++ b/src/resources/extensions/gsd/auto.ts @@ -833,6 +833,9 @@ export async function handleAgentEnd( // permanently stalled with no unit running and no watchdog set. if (s.pendingAgentEndRetry) { s.pendingAgentEndRetry = false; + // Clear gap watchdog from the previous cycle to prevent concurrent + // dispatch when the deferred handleAgentEnd calls dispatchNextUnit (#1272). + clearDispatchGapWatchdog(); setImmediate(() => { handleAgentEnd(ctx, pi).catch((err) => { const msg = err instanceof Error ? err.message : String(err); @@ -975,8 +978,12 @@ async function dispatchNextUnit( return; } - // Reentrancy guard - if (s.dispatching && s.skipDepth === 0) { + // Reentrancy guard — unconditional to prevent concurrent dispatch from + // gap watchdog or pendingAgentEndRetry during skip chains (#1272). + // Previously the guard was bypassed when skipDepth > 0, but the recursive + // skip chain's inner finally block resets s.dispatching = false before the + // outer call's finally runs, opening a window for concurrent entry. + if (s.dispatching) { debugLog("dispatchNextUnit reentrancy guard — another dispatch in progress, bailing"); return; } @@ -1448,8 +1455,12 @@ async function dispatchNextUnit( } if (dispatchResult.action !== "dispatch") { - await new Promise(r => setImmediate(r)); - await dispatchNextUnit(ctx, pi); + // Defer re-dispatch to next microtask so s.dispatching is released first, + // preventing reentrancy guard bypass during concurrent entry (#1272). + setImmediate(() => dispatchNextUnit(ctx, pi).catch(err => { + ctx.ui.notify(`Deferred dispatch failed: ${err instanceof Error ? err.message : String(err)}`, "error"); + pauseAuto(ctx, pi).catch(() => {}); + })); return; } @@ -1467,8 +1478,10 @@ async function dispatchNextUnit( } if (preDispatchResult.action === "skip") { ctx.ui.notify(`Skipping ${unitType} ${unitId} (pre-dispatch hook).`, "info"); - await new Promise(r => setImmediate(r)); - await dispatchNextUnit(ctx, pi); + setImmediate(() => dispatchNextUnit(ctx, pi).catch(err => { + ctx.ui.notify(`Deferred dispatch failed: ${err instanceof Error ? err.message : String(err)}`, "error"); + pauseAuto(ctx, pi).catch(() => {}); + })); return; } if (preDispatchResult.action === "replace") { @@ -1499,9 +1512,16 @@ async function dispatchNextUnit( if (idempotencyResult.reason === "completed" || idempotencyResult.reason === "fallback-persisted" || idempotencyResult.reason === "phantom-loop-cleared" || idempotencyResult.reason === "evicted") { if (!s.active) return; s.skipDepth++; - await new Promise(r => setTimeout(r, idempotencyResult.reason === "phantom-loop-cleared" ? 50 : 150)); - await dispatchNextUnit(ctx, pi); - s.skipDepth = Math.max(0, s.skipDepth - 1); + const skipDelay = idempotencyResult.reason === "phantom-loop-cleared" ? 50 : 150; + // Defer re-dispatch so s.dispatching is released first (#1272). + setTimeout(() => { + dispatchNextUnit(ctx, pi).catch(err => { + ctx.ui.notify(`Deferred skip-dispatch failed: ${err instanceof Error ? err.message : String(err)}`, "error"); + pauseAuto(ctx, pi).catch(() => {}); + }).finally(() => { + s.skipDepth = Math.max(0, s.skipDepth - 1); + }); + }, skipDelay); return; } } else if (idempotencyResult.action === "stop") { @@ -1532,8 +1552,11 @@ async function dispatchNextUnit( return; } if (stuckResult.action === "recovered" && stuckResult.dispatchAgain) { - await new Promise(r => setImmediate(r)); - await dispatchNextUnit(ctx, pi); + // Defer re-dispatch so s.dispatching is released first (#1272). + setImmediate(() => dispatchNextUnit(ctx, pi).catch(err => { + ctx.ui.notify(`Deferred recovery-dispatch failed: ${err instanceof Error ? err.message : String(err)}`, "error"); + pauseAuto(ctx, pi).catch(() => {}); + })); return; } @@ -1779,6 +1802,15 @@ export { export function _getUnitConsecutiveSkips(): Map { return s.unitConsecutiveSkips; } export function _resetUnitConsecutiveSkips(): void { s.unitConsecutiveSkips.clear(); } +/** + * Test-only: expose dispatching / skipDepth state for reentrancy guard tests. + * Not part of the public API. + */ +export function _getDispatching(): boolean { return s.dispatching; } +export function _setDispatching(v: boolean): void { s.dispatching = v; } +export function _getSkipDepth(): number { return s.skipDepth; } +export function _setSkipDepth(v: number): void { s.skipDepth = v; } + /** * Dispatch a hook unit directly, bypassing normal pre-dispatch hooks. * Used for manual hook triggers via /gsd run-hook. diff --git a/src/resources/extensions/gsd/tests/auto-reentrancy-guard.test.ts b/src/resources/extensions/gsd/tests/auto-reentrancy-guard.test.ts new file mode 100644 index 000000000..1cac097b9 --- /dev/null +++ b/src/resources/extensions/gsd/tests/auto-reentrancy-guard.test.ts @@ -0,0 +1,127 @@ +/** + * auto-reentrancy-guard.test.ts — Tests for the unconditional reentrancy guard. + * + * Regression for #1272: auto-mode stuck-loop where gap watchdog or + * pendingAgentEndRetry could enter dispatchNextUnit concurrently during + * recursive skip chains because the reentrancy guard was bypassed when + * skipDepth > 0. + * + * The fix makes the guard unconditional (`if (s.dispatching)` without + * `&& s.skipDepth === 0`), and defers recursive re-dispatch via + * setImmediate/setTimeout so s.dispatching is released first. + */ + +import { + _getDispatching, + _setDispatching, + _getSkipDepth, + _setSkipDepth, +} from "../auto.ts"; +import { createTestContext } from "./test-helpers.ts"; + +const { assertEq, assertTrue, report } = createTestContext(); + +async function main(): Promise { + // ─── Test-only accessors work ─────────────────────────────────────────── + console.log("\n=== reentrancy guard: test accessors round-trip ==="); + { + _setDispatching(false); + assertEq(_getDispatching(), false, "dispatching starts false"); + + _setDispatching(true); + assertEq(_getDispatching(), true, "dispatching set to true"); + + _setDispatching(false); + assertEq(_getDispatching(), false, "dispatching reset to false"); + } + + // ─── skipDepth accessors ──────────────────────────────────────────────── + console.log("\n=== reentrancy guard: skipDepth accessors round-trip ==="); + { + _setSkipDepth(0); + assertEq(_getSkipDepth(), 0, "skipDepth starts at 0"); + + _setSkipDepth(3); + assertEq(_getSkipDepth(), 3, "skipDepth set to 3"); + + _setSkipDepth(0); + assertEq(_getSkipDepth(), 0, "skipDepth reset to 0"); + } + + // ─── Guard blocks even when skipDepth > 0 (#1272 regression) ─────────── + console.log("\n=== reentrancy guard: blocks when dispatching=true regardless of skipDepth ==="); + { + // Simulate the scenario from #1272: dispatching=true + skipDepth>0 + // The old guard (`if (s.dispatching && s.skipDepth === 0)`) would allow + // concurrent entry when skipDepth > 0. The fix makes the check + // unconditional on skipDepth. + _setDispatching(true); + _setSkipDepth(2); + + // Verify dispatching is true — guard should block regardless of skipDepth + assertTrue( + _getDispatching() === true, + "dispatching flag is true during skip chain" + ); + + // The actual reentrancy guard in dispatchNextUnit checks: + // if (s.dispatching) { return; } + // We verify the state that would trigger the guard: + const wouldBlock = _getDispatching(); // unconditional check + const wouldBlockOld = _getDispatching() && _getSkipDepth() === 0; // old check + + assertTrue(wouldBlock === true, "new guard blocks when dispatching=true, skipDepth=2"); + assertTrue(wouldBlockOld === false, "old guard WOULD NOT block when dispatching=true, skipDepth=2 (the bug)"); + + // Clean up + _setDispatching(false); + _setSkipDepth(0); + } + + // ─── Guard allows entry when dispatching=false ────────────────────────── + console.log("\n=== reentrancy guard: allows entry when dispatching=false ==="); + { + _setDispatching(false); + _setSkipDepth(0); + assertTrue(!_getDispatching(), "guard allows entry when dispatching=false, skipDepth=0"); + + _setDispatching(false); + _setSkipDepth(3); + assertTrue(!_getDispatching(), "guard allows entry when dispatching=false, skipDepth=3"); + + _setSkipDepth(0); + } + + // ─── skipDepth does not affect guard decision (the fix) ───────────────── + console.log("\n=== reentrancy guard: skipDepth is irrelevant to guard decision ==="); + { + for (const depth of [0, 1, 2, 5]) { + _setDispatching(true); + _setSkipDepth(depth); + assertTrue( + _getDispatching() === true, + `guard blocks at skipDepth=${depth} when dispatching=true` + ); + } + + for (const depth of [0, 1, 2, 5]) { + _setDispatching(false); + _setSkipDepth(depth); + assertTrue( + _getDispatching() === false, + `guard allows at skipDepth=${depth} when dispatching=false` + ); + } + + // Clean up + _setDispatching(false); + _setSkipDepth(0); + } + + report(); +} + +main().catch((err) => { + console.error(err); + process.exit(1); +});