fix(notify): report a generation-skewed daemon as unhealthy - #4153
Open
probepark wants to merge 1 commit into
Open
fix(notify): report a generation-skewed daemon as unhealthy#4153probepark wants to merge 1 commit into
probepark wants to merge 1 commit into
Conversation
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
4 times, most recently
from
August 10, 2026 07:16
5ab0f6f to
b9dde5e
Compare
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
from
August 10, 2026 07:34
b9dde5e to
864b8c0
Compare
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
2 times, most recently
from
August 10, 2026 09:39
1c40658 to
7e5e0b1
Compare
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
from
August 10, 2026 11:02
7e5e0b1 to
ea77994
Compare
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
4 times, most recently
from
August 10, 2026 13:24
cd4265f to
d5f6e7f
Compare
Yeachan-Heo
pushed a commit
that referenced
this pull request
Aug 10, 2026
…me-process leaks The Discord fake-provider retry/ambiguous-send row (PR #4153, CI run 31387018881) and the Slack ACK-boundary replay row (PR #4098, earlier runs) both flaked with a 1000ms ConversationLockTimeoutError on an isolated effects.json.lock inside their own test process. Root cause: a well-formed lock file whose recorded pid is still alive is unreclaimable by any waiter (staleness only covers dead pids and empty files), so a release that failed between close() and unlink() -- or any interrupted release -- turns every later acquire into a full 1000ms timeout. The failure reproduces deterministically by planting a live-pid lock file. Fix, shared by every ConversationStore user (conversation mappings and the effects.json ChatEffectJournal alike): - release is idempotent: a failed close() no longer skips the unlink, and the unlink is ownership-checked (pid+incarnation+timestamp+nonce) so it can only remove the lock this holder wrote - waiters self-heal same-process leaks: a well-formed lock recorded under this process's own pid with no live holder (module-level held set) is removed instead of waiting out the timeout - the 1000ms lock timeout is unchanged and live cross-process locks are never deleted Verified with 3 new regression tests in sdk-daemon-concurrency.test.ts, the full Discord (52) and Slack (49) daemon suites, and the 12-file lock-consumer shard batch (390 tests). Constraint: no timeout inflation -- the 1000ms lock window is untouched Constraint: no blind lock deletion -- only provably leaked same-process locks are removed Rejected: raising the lock timeout | masks the leak instead of fixing it Rejected: unconditional waiter-side unlink | could delete a live holder's lock Confidence: high Scope-risk: medium Reversibility: revert-safe Tested: leaked-lock heal, live same-process holder protection, cross-process lock preservation Not-tested: cross-process leak heal (holder process must exit first; unchanged behavior)
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
from
August 10, 2026 17:38
d5f6e7f to
dc3d380
Compare
Yeachan-Heo
pushed a commit
that referenced
this pull request
Aug 10, 2026
…me-process leaks The Discord fake-provider retry/ambiguous-send row (PR #4153, CI run 31387018881) and the Slack ACK-boundary replay row (PR #4098, earlier runs) both flaked with a 1000ms ConversationLockTimeoutError on an isolated effects.json.lock inside their own test process. Root cause: a well-formed lock file whose recorded pid is still alive is unreclaimable by any waiter (staleness only covers dead pids and empty files), so a release that failed between close() and unlink() -- or any interrupted release -- turns every later acquire into a full 1000ms timeout. The failure reproduces deterministically by planting a live-pid lock file. Fix, shared by every ConversationStore user (conversation mappings and the effects.json ChatEffectJournal alike): - release is idempotent: a failed close() no longer skips the unlink, and the unlink is ownership-checked (pid+incarnation+timestamp+nonce) so it can only remove the lock this holder wrote - waiters self-heal same-process leaks: a well-formed lock recorded under this process's own pid with no live holder (module-level held set) is removed instead of waiting out the timeout - the 1000ms lock timeout is unchanged and live cross-process locks are never deleted Verified with 3 new regression tests in sdk-daemon-concurrency.test.ts, the full Discord (52) and Slack (49) daemon suites, and the 12-file lock-consumer shard batch (390 tests). Constraint: no timeout inflation -- the 1000ms lock window is untouched Constraint: no blind lock deletion -- only provably leaked same-process locks are removed Rejected: raising the lock timeout | masks the leak instead of fixing it Rejected: unconditional waiter-side unlink | could delete a live holder's lock Confidence: high Scope-risk: medium Reversibility: revert-safe Tested: leaked-lock heal, live same-process holder protection, cross-process lock preservation Not-tested: cross-process leak heal (holder process must exit first; unchanged behavior)
Yeachan-Heo
pushed a commit
that referenced
this pull request
Aug 11, 2026
…me-process leaks The Discord fake-provider retry/ambiguous-send row (PR #4153, CI run 31387018881) and the Slack ACK-boundary replay row (PR #4098, earlier runs) both flaked with a 1000ms ConversationLockTimeoutError on an isolated effects.json.lock inside their own test process. Root cause: a well-formed lock file whose recorded pid is still alive is unreclaimable by any waiter (staleness only covers dead pids and empty files), so a release that failed between close() and unlink() -- or any interrupted release -- turns every later acquire into a full 1000ms timeout. The failure reproduces deterministically by planting a live-pid lock file. Fix, shared by every ConversationStore user (conversation mappings and the effects.json ChatEffectJournal alike): - release is idempotent: a failed close() no longer skips the unlink, and the unlink is ownership-checked (pid+incarnation+timestamp+nonce) so it can only remove the lock this holder wrote - waiters self-heal same-process leaks: a well-formed lock recorded under this process's own pid with no live holder (module-level held set) is removed instead of waiting out the timeout - the 1000ms lock timeout is unchanged and live cross-process locks are never deleted Verified with 3 new regression tests in sdk-daemon-concurrency.test.ts, the full Discord (52) and Slack (49) daemon suites, and the 12-file lock-consumer shard batch (390 tests). Constraint: no timeout inflation -- the 1000ms lock window is untouched Constraint: no blind lock deletion -- only provably leaked same-process locks are removed Rejected: raising the lock timeout | masks the leak instead of fixing it Rejected: unconditional waiter-side unlink | could delete a live holder's lock Confidence: high Scope-risk: medium Reversibility: revert-safe Tested: leaked-lock heal, live same-process holder protection, cross-process lock preservation Not-tested: cross-process leak heal (holder process must exit first; unchanged behavior)
A daemon whose generation is not this build's keeps heartbeating, so health called it OK -- while `isCurrentCompatibleOwner` refuses to attach a session transport to it, `acquireDaemonOwnership` answers `provisional`, and `ensureTelegramDaemonRunning` reports `blocked_identity`. No transport attaches and no notification is ever published. Liveness is not serviceability, and reporting it as OK is what made the outage invisible. Health now names the skew and the remedy. The generation stays inventory metadata that never forces a live reload on its own, so the operator does the reload. Lore-id: 4052c7ae Constraint: must not auto-reload on skew -- that inverts the documented contract at telegram-daemon-contract.ts:9-12 Rejected: auto-reload the daemon on generation skew | separate product decision, and it sits inside Yeachan-Heo#4117's file Rejected: treat liveness as serviceability | that is the defect Confidence: high Scope-risk: narrow Reversibility: easy Tested: removing the skew branch turns 60 pass / 0 fail into 59 pass / 1 fail Not-tested: an old-generation daemon does not self-heal; it still needs `gjc daemon reload`
probepark
force-pushed
the
fix/issue-4052-notify-delivery
branch
from
August 11, 2026 01:32
dc3d380 to
38d73b6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the diagnosability half of #4052 — "Telegram notifications fail despite healthy daemon after 0.12.16".
The defect
checkNotificationHealthtreated a live heartbeat as proof of service. It is not.When a running daemon's generation is not this build's:
isCurrentCompatibleOwnerrefuses to attach a session to that owneracquireDaemonOwnershipanswersprovisionalensureTelegramDaemonRunningreportsblocked_identityMeanwhile the owner keeps heartbeating, so health reported
ok — daemon pid N alive with a fresh heartbeat. That is precisely the reported symptom: notifications silently dead, daemon confidently healthy. The health check answered "is the process alive" when the operator was asking "will my notifications arrive".The fix
Health now detects generation skew and names both the cause and the remedy, instead of falling through to the
okbranch.packages/coding-agent/src/sdk/bus/notification-service.ts:880-892.What this deliberately does not do
It does not auto-reload a skewed daemon.
telegram-daemon-contract.ts:9-12documents that a generation bump is inventory metadata and does not force a live reload; inverting that is a product decision, not a bug fix, and it would land inside #4117's file. So the outage becomes visible and the operator getsgjc daemon reload— the daemon still does not self-heal. That limitation is stated in the commit'sNot-testedtrailer rather than hidden.Verification
bun test packages/coding-agent/test/notifications-service.test.tsflags a live daemon whose generation this build refuses to attach a transport to)bun --cwd=packages/coding-agent run check:typesorigin/dev135ae3e2c)The mutation was run by me against the committed branch, not taken from the implementing lane's self-report.
Generation guard
No bump needed. The change is in
notification-service.ts, not undersdk/bus/telegram-*, andcheckNotificationHealthis not in that file's 11-symbol allowlist —daemonGenerationRelationis consumed byte-identically, so no manifest hash moves.