fix(notifications): report why the Telegram daemon child exits before readiness (#3761) - #3844
Conversation
|
Maintainer triage (external review): the change itself is sound and CI is green on the exact head (
(CHANGELOG and the manifest auto-merge, but the guard requires a bump per committed range.) Please rebase onto the current
The diagnostics implementation itself was verified against |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES — one blocker; the implementation itself is sound.
Blocker — DAEMON_GENERATION 50 is already claimed on dev by #3834 (merged 2026-08-05T06:31:15Z); this PR cannot merge into current dev as-is.
Verified independently (read-only, exact head f9a7532c):
- GitHub reports
mergeable: CONFLICTING,mergeStateStatus: DIRTYfor this head againstdev. - Current
devtelegram-daemon-contract.tsalready containsexport const DAEMON_GENERATION = 50;(claimed by #3834, the symlinked-notifications activation slice of the same issue). This PR bumps 49 → 50 on its own base, so bothtelegram-daemon-contract.tsand the generation pin intest/notifications-telegram-daemon.test.tsconflict withdev. - The
telegram-daemon-generation-guardrequires a strictly higherDAEMON_GENERATIONper committed range whenever a protected Telegram lifecycle declaration changes.run,renewDaemonHeartbeat, andrunDaemonInternalare all protected and all change here. Rebased onto currentdev, the range would be 50 → 50, so the guard throws (protected Telegram lifecycle change requires a strictly higher DAEMON_GENERATION). CI is green on the exact head only because the PR's own base was generation 49 (50 > 49).
Required fix (mechanical — no functional change requested):
- Rebase onto current
dev. - Bump
DAEMON_GENERATION50 → 51 and document generation 51 in the contract comment. - Update the pin test name/comment and
expect(DAEMON_GENERATION).toBe(51). - Regenerate the manifest (
bun scripts/telegram-daemon-generation-guard.ts --write-manifest) so it byte-matches the rebased head tree, then--validate-current-treeand runbun test scripts/telegram-daemon-generation-guard.test.ts. - Re-run the focused suites (
issue-3761-daemon-startup-diagnostics.test.ts,notifications-telegram-daemon.test.ts,notify-setup.test.ts) andbun --cwd=packages/coding-agent run check.
What I verified and found sound (no other changes requested):
describeDaemonHeartbeatRefusalmirrors the 14-conditionrenewDaemonHeartbeatadmission guard 1:1 in order — everyreturn falsesite in the guard has a matching named refusal, andcanBindProvisionalPidis computed identically. The guard itself is unchanged and stays authoritative;renewDaemonHeartbeat's boolean return contract is untouched (onRefusalis an optional addition, and the daemon is its only caller).- Security/privacy: the owner id (which doubles as the acquisition secret) is never written — only its pid is reported via
ownerPidFromOwnerId. The bot token never appears in any diagnostic line; the existingsanitizeDiagnostic(exact + token-shape redaction) and the newsanitizeDaemonDiagnostic(\d{6,}:[A-Za-z0-9_-]{20,}redaction) are defense-in-depth on the durable line.daemon.logis opened0o600inside a0o700directory. - Stderr boundary: the stderr sink is wired only at the
notify daemon-internalprocess boundary (the launcher already redirects stdout/stderr intonotifications/daemon.logindefaultDaemonSpawn). The embeddableTelegramNotificationDaemonnever writes to a host process's stderr without an explicit sink, and the sink call is try/caught so a diagnostic can never break the exit path it describes. - Contract boundaries:
RunDaemonInternalDeps.diagnostic,TelegramDaemonOptions.diagnostic, andrenewDaemonHeartbeat.onRefusalare all optional; wire protocol version stays 3. - Generated artifacts:
telegram-daemon-generation-manifest.jsonbyte-matches the PR head tree (guard--validate-current-treepasses); the CHANGELOG entry is accurate and under[Unreleased]. - CI on the exact head: all runs green — Telegram daemon generation guard,
gjc-state-gates,check:coding-agent,root-check, the three touched test suites, and the newissue-3761-daemon-startup-diagnosticssuite. - Local verification: 12/12 new diagnostics tests pass (including the real-subprocess test asserting the line lands in
daemon.logwithout the acquisition secret); the modifiednotify-setupandnotifications-telegram-daemoncases pass; no dangling references to the removedrecordDaemonCompatibilityDiagnostic.
f9a7532 to
489ba88
Compare
|
Rebased onto
Re-verified the admission mirror against the new base: Verdict in the PR body is rebound to the exact new head: |
489ba88 to
1329a0f
Compare
|
Re-verified on the new base:
Verdict rebound to the exact head: |
45c155c to
3b4698e
Compare
|
Generation re-claim plus three real code merges this time (#3828 touched the same call sites), all union merges with no behavior dropped from either side:
Re-verified on the new base (
The PR body verdict line is re-pinned to the exact head One process note rather than a code request: this is the third generation collision on the same issue (#3834 took 50, #3828 took 51). The guard's per-range bump requirement makes the counter a serialized resource, so any daemon-touching PR that merges first forces a rebase here even when the diagnostics change itself is untouched. This slice is behind two others by construction; if further daemon work is queued ahead of it, I will keep re-claiming, but merging it before the next daemon PR would stop the treadmill. |
|
Re-review request — the REQUEST_CHANGES blocker is resolved at the current head and no rebase is pending. The blocker was the generation-50 collision with #3834. That is settled: Re-verified just now against
Every item in the required fix list is satisfied at this head; the only thing still blocking is the CHANGES_REQUESTED review recorded against the older head The treadmill note from the previous comment still stands: this slice has now re-claimed the generation counter three times (#3834 took 50, #3828 took 51). It is currently the only daemon-touching branch that is green and conflict-free, so merging it before the next daemon PR ends the cycle. |
Generation 52 is already claimed by the pre-readiness daemon-child exit diagnostics slice (Yeachan-Heo#3844), which is ahead of this branch in review, so this slice claims 53 instead of guaranteeing a guard failure for whichever PR lands second. Lore-id: 5a3c91d7 Confidence: high Scope-risk: narrow Reversibility: easy Tested: telegram-daemon generation guard against current dev
* fix(ask): show multi-select state on remote asks The ask tool owns the multi-select loop and re-issues one remote request per toggle, but AskAnswerRequest carried no selection state: Telegram posted an identical prompt each time, so nothing showed that option 1 had been picked. Selection rendering existed only for durable workflow gates, which take a different presentation path. AskAnswerRequest now carries multi plus the selected option labels, the interactive presentation forwards them so the bus publishes selectedOptionIndices and the "(N selected)" prefix, and a presentation that brings its own navigation control keeps it instead of the gate's synthesized Done. Option renumbering now looks past a leading selection marker so pre-numbered deep-interview options render as "1. checked Alpha" rather than "1. checked 1. Alpha". Lore-id: b48e2d17 Confidence: high Scope-risk: narrow Reversibility: easy Tested: arbiter publication after a toggle, ask-tool remote requests across a toggle, daemon rendering of pre-numbered options Not-tested: live Telegram client rendering of the checkbox glyphs * test(notifications): pin the daemon generation to 53 Generation 52 is already claimed by the pre-readiness daemon-child exit diagnostics slice (#3844), which is ahead of this branch in review, so this slice claims 53 instead of guaranteeing a guard failure for whichever PR lands second. Lore-id: 5a3c91d7 Confidence: high Scope-risk: narrow Reversibility: easy Tested: telegram-daemon generation guard against current dev * test(notifications): stop the shutdown-admission join from racing wall-clock `effects.join` counts its deadline from the call, and the held-state race plus its assertion run inside that window before the persistence barrier is released. A 100 ms budget therefore reports `false` for a barrier that did hold as soon as the runner is loaded: shard-1-of-8 failed exactly this assertion on head 6d1c1b6, while the same shard passed on 160159c where every source file on that path is identical. The held half of the contract is proven by the race, not by the deadline, so the deadline only has to outlast the released work. The daemon's own shutdown join uses 1s; 5s here keeps the assertion load-bearing (a barrier that never settles still fails) without timing out on a slow runner. Lore-id: 9b2d5e14 Confidence: high Scope-risk: narrow Reversibility: easy Tested: assertion still fails when the released work never settles; suite green pinned to one core --------- Co-authored-by: yazzang-homelab <yazzang-homelab@users.noreply.github.com>
3b4698e to
f51d455
Compare
|
Rebased onto current What happened: the blocker review asked for a generation that does not collide on The rebase was not clean and I did not let git guess:
Re-verified on the new base:
The diagnostics code itself is unchanged from the head you reviewed as sound; only the generation claim, the two pins, the contract paragraph, and the manifest digests moved. Re-review requested — no rebase is pending and |
|
The blocker is resolved on the current head — this is now rebased onto Exact head
No functional change from the reviewed revision — the rebase carried the generation bump and the manifest regeneration only. |
f51d455 to
3b65a82
Compare
|
Rebased onto current New exact head
@Yeachan-Heo ready for re-review against this head. |
|
지금 한쪽을 55로 올려두는 건 의미가 없다 — dev가 움직이면 다시 어긋난다. 머지 순서만 정해주면 두 번째 것이 리베이스하면서 55로 올리면 된다. #3891이 먼저 가면 내가 이쪽을 55로 올리겠다. |
A detached `notify daemon-internal` child exited with status 0, an empty notifications/daemon.log, and no reachable diagnostic on every startup refusal, so a failed activation was indistinguishable from a daemon that was never spawned (Yeachan-Heo#3761). `logger` is the wrong channel here: a child that exits during startup never flushes it. The launcher already redirects the child's stdout/stderr into notifications/daemon.log, so the daemon-internal entrypoint now passes that stderr as a diagnostic sink and every pre-readiness exit writes one credential-free line to it, plus a notice once ownership reaches ready. renewDaemonHeartbeat reports the exact refusing condition instead of a bare false; its admission guard is unchanged and stays authoritative. Lore-id: 3761-diag Constraint: the owner id doubles as the acquisition secret -- diagnostics carry only its pid Constraint: the embeddable daemon class must not write to a host process's stderr Rejected: logger-only diagnostics | a startup-exiting child never flushes that sink Rejected: a new telegram-daemon.diagnostics.json writer | racy, and the log fd is already wired Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/issue-3761-daemon-startup-diagnostics.test.ts (12 pass, includes a real subprocess whose stderr is redirected into daemon.log) Tested: notifications-telegram-daemon (514 pass), notify-setup, generation guard base..head Not-tested: live Linux Telegram bot activation and inbound round trip (no maintainer credentials)
3b65a82 to
b2be479
Compare
|
Closing this contribution under maintainer direction. Do not open further PRs or issues in this repository without explicit maintainer approval. — |
|
현재 앞선 head에서 이 PR의 항목이 이미 배포된 이제
참고: 내 워크트리의 프리빌드 natives 애드온이 리베이스로 들어온 새 natives 소스와 ABI가 안 맞아 로컬에서 2건이 #3941(CHANGELOG 이력 가드)이 방금 머지됐으므로, 이제 이런 상태는 CI에서 자동으로 걸린다. |
|
This item is being moved to the Tier 3 review lane. Further review is deferred until after the current emergency stabilization period. — |
2 participants
What
Every pre-readiness exit of the detached Telegram notification daemon child now writes one credential-free line explaining why it exited, on the child's own stderr — the fd the launcher already redirects into
notifications/daemon.log— plus a notice once ownership reachesready.Covered exits:
--owner-idis not alivelogger.warnonly (never flushed by a startup-exiting child)config.ymldoes not enable a complete Telegram providerTelegramNotificationDaemon.run)renewDaemonHeartbeatfalse), except twologger.warnpathsreadyrenewDaemonHeartbeatgains an optionalonRefusal(reason)and reports the exact refusing condition (contended/unusable transition lock, which state field mismatched, lock rebind failure, state write failure, sidecar proof failure, ready-publication failure). The admission guard is unchanged and stays authoritative —describeDaemonHeartbeatRefusalonly names the first failing condition for the message, and falls back tounspecified mismatch.Secret handling: the owner id doubles as the acquisition secret, so diagnostics carry only the pid derived from it, never the id; token-shaped values are redacted by
sanitizeDaemonDiagnostic. The stderr sink is wired only at thenotify daemon-internalprocess boundary, so the embeddable daemon class never writes to a host process's stderr (asserted by a test).DAEMON_GENERATION51 → 52 (rebased ontodevagain after #3828 claimed 51; #3834 had claimed 50), contract documented, generation pin test updated, manifest regenerated.Why
Refs #3761— the diagnostic slice, which needs no Telegram credentials. Reported symptom:notify daemon-internal"exits immediately with status 0 and no output on either stream",notifications/daemon.log0 bytes, and (follow-up comment)run()returning in 4 ms fromrenewDaemonHeartbeatwith no log line, which made the whole activation failure unobservable from outside. The issue asks for exactly this: "even if the ownership refusal is correct, it should not be silent. A one-line reason on the child's stderr (which the launcher can capture intonotifications/daemon.log) would have turned this whole investigation into one command."loggeris the wrong channel here — a child that exits during startup never flushes it. This does not claim to fix the activation failure itself, does not auto-close the issue, and is independent of the #3834 symlink hypothesis and of the owner-required live reproduction gate. It makes the remaining gate cheap to satisfy: the required "child exit/startup diagnostics" and "ownership transition from provisional to ready" evidence now falls out ofnotifications/daemon.log.Slice ledger for #3761: #3763 (merged, wording) and #3768 (merged, recovery) left the child-side diagnostic bullet of the issue's "Tests worth adding" unclaimed; this is that bullet.
Testing
bun test packages/coding-agent/test/issue-3761-daemon-startup-diagnostics.test.ts— 12 pass. Includes a real subprocess whose stdout/stderr are an fd onnotifications/daemon.log(the exact production wiring), asserting the reason is readable from the file after the child is gone and that the owner-id secret is absent.bun test issue-3761-daemon-startup-diagnostics.test.ts notifications-telegram-daemon.test.ts notify-setup.test.ts— 620 pass, 0 fail on this head (includes the generation-52 pin).bun scripts/telegram-daemon-generation-guard.ts --validate-current-tree→ OK;bun scripts/telegram-daemon-generation-guard.ts 844002de8990b46ee43452759938de1820b3cce8 3b4698e727e9f12410fadd8ba9f26c908e22b1c0→v43 required generation bump verified;bun test scripts/telegram-daemon-generation-guard.test.ts— 48 pass.bun --cwd=packages/coding-agent run check— biome clean,tsc --noEmitclean.bun test packages/coding-agent/test/sdk-operation-inventory.test.ts— 17 pass on this head.GJC verdict
No independent architect/critic review was performed; author self-review only. Needs maintainer review.
devbun checkpasses