-
-
Notifications
You must be signed in to change notification settings - Fork 40.5k
fix(continuous-learning-v2): warn when the observer never survives a hook invocation (#2489) #2606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
e7302d9
f6d2e4a
cabb433
05fe037
12ea937
bf10629
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -375,6 +375,14 @@ _REMOVE_FILE_IF_PRESENT() { | |
| _START_OBSERVER_LOGGED() { | ||
| local bootstrap_log="${PROJECT_DIR}/observer-start.log" | ||
| mkdir -p "$PROJECT_DIR" | ||
| # Every call site below sits inside the lazy-start lock (flock / lockfile / | ||
| # mkdir), so the streak read-modify-write in _NOTE_OBSERVER_NOSURVIVE is | ||
| # serialized here without a second lock -- concurrent hook invocations cannot | ||
| # lose an increment or double-log the warning. Counting at the restart (rather | ||
| # than at detection) also means N racing hooks record one death, not N. | ||
| if [ "${OBSERVER_DIED:-false}" = "true" ]; then | ||
| _NOTE_OBSERVER_NOSURVIVE | ||
| fi | ||
| "${SKILL_ROOT}/agents/start-observer.sh" start >> "$bootstrap_log" 2>&1 || true | ||
| } | ||
|
|
||
|
|
@@ -393,12 +401,58 @@ _CHECK_OBSERVER_RUNNING() { | |
| if kill -0 "$pid" 2>/dev/null; then | ||
| return 0 # Process is alive | ||
| fi | ||
| # Stale PID file - remove it | ||
| # Stale PID file - remove it. A well-formed PID that is no longer alive | ||
| # means an observer we launched has since died, which is the only evidence | ||
| # of non-survival any process ever sees (#2489). Record it; the caller | ||
| # decides whether the streak is long enough to warn about. | ||
| OBSERVER_DIED=true | ||
| _REMOVE_FILE_IF_PRESENT "$pid_file" | ||
| fi | ||
| return 1 # No PID file or process dead | ||
| } | ||
|
|
||
| # The observer is lazy-started from a hook process that exits immediately after. | ||
| # start-observer.sh's own liveness check runs inside that still-living process | ||
| # tree, so it always sees a healthy observer and reports success -- on native | ||
| # Windows the reap happens later, when the hook's Job Object closes. The next | ||
| # hook invocation is therefore the only place the death is observable, and | ||
| # before #2489 it silently deleted the stale PID and restarted, once per tool | ||
| # call, forever. Warn once per streak so this is signal rather than noise. | ||
| _NOTE_OBSERVER_NOSURVIVE() { | ||
| local streak_file="${PROJECT_DIR}/.observer-nosurvive-count" | ||
| local log_file="${PROJECT_DIR}/observer-start.log" | ||
| local warn_after="${ECC_OBSERVER_NOSURVIVE_WARN_AFTER:-3}" | ||
| local streak | ||
| streak=$(cat "$streak_file" 2>/dev/null || echo 0) | ||
| case "$streak" in ''|*[!0-9]*) streak=0 ;; esac | ||
| case "$warn_after" in ''|*[!0-9]*|0) warn_after=3 ;; esac | ||
| streak=$((streak + 1)) | ||
| printf '%s\n' "$streak" > "$streak_file" 2>/dev/null || true | ||
|
|
||
| # Fire on equality, not >=, so a persistent failure logs once per streak | ||
| # instead of once per tool call. | ||
| if [ "$streak" -eq "$warn_after" ]; then | ||
| { | ||
| printf '[observe] Observer did not survive to the next hook invocation %s times in a row.\n' "$streak" | ||
| printf '[observe] Startup reports success, but the process is gone by the following tool call, so no analysis ever runs.\n' | ||
| case "$(uname -s 2>/dev/null | tr '[:upper:]' '[:lower:]')" in | ||
| *mingw*|*msys*|*cygwin*) | ||
| printf '[observe] On native Windows (Git Bash/MSYS2) this is expected: the background launch does not detach the observer from the hook process Job Object, so it is killed when the hook exits. Run under WSL2, Linux or macOS. See issue #2489.\n' | ||
| ;; | ||
| *) | ||
| printf '[observe] Check %s for the reason the observer exited.\n' "${PROJECT_DIR}/observer.log" | ||
| ;; | ||
| esac | ||
| printf '[observe] Set ECC_OBSERVER_NOSURVIVE_WARN_AFTER to change this threshold (currently %s).\n' "$warn_after" | ||
| } >> "$log_file" 2>/dev/null || true | ||
| fi | ||
| return 0 | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| _RESET_OBSERVER_NOSURVIVE_STREAK() { | ||
| _REMOVE_FILE_IF_PRESENT "${PROJECT_DIR}/.observer-nosurvive-count" | ||
| } | ||
|
|
||
| if [ -f "${CONFIG_DIR}/disabled" ]; then | ||
| OBSERVER_ENABLED=false | ||
| else | ||
|
|
@@ -427,9 +481,21 @@ fi | |
|
|
||
| # Check both project-scoped AND global PID files (with stale PID recovery) | ||
| if [ "$OBSERVER_ENABLED" = "true" ]; then | ||
| # Clean up stale PID files first | ||
| _CHECK_OBSERVER_RUNNING "${PROJECT_DIR}/.observer.pid" || true | ||
| _CHECK_OBSERVER_RUNNING "${CONFIG_DIR}/.observer.pid" || true | ||
| # Clean up stale PID files first. | ||
| # `if` context (not `|| true`) so `set -e` stays satisfied while we still | ||
| # capture whether either PID file pointed at a live observer. | ||
| OBSERVER_ALIVE=false | ||
| OBSERVER_DIED=false | ||
| if _CHECK_OBSERVER_RUNNING "${PROJECT_DIR}/.observer.pid"; then OBSERVER_ALIVE=true; fi | ||
| if _CHECK_OBSERVER_RUNNING "${CONFIG_DIR}/.observer.pid"; then OBSERVER_ALIVE=true; fi | ||
|
|
||
| # A live observer clears the streak so a later one-off crash does not inherit | ||
| # an old count. This is an idempotent unlink, not a read-modify-write, so it | ||
| # needs no lock. The matching increment runs inside the lazy-start lock, in | ||
| # _START_OBSERVER_LOGGED. | ||
| if [ "$OBSERVER_ALIVE" = "true" ]; then | ||
| _RESET_OBSERVER_NOSURVIVE_STREAK | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
An invocation can observe a live observer and pause before this reset. If that observer then exits, another invocation detects the stale PID and persists a no-survival increment while holding the lazy-start lock. The first invocation subsequently unlinks the counter here, erasing the newer increment. Since the warning fires only when the persisted count equals the threshold, the next failure starts the count over and delays or suppresses the warning. Serialize the reset with the counter increment using the same lock, and revalidate liveness after acquiring it. ArtifactsDeterministic observer no-survival reset race harness
Baseline threshold warning execution output
Concurrent liveness reset race execution output
Twenty repeated observer reset race executions
|
||
| fi | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| # Check if observer is now running after cleanup | ||
| if [ ! -f "${PROJECT_DIR}/.observer.pid" ] && [ ! -f "${CONFIG_DIR}/.observer.pid" ]; then | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_NOTE_OBSERVER_NOSURVIVEcombines threshold parsing, persistence, platform detection, message construction, and logging across approximately 62 lines, exceeding the repository's 50-line function limit and making these behaviors harder to review and test independently.File Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!