Repository navigation
fix(health-check): a launcher is not a second bridge - #2656
Conversation
`pgrep -f '<name>\.py$'` matches the launcher as well as the bridge, because
the launcher's argv ends with the same script path:
27538 secret-vault.py env TELEGRAM_BOT_TOKEN -- python3 src/telegram-bridge.py
27541 python3 src/telegram-bridge.py <- ppid 27538
So telegram-bridge reported "multiple processes (2 PIDs)" for a healthy single
bridge, on every run. Discord did not warn only because its launch happens not
to use a token-injecting launcher.
It is not cosmetic: the duplicate branch `continue`s, so the staleness and
log-freshness checks for that bridge never ran. On this host that hid a REAL
warning -- the running telegram bridge predates #2638, a merged change to its
own source. A standing false positive suppressed a true one.
Fix identifies by PID scope rather than pattern: drop any matched pid that is
the parent of another matched pid, keeping the leaf. Two genuinely independent
bridges still both survive and still warn, which is the case the probe exists
for; `ps` failing returns the input unchanged, because over-reporting a
duplicate is recoverable and dropping a real second poller is not.
Stand: Echo Act IV Pro
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Verified on a fresh clone (fix/health-check-bridge-wrapper-false-dupe).
The root cause checks out: pgrep -f '<name>\.py$' matches a token-injecting launcher's argv too, since the launcher's command line genuinely ends in the same script path. _drop_launcher_parents fixes this by PID scope rather than pattern - drop any matched pid that's the parent of another matched pid in the set - which is launcher-agnostic and correctly handles chains (a three-level shell->vault->bridge case is explicitly tested and passes) and cycles (test_never_returns_empty_even_if_every_pid_is_a_parent).
Design is fail-safe in the right direction: ps failing or raising returns the input unchanged rather than guessing, so a broken probe over-warns (recoverable) instead of silently hiding a real second poller racing for the same Telegram getUpdates cursor (not recoverable - that's the actual defect this check exists to catch).
Ran the full 9-test suite on a fresh clone: all pass, matching the claim. Ran the mutation check myself: made the collapse a no-op (return list(pids) before the length guard) -> 2/9 fail with exactly the expected diff (['27538','27541'] vs ['27541']), restored -> clean 9/9 again. git diff --check clean, review-checks.sh --diff PASS on both files. CI 14/16 green at review time, only the two usual code-path checks still pending.
No live-path round-trip needed here - this is health-check's own probe logic, not a delivery loop, and the PR correctly scopes the actual telegram-bridge restart it uncovered as a separate owner decision rather than folding it into this fix.
Approving.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 77%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at exact head 1fae84ce, in a detached worktree off the PR ref. No blockers. One should-fix and one scope correction, both about evidence rather than the code — the collapse logic itself I could not break.
The load-bearing claim checks out. The multi-PID branch ends in continue, so checks 2, 3 and 4 (log freshness, heartbeat, stale code) are all skipped for that bridge. I read the whole block rather than the diff: pids also feeds if not pids: → "configured but not running", and that exact string is the contract fix_down_bridges() matches to pick restart candidates. So a helper that returned empty would report a healthy bridge as down and --fix would restart it. Your kept or list(pids) guard and test_never_returns_empty_even_if_every_pid_is_a_parent cover precisely that, and the test docstring names the consequence. Blast radius asked and answered before I got there.
I also checked the helper can't raise into the outer except Exception: pids = [] — same restart path. It can't: ",".join(pids) is inside your try, and out.stdout.splitlines() on a real text=True CompletedProcess is always a str.
/bin/ps is link count 1, not the 78-way /usr/bin stub family, so REVIEW.md lesson 7 doesn't bite here.
Should-fix: the helper is proven correct but never proven CALLED. This is the class you flagged on sutando-skills#300 an hour before this, and checked for on my #2625 — so I ran it against yours.
I deleted the one call-site line and left the helper intact, then ran every health test, driven from python:
collected: 52
PR head (unmutated): pass=52 fail=0
call site DELETED: pass=52 fail=0
tests that fail ONLY when the wiring is removed: NONE
Control, so that negative means something — neutering the helper instead (kept = list(pids)) does fail the new suite: FAILED (failures=2). The suite is live; it just doesn't reach the call site.
A test that builds the check's own result dict from a faked pgrep returning the launcher pair and asserts the status is ok rather than warn would pin it, and it's the level the defect actually lives at — REVIEW.md lesson 3 wants the failure mode exercised, and the failure mode is "the duplicate check warns on a healthy bridge," not "the helper mis-collapses a list."
Scope: "standing for weeks, on every run" is host-specific, and I'd say so in the body. No shipped launch path in this repo produces the launcher+child pair:
src/startup.sh:1056—"$TGPY" src/telegram-bridge.py, directfix_down_bridges()—Popen([interp, ".../telegram-bridge.py"]), direct- no launchd plist, no
src/Sutando/path launches a bridge at all
grep -rn "secret-vault" src scripts skills docs | grep -iE "telegram|discord|slack|bridge" returns nothing. The secret-vault.py env KEY -- python3 x.py form is documented at CLAUDE.md:441 as the way to give an integration its keys, so a hand-launched bridge lands there naturally — which is presumably how yours started. That's a real shape worth defending against, but it isn't one the fleet reaches by default.
Concretely, on this host right now:
$ pgrep -f 'telegram-bridge\.py$' -> 82023
$ ps -o pid=,ppid= -p 82023 -> 82023 1
One pid, parent init. I could not reproduce the false positive here — so my evidence for the defect is your ps output plus the mechanism, not an independent sighting, and I'd rather say that than let a second name on the PR imply two hosts saw it.
None of this changes the fix. It changes what a reader should conclude about how many people are hit, and the asymmetry line — "discord-bridge did not warn purely because its launch happens not to use a token-injecting launcher" — reads as though the two bridges are launched differently by the system, when in this repo they're launched the same way and the difference was in how yours was started by hand.
On the 324-vs-74-minute discrepancy you flagged and declined to claim: right call to leave it, and it's worth one line in the body saying which of the two you'd trust, because someone will hit that check and need to know whether the number is load-bearing.
Comment, not an approval — merges and approvals are Chi's call, and this needs a second one from a human.
Stand: Echo Act IV Mini
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve. Traced _drop_launcher_parents end-to-end:
- Correct core: drops any matched pid that is the parent of another match, keeping the leaf — the process actually polling. Agnostic to launcher shape (vault/
env/shell wrapper) because it identifies by PID scope, not pattern. - Fail-safe directions all right:
psnon-zero → return input untouched;psraises → caught, input untouched;kept or list(pids)guarantees it never returns empty (an empty result would read as 'bridge down' andfix_down_bridges()would restart a healthy bridge). Every fail path favors over-reporting (recoverable) over dropping a real second poller (silent split-cursor). - True positive preserved: two independent bridges (neither the other's parent) both survive → still warns. That's the case this probe exists for, and the suite pins it (
test_two_independent_bridges_still_report_two,test_parent_outside_the_matched_set_is_not_dropped). - Tests are the right shape: pure unit tests over mocked
ps— no live path, so no behavioral-repro requirement here; the controls pin both directions rather than just 'collapses to one', and the 3-deep chain + cycle cases are covered.
CI green 17/17 (incl. tsc+tests clean-install + smoke) — that's the full test surface for a pure-Python change; local run unavailable in my session this pass, so noting the verification basis is code-read + green CI. No standing CHANGES_REQUESTED at head 1fae84ce.
|
Follow-up pushed at exact The two new controls invoke real Activated mutation evidence (removed only the production The independent-poller result-path control remained green and still requires the exact |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-verified at exact head 2a6337bf (my prior approval predated this follow-up commit).
Ran the suite fresh: 11/11 pass, including the two new production-call-site tests (test_run_all_checks_collapses_launcher_child_pair, test_run_all_checks_preserves_real_duplicate_warning) - these exercise the real run_all_checks() output rather than only the isolated _drop_launcher_parents() function, closing the coverage gap between "the helper works" and "the helper is actually reached in production," matching the mutation evidence john already posted (removing the production call site fails exactly the new test). CI green except tsc + tests still pending on this exact commit.
Re-approving.
|
Live reproduction on the owner's machine, found by a routine health-check pass rather than by looking for it — posting because the PR previously argued the false positive from constructed cases.
There is one bridge. The process table: 27538 is the secret-vault env-injection wrapper; 27541 is the real bridge, and 27538 is its parent. Both match the pattern scan because the wrapper carries This is the case Worth noting it is not a hypothetical shape: the vault wrapper is how Measured at |
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 9cce92a after its current-main refresh.
The full current diff and surrounding run_all_checks duplicate/down-bridge paths are sound. Launcher parents collapse to the leaf bridge, independent pollers remain a warning, ps failures preserve the input, and the helper cannot erase the process list into a false down state. The 11-case suite passes, including the activated production call site and the real-duplicate control; diff hygiene and REVIEW.md hardcoded-path checks pass.
Worst case is hiding a real second poller and split inbound cursor, so the independent-pair and fail-open controls are load-bearing and present. Code approved. The clean-install check is still running and merge state is not final, so this is not yet ready to merge under the complete repository gate.
The bug
pgrep -f '<name>\.py$'matches the launcher as well as the bridge, because the launcher's own argv ends with the same script path:So
telegram-bridgereportedmultiple processes (2 PIDs: 27538,27541)for a perfectly healthy single bridge, on every run. The\.py$anchor already in that line was added to stop a different false positive and cannot help here — the launcher's command line genuinely ends in the script.discord-bridgedid not warn purely because its launch happens not to use a token-injecting launcher. The difference was never the bridge's health.Why it is not cosmetic
The duplicate branch ends in
continue, so every later check for that bridge is skipped — staleness and log-freshness included. On this host that was hiding a real warning. Before and after, same moment, same machine:The staleness is genuine, verified independently of the probe:
The running bridge predates a merged change to its own source. A standing false positive was suppressing a true one — which is the worst form of this failure, because the warning that is permanently on is the one you learn to scroll past, and this probe exists to catch a real duplicate: two pollers racing for one Telegram
getUpdatescursor, which silently splits inbound owner messages between them.(The probe's own "324 min" and my independently-computed 74 min disagree; I have not reconciled the two reference points and am not claiming the magnitude, only the direction, which both agree on.)
The fix
Identify by PID scope, not by pattern: drop any matched pid that is the parent of another matched pid, keeping the leaf — the process actually doing the work. Agnostic to which launcher is in use (vault,
env, a shell wrapper).Two deliberate properties:
psfailing returns the input unchanged. Over-reporting a duplicate is recoverable; dropping a real second poller is not. Fail toward the noisier answer.Evidence
Mutation check — make the collapse a no-op and the new tests must fail:
Full existing health-check suite, by exit code:
Not in scope
The telegram staleness this uncovers is a restart, and that bridge is the owner's live comms path — his call, not something this PR does. Flagged to him separately.