Repository navigation
test(agent-api): prevent telemetry teardown segfault - #2388
Conversation
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Cold-reviewed exact head bfae7604.
Root cause verified end-to-end: the injection suite loads the real agent-api.py and drives handlers that emit product telemetry (task_processed at src/agent-api.py:128) — post-#2274 those start daemon urllib threads, and a short-lived interpreter can exit while they're inside OpenSSL. I hit this exact signature earlier today from the other side (silent clean-install red on #2385's first run, later green — same class; and the #2373/#2370 logs show 'All task-field injection tests passed.' followed by 'Segmentation fault (core dumped)', matching your isolation).
Fix verified: SUTANDO_TELEMETRY=0 is the documented opt-out (src/telemetry.py:25), set BEFORE the module load so the emitter never spawns threads in this suite. Scope is exactly right — one env line in one unrelated-to-telemetry test; production behavior and the dedicated telemetry suite untouched.
Ran at this head: injection suite passes, telemetry suite 24/24 ALL PASS. (macOS can't reproduce the segfault either way — your 100/100 Linux exit runs are the load-bearing repro evidence; noting I verified behavior, not the crash itself.)
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Verified: SUTANDO_TELEMETRY=0 is a real, pre-existing opt-out already checked in src/telemetry.py:122, so this test-only env var set is a legitimate hermetic-test fix, not a new mechanism. Ran the test locally — passes cleanly (macOS doesn't reproduce the Linux-only segfault, as expected, but confirms no regression in the test's own assertions).
This is the actual root cause I was hunting for while reviewing #2370 and flagged on issue #2234 as a suspected shared CPython-finalization crash — turns out it's more specific and more fixable than that: the real telemetry daemon threads spawned by this suite's handler calls can still be mid-flight in OpenSSL when the short-lived test interpreter exits. The PR's own evidence (100/100 clean exits at HEAD vs. reproducing the exact CI failure signature at parent) is convincing, and the fix is minimal, hermetic-only, and doesn't touch production telemetry behavior. This should unblock #2370 and #2373's spurious CI failures. Approving.
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Verified: SUTANDO_TELEMETRY=0 is a real, pre-existing opt-out already checked in src/telemetry.py:122, so this test-only env var set is a legitimate hermetic-test fix, not a new mechanism. Ran the test locally — passes cleanly (macOS does not reproduce the Linux-only segfault, as expected, but confirms no regression in the tests own assertions).
This is the actual root cause I was hunting for while reviewing #2370 and flagged on issue #2234 as a suspected shared CPython-finalization crash — turns out it is more specific and more fixable than that: real telemetry urllib daemon threads spawned by this suites handler calls can still be mid-flight in OpenSSL when the short-lived test interpreter exits. The PRs own evidence (100/100 clean exits at HEAD vs reproducing the exact CI failure signature at parent) is convincing, and the fix is minimal, hermetic-only, and does not touch production telemetry behavior. This should unblock #2370 and #2373s spurious CI failures. Approving.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 68%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head bfae760496dd22b7e95b0df0e1379f9cdd7f9ccb.
No blocker found. The failure mode is verified in #2373's current clean-install log: tests/agent-api-task-field-injection.test.py prints its success line and then segfaults. The suite drives three real Twilio handlers, each calls _emit_task_processed(), and the test-local SUTANDO_TELEMETRY=0 is set before agent-api.py loads, so those daemon urllib/OpenSSL dispatches are suppressed without changing production code.
The isolation boundary is safe: standalone Python suites run in separate interpreters, so the environment mutation cannot leak into tests/telemetry.test.py; that dedicated suite still exercises enabled, opt-out, async, synchronous-flush, CLI, persistence, and payload behavior.
Independent verification passed: task-field injection suite, telemetry suite 24/24, Python compilation, git diff --check, and repository hardcoded-path scan. Worst-case disruption would be hiding telemetry behavior behind a leaked process-global opt-out; per-process test discovery plus the independently passing telemetry suite mitigates that.
Code-ready from this review lane. GitHub's required tsc + tests (clean install) check is still in progress, so the PR is not merge-ready until it completes green.
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 75339b0d215ed28500cd6e9e2a09f4dba0b544a2 after the merge from main.
The refresh is merge-only. The PR-owned task-field test blob and the surrounding src/agent-api.py / src/telemetry.py blobs are byte-identical to approved bfae7604; the only child delta is merged #2338's already-reviewed bot2bot scope guard.
Current-head verification passed: task-field injection suite, bot2bot scope-guard suite, Python compilation, git diff --check, and repository hardcoded-path scan. The dedicated telemetry suite remains the same 24/24 version that passed during the prior exact-patch review.
No blocker found. Code-ready and mergeable from this review lane; required refreshed CI is still running, so merge readiness still waits on green checks.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 75339b0d215ed28500cd6e9e2a09f4dba0b544a2.
No blockers found. The PR keeps the task-field injection suite hermetic by setting the pre-existing SUTANDO_TELEMETRY=0 opt-out before loading src/agent-api.py, so the handler calls still exercise real task-writing behavior without spawning fire-and-forget telemetry threads in this short-lived interpreter. The dedicated telemetry suite remains untouched and still covers opt-in delivery, opt-out behavior, task source bucketing, and the synchronous CLI flush path.
Focused verification passed:
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pycache-2388-current python3 tests/agent-api-task-field-injection.test.pyPYTHONPYCACHEPREFIX=/private/tmp/sutando-pycache-2388-current python3 tests/telemetry.test.py(rerun with localhost bind allowed after the sandbox blocked the test server)PYTHONPYCACHEPREFIX=/private/tmp/sutando-pycache-2388-current python3 -m py_compile tests/agent-api-task-field-injection.test.py src/agent-api.py src/telemetry.pygit diff --check origin/main...HEAD
GitHub CI was still finishing on this merge-only head at review time; merge should still wait for the required checks to complete green.
Reviewed by Qingyun's Personal Codex.
`main` currently fails `tsc + tests (clean install)` for any branch that pulls it
in. The failure is a post-success interpreter crash, not a test failure:
github-webhook-access-tier: 31/31 passed
double free or corruption (out)
Segmentation fault (core dumped)
##[error]Process completed with exit code 1.
Mechanism (diagnosed by @liususan091219 on #2324): since #2274 the task-accepting
paths emit `task_processed`, and `src/github-webhook.py:_emit_github_telemetry()`
lazily imports it. With flush=False the production emitter starts a DAEMON urllib
thread, which can still be inside OpenSSL while this short-lived interpreter
finalizes. A CI runner has no telemetry opt-out marker, so `enabled()` is True.
#2388 fixed exactly one suite this way (tests/agent-api-task-field-injection.test.py).
This is a second suite with the identical mechanism; same fix, same reasoning, so
the two stay consistent.
## What is and is not verified
NOT reproducible on macOS — the glibc double-free does not occur here, so I cannot
show the crash stopping. Saying so plainly rather than implying otherwise.
What IS verified locally is the precondition the crash requires, and that this
change removes it. Running the real suite under a probe that inspects threads at
interpreter exit:
unpatched: telemetry_env=None lingering_threads=['Thread-1']
patched: telemetry_env=0 lingering_threads=[]
And the lever itself:
SUTANDO_TELEMETRY unset -> enabled()=True, task_processed spawns 1 thread
SUTANDO_TELEMETRY=0 -> enabled()=False, task_processed spawns 0 threads
So: a live daemon thread survives to finalization without this, and does not with
it. That is the documented cause, eliminated. Suite still 31/31.
Placed after `from __future__ import annotations` (which must lead the file) and
before every regular import, so nothing can import `telemetry` first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DyfS386nrKn8FFdtRLme6x
…do (#2372) * fix(hooks): install hooks pointing at THIS clone, not ~/Desktop/sutando install-claude-hooks.sh derives REPO_DIR on line 56 and then did not use it: the three script hooks hardcoded $HOME/Desktop/sutando. The $ was escaped, so that path landed in settings.json literally and was expanded at HOOK-RUN time, resolving to the same directory on every host regardless of where the repo is. On this machine the clone is at "Library/Application Support/space.ag2.app/engine/sutando" and a sibling sits at "Documents/github/sutando"; neither is ~/Desktop. Running the installer here would wire four hooks whose scripts do not exist, and hooks fail silently — the absence looks identical to "the feature is off". Before (what main installs on this host): MISSING ~/Desktop/sutando/src/session-handoff.sh <- fires, fails, silent MISSING ~/Desktop/sutando/src/check-pending-tasks.sh After (expanded at install time from the derived REPO_DIR): bash <clone>/src/session-handoff.sh "$TRANSCRIPT_PATH" bash <clone>/src/check-pending-tasks.sh EXISTS both. $HOME is left alone in the transcript-archive hook: ~/Desktop/sutando-conversations is a genuine user-chosen destination, not a repo path, so deriving it would be wrong. Found while root-causing a dead relay pipeline: workspace/relay/ holds 124 notes with relay/processed/ empty and no session-state.md, because session-handoff.sh has never run here — the PreCompact/SessionEnd hooks that invoke it were never installed. This does not by itself prove the hardcoded path caused that (no repo-local .claude/settings.json exists in either clone, so the installer appears never to have been run at all), but it does mean following the documented install path could not have fixed it. Not running the installer here — wiring hooks changes the owner's live environment and is their call. This fixes the script so the wiring works when they choose to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZ5eEAmPuFnSA2TkqtQhzZ * fix(hooks): quote the installed repo path and migrate stale hook variants Both blockers, plus a third the reviews implied but did not name. 1. QUOTING. REPO_DIR is expanded at INSTALL time, so its literal text is re-parsed by a shell at HOOK-RUN time. Unquoted, the clone this fix targets — "Library/Application Support/space.ag2.app/engine/sutando" — splits on the space and every hook dies with `bash: /Users/you/Library: No such file or directory`, silently, because a hook's exit status is not surfaced. Paths now go through shq(), which single-quotes with '\'' escaping: metacharacter-proof, unlike double quotes which would still interpolate $ and backtick. 2. MIGRATION. Phase 1 compares exact command strings, so it could only ever ADD. Hook specs now carry a stable <marker>, and a new phase 0 removes any command for that event carrying our marker but not byte-identical to the one being installed. That covers the legacy $HOME/Desktop/sutando/... entries called out in review. 3. THE THIRD CASE. That same add-only comparison means anyone who ran the installer at 355c4cf holds an UNQUOTED entry which no exact-match can find — so the broken and the fixed hook would both fire. Phase 0 sweeps it, and the same mechanism cleans an entry left by a different clone. Scoping: the sweep is marker-scoped, so "src/session-handoff.sh" cannot match the transcript-archive hook's "sutando-conversations/" — the separate archive destination is preserved, as asked — and an operator's hand-added hook is untouched. Phase 0 runs BEFORE phase 1 so the fresh command is never swept. tests/install-claude-hooks.test.sh — 16 checks, every fixture path containing a space. The load-bearing assertions EXECUTE the stored command string and assert which script it reached, rather than asserting the JSON contains a path: ok SessionEnd stored command executes the intended script ok legacy Desktop session-handoff hook removed (PreCompact/SessionEnd) ok legacy Desktop check-pending-tasks hook removed (Stop) ok transcript-archive hook preserved (different marker) ok operator-added unrelated hook preserved ok exactly one SessionEnd handoff hook (no old+new double-fire) ok second run adds nothing (added=0) / byte-identical settings.json ok unquoted stale variant swept on re-run Discriminates on both baselines: 8 of 16 fail on origin/main and 11 of 16 on 355c4cf, including all three "stored command executes" and all three legacy-migration checks. (On a broken baseline the fixture-sanity check also fails, as a downstream artifact rather than an independent signal.) bash -n clean, hardcoded-path scanner clean, git diff --check clean. shellcheck not run locally; CI gates at -S error and tests/ is in its scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLGykMovYC6LM99914TQNC * fix(hooks): sweep only installer-generated shapes, not any same-marker command The phase-0 sweep treated "contains our marker" as ownership and deleted every same-marker command that was not byte-identical to the one being installed. An operator hook invoking the same script with an extra flag matches that test, so a re-run silently removed it — and a removed hook leaves no trace. The comment above the loop claimed the opposite ("a hook the operator added by hand is untouched"), which is how it survived review. Ownership is now the SHAPE this installer writes: the current command with the repo path replaced by a wildcard, anchored at both ends, matched with quotes normalized away so the quoted and unquoted forms both qualify. Customized commands carry extra text, fail the trailing anchor, and survive. A different clone's entry still matches — that shape is installer-generated, just not by this checkout — so this narrows ownership without disabling the migration. Quote stripping is used for MATCHING only; nothing is rewritten from it. Adds 6 checks. 4 of them fail on the parent commit: the three operator-hook survival cases and the combined "sweeping a stale entry did not take the operator hooks with it". The other two are regression guards that pass either way, including "another clone's installer-shaped entry is still swept", which is what proves the fix is not simply a weaker sweep. * fix(hooks): derive the sweep shape structurally, not by stripping quotes An apostrophe is legal in a checkout path, and `shq` escapes it as '\'' — so deleting quote characters to "normalize" a command leaves a stray backslash behind. On a clone at `/repo'quote` the normalized command and the normalized repo path then disagreed, the split found nothing, and the shape collapsed to an exact match of the whole command. Too narrow to recognise the installer's own earlier output, so the stale unquoted entry survived and both hooks kept firing: before_rerun=2, after_rerun=2. Nothing crashed, which is what made it worth a regression test. The shape is now built from structure rather than string surgery: command word + marker (already the stable structural token) + the literal tail after the marker, with the path region wildcarded and the closing quote optional. Quoted and unquoted forms both match; a customized command has extra text and fails the trailing anchor, so it still survives. The jq predicates test the raw command — no lossy pre-processing anywhere. Adds 3 checks on a fixture path containing BOTH a space and an apostrophe. One is the discriminator (`stale variant IS swept on re-run`) and fails on the parent; the other two are a fixture-sanity check and a does-it-execute guard that pass either way. * fix(hooks): the sweep's path region must start like a path, not a flag The trailing anchor protected customization AFTER the script path but said nothing about customization BEFORE it. A bare wildcard between the command word and the marker swallowed a leading flag, so an operator hook bash -x <repo>/src/session-handoff.sh "$TRANSCRIPT_PATH" was classified installer-owned and deleted on re-run. Reproduced on the parent: before 2, after 1 — the -x hook gone, silently. Same durability class as the two earlier false positives, approached from the other side of the path. The path region now requires an optional opening quote followed by a character that is neither a space nor `-`. That admits every shape this installer writes or has written — /abs/..., '/quoted path/...', '/repo'\''quote/...' — plus the legacy $HOME/Desktop/... entries the migration exists to remove. A flag or wrapper token fails immediately. Requiring a literal `/` would have been the obvious rule and is wrong: it spares exactly the legacy $HOME entries this sweep must sweep. There is a committed check for that so the mistake cannot be reintroduced quietly. Adds 4 checks. 2 fail on the parent: the bash -x survival and the resulting count. The other 2 — fixture sanity and "legacy is still swept" — pass either way; the second is the guard against over-narrowing. * fix(hooks): never sweep a hook that embeds no repo path Phase 0 migrates entries whose PATH went stale — a legacy $HOME/Desktop clone, an unquoted form, another checkout. The transcript-archive hook embeds no repo path at all ($HOME only), so nothing about it can go stale and sweeping it can only destroy someone else's command. With a wildcard shape the `.*` spanned the SOURCE ARGUMENT rather than a path, so an operator archiving from a different variable cp "$CUSTOM_TRANSCRIPT_PATH" "$HOME/Desktop/sutando-conversations/…" matched "our shape" and was deleted and replaced on re-run, silently discarding their archival policy. Reproduced on the parent. This is a regression I introduced: an earlier revision had an explicit "no repo path -> exact match" branch, and rewriting the shape structurally dropped it. Restored as an explicit skip, which states the intent instead of encoding it as a pattern that happens to be unreachable. The guard compares against the ESCAPED path body, not raw $REPO_DIR: shq rewrites an internal `'` as `'\''`, so on /repo'quote a raw comparison never matches and the guard would skip the sweep for exactly the hooks it must run on — the same raw-vs-escaped confusion behind the apostrophe bug this file already fixed once. Caught by the existing apostrophe test. Adds 3 checks. 1 fails on the parent (the operator's archive command is destroyed); the other 2 assert both of our hooks still install, and pass either way — they are the guard against "fix it by skipping everything". * docs(hooks): the ownership-test summary described a removed implementation Four fixes landed on this block tonight and the header comment was never re-read as a whole. It still claimed the sweep compares "with quotes normalized away so the quoted and unquoted forms both match" — which is the 479ffe0 approach that ddaa425 REMOVED for being lossy. So the summary described an implementation that no longer exists, and specifically the one the code was changed away from. A comment describing work that no longer exists reads as a live description; there is no tense marker to tell a reader otherwise. Worse here, the next person to "restore consistency" would have reintroduced the apostrophe bug. Header now states the actual three conditions, each annotated with the false positive that produced it — embeds-the-repo-path, path region must start like a path, exact tail — and says plainly that matching is on the RAW command with no pre-processing. The two remaining quote-stripping mentions further down are historical by construction ("never by stripping quotes", "was lossy and wrong") and are correct as prohibitions. Comment-only; 33/33 unchanged. * test(github-webhook): disable telemetry in the access-tier suite `main` currently fails `tsc + tests (clean install)` for any branch that pulls it in. The failure is a post-success interpreter crash, not a test failure: github-webhook-access-tier: 31/31 passed double free or corruption (out) Segmentation fault (core dumped) ##[error]Process completed with exit code 1. Mechanism (diagnosed by @liususan091219 on #2324): since #2274 the task-accepting paths emit `task_processed`, and `src/github-webhook.py:_emit_github_telemetry()` lazily imports it. With flush=False the production emitter starts a DAEMON urllib thread, which can still be inside OpenSSL while this short-lived interpreter finalizes. A CI runner has no telemetry opt-out marker, so `enabled()` is True. #2388 fixed exactly one suite this way (tests/agent-api-task-field-injection.test.py). This is a second suite with the identical mechanism; same fix, same reasoning, so the two stay consistent. ## What is and is not verified NOT reproducible on macOS — the glibc double-free does not occur here, so I cannot show the crash stopping. Saying so plainly rather than implying otherwise. What IS verified locally is the precondition the crash requires, and that this change removes it. Running the real suite under a probe that inspects threads at interpreter exit: unpatched: telemetry_env=None lingering_threads=['Thread-1'] patched: telemetry_env=0 lingering_threads=[] And the lever itself: SUTANDO_TELEMETRY unset -> enabled()=True, task_processed spawns 1 thread SUTANDO_TELEMETRY=0 -> enabled()=False, task_processed spawns 0 threads So: a live daemon thread survives to finalization without this, and does not with it. That is the documented cause, eliminated. Suite still 31/31. Placed after `from __future__ import annotations` (which must lead the file) and before every regular import, so nothing can import `telemetry` first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DyfS386nrKn8FFdtRLme6x --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: john-the-dev <johnv.the.dev@gmail.com>
What changed, and why?
tests/agent-api-task-field-injection.test.pynow disables product telemetry before loading and driving the real agent API handlers. This suite tests task-file confinement, not telemetry; after #2274 landed, its three handler calls started real daemonurllibthreads. On Linux clean-install runners the interpreter can exit while those threads are still inside the network/OpenSSL stack, producing a segmentation fault after every assertion passes.The production telemetry behavior is unchanged. The dedicated telemetry suite still covers enabled, opt-out, async, flush, CLI, persistence, and payload behavior.
Review first
tests/agent-api-task-field-injection.test.py:34-41User-visible behavior
No product behavior changes. This removes a nondeterministic required-CI failure that currently blocks unrelated #2370 and #2373.
Documentation consistency
N/A — test isolation only; no feature, config, or public behavior changed.
Before / after evidence
The refreshed #2373 clean-install job on parent
c71a637dended with:The same unchanged test produced the same post-success failure twice on #2370.
A local debug run against the parent test proves the three real emitters are enabled:
At HEAD, the exact same command proves the test-local opt-out prevents thread creation:
Repeated short-process exit check at HEAD:
Tests
Live path
N/A — production bridge/network/startup code is unchanged. The test intentionally stops exercising live PostHog I/O; telemetry's dedicated tests continue to exercise the dispatch contract with controlled fakes.
Edge cases / risk / rollback
agent-api.pycan lazily importtelemetry.