Skip to content

Commit cabb433

Browse files
committed
fix(continuous-learning-v2): serialize the non-survival streak under the lazy-start lock
observe.sh runs on every tool call, so the streak read-modify-write could race between concurrent invocations -- losing an increment or logging the warning twice. That is the same class of bug the signal counter hit in #2296, and this repo's rule is to never fall back to an unlocked read-modify-write. Rather than add a second lock, move the increment into _START_OBSERVER_LOGGED. All three of its call sites already run inside the lazy-start lock (flock / lockfile / mkdir), so the update is serialized with no new machinery. Counting at the restart instead of at detection also means N racing hooks record one death rather than N. The reset stays in the caller: it is an idempotent unlink, not a read-modify-write, so it needs no lock. Adds a regression case pinning the increment inside _START_OBSERVER_LOGGED and asserting all three call sites remain locked.
1 parent f6d2e4a commit cabb433

2 files changed

Lines changed: 30 additions & 4 deletions

File tree

‎skills/continuous-learning-v2/hooks/observe.sh‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,14 @@ _REMOVE_FILE_IF_PRESENT() {
375375
_START_OBSERVER_LOGGED() {
376376
local bootstrap_log="${PROJECT_DIR}/observer-start.log"
377377
mkdir -p "$PROJECT_DIR"
378+
# Every call site below sits inside the lazy-start lock (flock / lockfile /
379+
# mkdir), so the streak read-modify-write in _NOTE_OBSERVER_NOSURVIVE is
380+
# serialized here without a second lock -- concurrent hook invocations cannot
381+
# lose an increment or double-log the warning. Counting at the restart (rather
382+
# than at detection) also means N racing hooks record one death, not N.
383+
if [ "${OBSERVER_DIED:-false}" = "true" ]; then
384+
_NOTE_OBSERVER_NOSURVIVE
385+
fi
378386
"${SKILL_ROOT}/agents/start-observer.sh" start >> "$bootstrap_log" 2>&1 || true
379387
}
380388

@@ -481,12 +489,12 @@ if [ "$OBSERVER_ENABLED" = "true" ]; then
481489
if _CHECK_OBSERVER_RUNNING "${PROJECT_DIR}/.observer.pid"; then OBSERVER_ALIVE=true; fi
482490
if _CHECK_OBSERVER_RUNNING "${CONFIG_DIR}/.observer.pid"; then OBSERVER_ALIVE=true; fi
483491

484-
# Count non-survival once per hook invocation, not once per PID file: the
485-
# double-check calls below run after the stale files are already gone.
492+
# A live observer clears the streak so a later one-off crash does not inherit
493+
# an old count. This is an idempotent unlink, not a read-modify-write, so it
494+
# needs no lock. The matching increment runs inside the lazy-start lock, in
495+
# _START_OBSERVER_LOGGED.
486496
if [ "$OBSERVER_ALIVE" = "true" ]; then
487497
_RESET_OBSERVER_NOSURVIVE_STREAK
488-
elif [ "$OBSERVER_DIED" = "true" ]; then
489-
_NOTE_OBSERVER_NOSURVIVE
490498
fi
491499

492500
# Check if observer is now running after cleanup

‎tests/hooks/observe-nosurvive-warning.test.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,24 @@ test('observe.sh records non-survival when clearing a live-looking stale PID', (
217217
);
218218
});
219219

220+
test('the streak increment runs under the lazy-start lock, never unlocked', () => {
221+
const content = fs.readFileSync(observeShPath, 'utf8');
222+
// observe.sh fires on every tool call, so an unlocked read-modify-write on
223+
// the streak file would lose increments or double-log the warning -- the same
224+
// race the signal counter hit in #2296. The increment must therefore live in
225+
// _START_OBSERVER_LOGGED, which every call site invokes inside the
226+
// flock/lockfile/mkdir lazy-start lock.
227+
const starter = content.match(/_START_OBSERVER_LOGGED\(\)\s*\{[\s\S]*?\n\}/);
228+
assert.ok(starter, 'observe.sh should still define _START_OBSERVER_LOGGED');
229+
assert.ok(
230+
starter[0].includes('_NOTE_OBSERVER_NOSURVIVE'),
231+
'the streak increment should run inside _START_OBSERVER_LOGGED, under the lazy-start lock'
232+
);
233+
// Every _START_OBSERVER_LOGGED call site must be inside a lock branch.
234+
const callSites = content.split('\n').filter((line) => /^\s+_START_OBSERVER_LOGGED\s*$/.test(line));
235+
assert.strictEqual(callSites.length, 3, 'expected the three locked lazy-start call sites');
236+
});
237+
220238
test('the non-survival warning is threshold-gated, not logged every call', () => {
221239
const content = fs.readFileSync(observeShPath, 'utf8');
222240
assert.ok(

0 commit comments

Comments
 (0)