diff --git a/skills/continuous-learning-v2/hooks/observe.sh b/skills/continuous-learning-v2/hooks/observe.sh index bf800e772..fafb57a62 100755 --- a/skills/continuous-learning-v2/hooks/observe.sh +++ b/skills/continuous-learning-v2/hooks/observe.sh @@ -569,7 +569,10 @@ fi # which caused runaway parallel Claude analysis processes. SIGNAL_EVERY_N="${ECC_OBSERVER_SIGNAL_EVERY_N:-20}" SIGNAL_COUNTER_FILE="${PROJECT_DIR}/.observer-signal-counter" -SIGNAL_COUNTER_LOCK="${SIGNAL_COUNTER_FILE}.lock" +# Use a regular-file OS lock. Keep legacy mkdir locks untouched: a killed +# Windows process cannot run shell traps, and reclaiming those directories can +# race a live owner. The versioned file makes a stranded directory irrelevant. +SIGNAL_COUNTER_LOCK="${SIGNAL_COUNTER_FILE}.lock.v2" ACTIVITY_FILE="${PROJECT_DIR}/.observer-last-activity" touch "$ACTIVITY_FILE" 2>/dev/null || true @@ -578,9 +581,7 @@ touch "$ACTIVITY_FILE" 2>/dev/null || true # tool call (which can fire every second), so concurrent invocations previously # raced on this counter: both read the same value, both incremented, and one # write was lost, signaling the observer at unpredictable intervals (#2296). -# Prefer flock (a kernel advisory lock the OS releases automatically if the hook -# is killed); fall back to the atomic mkdir lock this script already uses for -# the lazy-start path above. Both wrap the same read-modify-write below. +# Prefer flock; Python supplies the same OS-released lock on macOS and Windows. should_signal=0 _clv2_bump_signal_counter() { @@ -613,38 +614,61 @@ if command -v flock >/dev/null 2>&1 && exec 8>"$SIGNAL_COUNTER_LOCK" 2>/dev/null fi exec 8>&- 2>/dev/null || true else - # No flock (e.g. macOS): atomic mkdir lock with a bounded spin so the hook - # never blocks indefinitely. A trap releases the lock on every exit path -- - # including the async-timeout SIGTERM -- so a killed hook does not strand the - # directory. We deliberately do NOT hand-roll PID-based stale reclaim: - # re-verifying then removing another process's lock is racy and can delete a - # live re-acquirer's directory, reintroducing the very race this fixes. - _signal_lock_held=0 - _signal_lock_spins=0 - while [ "$_signal_lock_spins" -lt 100 ]; do - if mkdir "$SIGNAL_COUNTER_LOCK" 2>/dev/null; then - # EXIT cleans up on normal completion. INT/TERM must release AND exit: - # a signal trap that only released the lock would otherwise fall through - # and continue the read-modify-write without ownership. - trap 'rmdir "$SIGNAL_COUNTER_LOCK" 2>/dev/null || true' EXIT - trap 'rmdir "$SIGNAL_COUNTER_LOCK" 2>/dev/null || true; exit 130' INT - trap 'rmdir "$SIGNAL_COUNTER_LOCK" 2>/dev/null || true; exit 143' TERM - _signal_lock_held=1 - break - fi - _signal_lock_spins=$((_signal_lock_spins + 1)) - sleep 0.02 - done - if [ "$_signal_lock_held" -eq 1 ]; then - # Bump only under the held lock -- never an unlocked read-modify-write. - _clv2_bump_signal_counter - rmdir "$SIGNAL_COUNTER_LOCK" 2>/dev/null || true - trap - EXIT INT TERM - fi - # If the lock could not be acquired within the spin budget we skip this tick - # rather than racing on an unlocked counter. Dropping one throttle tick under - # extreme contention only delays the next observer signal slightly; it never - # corrupts the counter or signals spuriously. + # One interpreter, monotonic elapsed-time wait, no MSYS2 sleep subprocesses. + # Both fcntl and msvcrt release ownership even on SIGKILL/TerminateProcess. + # If locking is unavailable or busy, skip the optional tick without an + # unlocked update. Observation capture above has already completed. + should_signal=$("$PYTHON_CMD" - "$SIGNAL_COUNTER_LOCK" "$SIGNAL_COUNTER_FILE" "$SIGNAL_EVERY_N" <<'PY' +import os +import re +import sys +import time + +try: + if os.name == "nt": + import msvcrt + # Git Bash passes /c/... paths to native Windows Python unchanged. + for index in (1, 2): + sys.argv[index] = re.sub(r"^/([A-Za-z])/", r"\1:/", sys.argv[index]) + def acquire(handle): + handle.seek(0) + msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) + else: + import fcntl + def acquire(handle): + fcntl.flock(handle.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + + with open(sys.argv[1], "a+b") as guard: + if os.name == "nt" and os.fstat(guard.fileno()).st_size == 0: + guard.write(b"0") + guard.flush() + deadline = time.monotonic() + 2.0 + while True: + try: + acquire(guard) + break + except OSError: + if time.monotonic() >= deadline: + print(0) + sys.exit(0) + time.sleep(0.02) + try: + with open(sys.argv[2], encoding="ascii") as source: + value = source.read().strip() + counter = int(value) if value and value.isascii() and value.isdigit() else 0 + counter += 1 + signal = int(counter >= int(sys.argv[3])) + if signal: + counter = 0 + except FileNotFoundError: + counter, signal = 1, 0 + with open(sys.argv[2], "w", encoding="ascii") as target: + target.write(str(counter) + "\n") + print(signal) +except (ImportError, OSError, ValueError): + print(0) +PY + ) || should_signal=0 fi # Signal observer if running and throttle allows (check both project-scoped and global observer, deduplicate) diff --git a/tests/hooks/observe-signal-counter-race.test.js b/tests/hooks/observe-signal-counter-race.test.js index 55c9aea77..c116a291d 100644 --- a/tests/hooks/observe-signal-counter-race.test.js +++ b/tests/hooks/observe-signal-counter-race.test.js @@ -6,8 +6,8 @@ * every N observations (#521). The bump used a plain read-modify-write with no * locking, so concurrent hook invocations could read the same value, both * increment, and lose a write — the observer then fired at unpredictable - * intervals. The fix serializes the read-modify-write with an atomic mkdir - * lock. + * intervals. The fix serializes the read-modify-write with an OS-released + * file lock. * * These tests drive the real observe.sh (reusing the stub harness from * observer-memory.test.js) and assert the lock's invariant: with the reset @@ -68,9 +68,8 @@ const observeShPath = path.join(repoRoot, 'skills', 'continuous-learning-v2', 'h const isWindows = process.platform === 'win32'; const hasPython = !isWindows && spawnSync('python3', ['--version']).status === 0; -// When the runner has flock the lock is exact (blocking, kernel auto-release); -// without it observe.sh uses a best-effort mkdir spin that may drop at most one -// increment under pathological contention. +// Both lock implementations wait for at most two seconds, then drop an optional +// tick under pathological contention. const hasFlock = !isWindows && spawnSync('bash', ['-c', 'command -v flock']).status === 0; // Build a self-contained observe.sh sandbox (stub detect-project.sh + @@ -129,7 +128,7 @@ function buildSandbox() { } // Run observe.sh once against the sandbox. Resolves when the process exits. -function runObserve(testObserve, projectDir) { +function runObserve(testObserve, projectDir, extraEnv = {}) { const input = JSON.stringify({ tool_name: 'Read', tool_input: { file_path: '/tmp/test.txt' }, @@ -147,7 +146,8 @@ function runObserve(testObserve, projectDir) { CLAUDE_PROJECT_DIR: projectDir, // Reset threshold far above the invocation count, so no reset fires and // the final counter equals the number of invocations. - ECC_OBSERVER_SIGNAL_EVERY_N: '100000' + ECC_OBSERVER_SIGNAL_EVERY_N: '100000', + ...extraEnv }, stdio: ['pipe', 'ignore', 'pipe'] }); @@ -185,18 +185,6 @@ function readCounter(projectDir) { console.log('\n=== observe.sh signal-counter race regression (#2296) ===\n'); -test('observe.sh uses a lock around the throttle-counter update', () => { - const content = fs.readFileSync(observeShPath, 'utf8'); - assert.ok( - content.includes('SIGNAL_COUNTER_LOCK'), - 'observe.sh should define a lock for the signal counter' - ); - assert.ok( - /flock 8\b/.test(content) || /mkdir "\$SIGNAL_COUNTER_LOCK"/.test(content), - 'observe.sh should acquire the counter lock via flock or an atomic mkdir' - ); -}); - test('observe.sh guards against a corrupt (non-integer) counter file', () => { const content = fs.readFileSync(observeShPath, 'utf8'); assert.ok( @@ -241,12 +229,12 @@ async function runConcurrent() { `with flock the counter must be exactly ${K}, got ${counter}` ); } else { - // mkdir fallback is best-effort: it may drop at most one increment if its + // The portable fallback may drop at most one increment if its // bounded spin is exhausted, but never the multi-increment loss the // unlocked code exhibited. assert.ok( counter >= K - 1, - `mkdir fallback should keep the counter >= ${K - 1}, got ${counter}` + `portable fallback should keep the counter >= ${K - 1}, got ${counter}` ); } } finally { @@ -254,10 +242,69 @@ async function runConcurrent() { } } +async function runStrandedLock() { + const { testDir, projectDir, testObserve } = buildSandbox(); + try { + // Windows TerminateProcess cannot run the shell's cleanup traps. Preserve + // that exact leftover, and make each external sleep as slow as MSYS2. + const stale = path.join(projectDir, '.observer-signal-counter.lock'); + fs.mkdirSync(stale); + fs.writeFileSync(path.join(projectDir, '.observer-signal-counter'), '7\n'); + const bashEnv = path.join(testDir, 'no-flock.sh'); + fs.writeFileSync(bashEnv, [ + 'command() {', + ' if [ "$1" = "-v" ] && { [ "$2" = "flock" ] || [ "$2" = "lockfile" ]; }; then return 1; fi', + ' builtin command "$@"', + '}', + 'sleep() { /bin/sleep 0.08; }', + '' + ].join('\n')); + const started = Date.now(); + await runObserve(testObserve, projectDir, { BASH_ENV: bashEnv }); + assert.ok(Date.now() - started < 3000, 'stale lock must not consume the runner timeout'); + assert.strictEqual(readCounter(projectDir), 8, 'a leftover legacy directory must not freeze signalling'); + assert.ok(fs.statSync(stale).isDirectory(), 'legacy lock must be left untouched'); + } finally { + cleanupDir(testDir); + } +} + +async function runKilledOwner() { + const { testDir, projectDir, testObserve } = buildSandbox(); + let owner; + try { + fs.writeFileSync(path.join(projectDir, '.observer-signal-counter'), '7\n'); + const guard = path.join(projectDir, '.observer-signal-counter.lock.v2'); + const bashEnv = path.join(testDir, 'no-flock.sh'); + fs.writeFileSync(bashEnv, 'command() { if [ "$1" = "-v" ] && [ "$2" = "flock" ]; then return 1; fi; builtin command "$@"; }\n'); + owner = spawn('python3', ['-u', '-c', + 'import fcntl,sys,time\nf=open(sys.argv[1], "a+b")\nfcntl.flock(f,fcntl.LOCK_EX)\nprint("locked",flush=True)\ntime.sleep(30)', guard]); + await new Promise((resolve, reject) => { + owner.once('error', reject); + owner.stdout.once('data', resolve); + }); + const started = Date.now(); + await runObserve(testObserve, projectDir, { BASH_ENV: bashEnv }); + assert.ok(Date.now() - started < 4000, 'busy OS lock must stay within the runner budget'); + assert.strictEqual(readCounter(projectDir), 7, 'busy lock must not allow an unlocked update'); + const closed = new Promise(resolve => owner.once('close', resolve)); + owner.kill('SIGKILL'); + await closed; + await runObserve(testObserve, projectDir, { BASH_ENV: bashEnv }); + assert.strictEqual(readCounter(projectDir), 8, 'OS must release a killed owner without cleanup traps'); + assert.ok(fs.statSync(guard).isFile(), 'persistent guard bytes must not retain lock ownership'); + } finally { + if (owner) owner.kill('SIGKILL'); + cleanupDir(testDir); + } +} + (async () => { if (!isWindows && hasPython) { await asyncTest('sequential invocations increment the counter exactly once each', runSequential); await asyncTest('concurrent invocations never lose a counter increment', runConcurrent); + await asyncTest('stranded mkdir lock cannot delay or freeze later observations', runStrandedLock); + await asyncTest('busy lock skips safely and a killed owner releases it automatically', runKilledOwner); } else { console.log(' - skipping shell-execution tests (requires non-Windows + python3)'); }