WEBDIENSTE/ECC
1
0
Fork 0
mirror of https://github.com/affaan-m/ECC.git synced 2026-10-18 22:13:45 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix: prevent stranded observation counter locks after process death

Browse Source
This commit is contained in:
haelyra
2026-10-11 02:05:20 -04:00
parent 807c98aff6
commit 7f226d069c
2 changed files with 128 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
skills/continuous-learning-v2/hooks/observe.sh
+60 -36
View File
@@ -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)
tests/hooks/observe-signal-counter-race.test.js
+68 -21
View File
@@ -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)');
}