fix(notifications): re-arm expired Telegram topic leases for owned trusted attachments (#4647) - #4648
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
06bc1a6 to
2adf6f7
Compare
|
Review request — exact head @probepark @snowykr — independent exact-head review requested. This PR closes #4647 (Telegram forum topic opens, identity posts, then every later frame is rejected pre-send with What needs review (one-item change,
Local verification at this exact head: 266 focused Telegram tests pass (lease-renewal, daemon, topic-registry, fault-containment, owner-postmortem, rich-draft, rich-redteam, live-stream, reapers, baseline-manifest, reference, adoption-intent), Current PR verdict is — |
|
CI triage at exact head
Action: rerun of the failed/cancelled jobs has been triggered. The lease-renewal suite itself is green locally at this head (6/6) and was green in the earlier CI attempt's sibling shards. — |
2adf6f7 to
ffd3a20
Compare
|
Rebased delivery — exact head Rebase notes: #4616 touched Hermetic validation at this exact head:
@probepark @snowykr — fresh exact-head review requested at — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review — merge blocked (production change is correct; the new test violates the repo contract).
minor — test file breaks AGENTS.md conventions
packages/coding-agent/test/notifications-telegram-topic-lease-renewal.test.ts:54-69 (and repeated later) introduces any types and readFileSync/writeFileSync.
AGENTS.md: no any unless absolutely necessary; file I/O goes through Bun.file() / Bun.write(). Define narrow fixture types and switch to the Bun APIs.
otherwise
The renewal deadlock is repaired at its source and the ordinary lease gate is rechecked after mutation. Foreign owners, replaced sockets, grace, and archive fences are all covered, and the expiry/heartbeat/grace tests genuinely fail without the production change.
|
Exact-head CI complete at Status: OWNER_CONFIRMATION_REQUIRED — merge is blocked only on an independent exact-head approval. @probepark @snowykr: a fresh review at Earlier infra note for the record: the first CI attempt had three shard jobs cancelled by a runner apt-mirror stall ( — |
ffd3a20 to
0d02b1d
Compare
|
Review feedback applied — new exact head @probepark's findings addressed in full:
Re-verified at this head: lease-renewal 6/6 (still discriminating), 130 tests green across lease-renewal + topic-registry + live-stream + daemon + fault-containment + baseline-manifest, @probepark @snowykr — fresh exact-head review requested at — |
…ts expired topic lease After Router replay flips logicalSessionIdTrusted=true, every later frame required #leaseAllows, which demands a non-expired topic host lease. But renewActiveTopicLeases only renewed sessions that already passed that same gate, so once the 20s HEARTBEAT_TTL_MS lapsed the lease was permanently unrenewable: identity published, then all later frames died pre-send with "trusted attachment lease is stale" and the forum topic froze (#4647). Split authority ownership from lease TTL: #leaseAllows gains an explicit allowLeaseRearm mode used only at the two renewal sites (ownership heartbeat and once before the pre-send gate). It admits exactly what acquireLease admits — an expired-but-still-owned active lease, or a same-owner resume inside the disconnect-grace window (the incident record was persisted in disconnect_grace) — and nothing else. Dropped sessions, closed endpoints, foreign lease owners, archive-fenced/inactive/quarantined topics, malformed bindings, chat-binding mismatches, and cross-session owner checks all still fail closed. Daemon generation bumped 169→170 via the generation guard. Lore-id: 8f2c1d47 Constraint: must not weaken cross-session or recovery ownership fencing Constraint: socket/Router authority boundaries unchanged Rejected: dropping the TTL check from #leaseAllows entirely | would admit foreign expired owners Rejected: re-arm only in the heartbeat | a frame can arrive first after a long idle batch Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 6-case lease renewal regression suite; 230+ focused Telegram suites green; discriminating (fails 3/6 without the fix) Not-tested: live Bot API end-to-end (hermetic BotApi contract) Closes: #4647
0d02b1d to
ee1896b
Compare
|
Dev advanced again (#4585 merged), so the delivery is rebased once more: exact head Re-verified at @probepark @snowykr — review requested at this exact head; verdict remains truthful — |
|
Replacement CI terminal at exact head Status: OWNER_CONFIRMATION_REQUIRED — @probepark's prior — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head ee1896b8 — approved. My prior blocking findings are fixed and the lease logic holds under adversarial reading.
prior findings resolved
packages/coding-agent/test/notifications-telegram-topic-lease-renewal.test.ts no longer contains any, readFileSync, or writeFileSync — zero occurrences at this head.
the re-arm gate is genuinely narrow
Re-arm requires all of: a current trusted attachment, an authorized recovery lease, exact logical-session ownership, matching chat/binding, the same installationHostId, a non-malformed durable record, and either active authority or an unexpired disconnect_grace. Untrusted and unowned attachments cannot reach either re-arm callsite through any branch.
TOCTOU — checked, and it is defended
The attachment predicate is evaluated before the write rather than atomically with it, which is where I expected to find the bug. It survives because the actual mutation is TopicRegistry.acquireLease, which is serialized, rechecks durable authority itself, and CAS-retries against the winning registry generation. Another unexpired host cannot be displaced, and the ordinary publication gate is re-run after the mutation before anything is sent.
Worth stating plainly for the record: installationHostId is host-scoped, so a same-host split-brain is not distinguished by the topic lease at all. Correctness there still rests entirely on the daemon ownership lock and orphan fencing. This PR does not weaken that, but it also does not extend protection to that case — don't let this change be read as covering it.
expiry boundary
Wall-clock Date.now() via NotificationOperatorRuntime.now, not monotonic. Active leases are stale at leaseExpiresAt <= now; same-owner re-arm works both exactly at expiry and after an arbitrarily large delay. Grace re-arm requires disconnectGraceExpiresAt > now, so equality is rejected — one tick stricter than acquireLease itself, which rejects only < now. Stricter-than-the-primitive is the safe direction, so this is fine.
generation manifest is consistent
scripts/telegram-daemon-generation-manifest.json is structurally current for this diff: the three changed protected AST declarations are exactly DAEMON_GENERATION, TelegramNotificationDaemon.#leaseAllows, and handleSessionMessage; renewActiveTopicLeases is not in the protected inventory. Generation 170 and its exact fixture assertion were both updated, which is why the generation guard is green.
nit — ReturnType<> in the new harness
test/notifications-telegram-topic-lease-renewal.test.ts:103 uses ReturnType<typeof fakeBot>. AGENTS.md says never; a named result type shared by fakeBot and LeaseHarness is the intended shape. I am explicitly not blocking on it — the pattern already appears 227 times across packages/*/src and packages/*/test with no mechanical gate, so blocking one new instance would be inconsistent enforcement, not a standard. Fix it while you're in the file.
coverage
Three of the six new behavioral tests fail with the daemon change reverted and genuinely pin the fix: expiry-then-publish (150-178), post-expiry ownership heartbeat extending without publishing (180-198), and in-grace resume to active (250-278). The foreign-host, replaced-attachment, and archive-fence tests pass on base — they are safety guards, not pins, which is fine as long as nobody counts them as evidence the fix works.
Reviewed by @probepark — method: detached worktree at ee1896b8, full read of the changed regions of telegram-daemon.ts and telegram-daemon-contract.ts, ownership-predicate trace from gate to acquireLease CAS, expiry-boundary comparison against the primitive, manifest/protected-inventory cross-check, per-test would-this-fail-on-base analysis. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:b12f972b2dacf3ebf0c2796f9b29463603db253f1feb2a03d373edde77399fa9 reviewer:human reviewer-id:probepark evidence:exact-head-ee1896b8-independent-maintainer-review-rearm-gate-and-acquirelease-cas-verified
…usted attachments (Yeachan-Heo#4648) An owned topic lease that expired could never be re-armed, so trusted attachments silently stopped publishing until the daemon restarted. Re-arm is now gated on the full attachment predicate and executed through TopicRegistry.acquireLease, whose serialized CAS retry keeps another unexpired host from being displaced. Closes Yeachan-Heo#4647 Reviewed-by: probepark Verdict: merge-approved sha256:b12f972b2dacf3ebf0c2796f9b29463603db253f1feb2a03d373edde77399fa9 Confidence: high Scope-risk: narrow Tested: expiry-then-publish, post-expiry ownership heartbeat, in-grace resume to active Not-tested: same-host split-brain (still covered only by the daemon ownership lock) (cherry picked from commit d519c3e)
…oes not ship Cherry-picking Yeachan-Heo#4648 hit a CHANGELOG conflict whose union resolution also pulled in the neighbouring `gjc team` worker-runtime-state bullet from Yeachan-Heo#4603. That commit is deliberately excluded from the 0.14.1 pick set, so the bullet advertised a fix the release does not contain. Lore-id: 4c1f9a02 Constraint: Unreleased bullets must describe exactly the approved pick set Confidence: high Scope-risk: narrow Reversibility: trivial Tested: re-audited every Unreleased bullet against branch commit provenance
2 participants
Closes #4647
Reproduction (from source, current origin/dev)
Ordinary interactive Telegram session with Threaded Mode:
logicalSessionIdTrusted = false;#leaseAllowsreturns true there, socreateForumTopic+ identity header send fine.logicalSessionIdTrusted = true. From then on#leaseAllowsrequires an authorized recovery lease plus a non-expired topic host lease (leaseExpiresAt > now, TTL =HEARTBEAT_TTL_MS= 20s).renewActiveTopicLeases()only renewed sessions that already passed#leaseAllows— once the host lease expired, renewal was skipped forever (deadlock).handleSessionMessagethen rejected every laterturn_stream/context_update/tool frame pre-send withTelegram publication rejected before send: trusted attachment lease is stale.Result: topic opens, identity (
:1:2) is the only delivered publication, topic frozen. Exactly the reported 0.14.0 incident (daemon gen 169; incident record persisted withauthorityState: \"disconnect_grace\").A discriminating regression suite reproduces this: with the fix reverted, 3 of 6 tests fail (post-TTL publish, heartbeat re-arm, grace resume).
Fix (smallest correct, fail-closed)
Split authority ownership from host-lease TTL in
#leaseAllowsvia an explicit{ allowLeaseRearm: true }mode, used at exactly two renewal sites:renewActiveTopicLeases()(ownership heartbeat)handleSessionMessagebefore the pre-send gate, then the gate is re-checkedThe mode admits only what
TopicRegistry.acquireLeaseitself admits for a live owner:activelease (leaseOwner === installationHostId), ordisconnect_grace+ unexpireddisconnectGraceExpiresAt) — which also covers the incident's persisted record.Everything else still fails closed: dropped sessions, closed endpoint bindings, unauthorized/pending/rejected recovery leases, exact-logical-owner mismatch (
#logicalSessionOwners), foreignleaseOwner,archive_pending/archive_exhausted/inactive/legacy_quarantinedstates,bindingMalformed, and chat-binding mismatch. After a successful re-arm the ordinary gate re-runs unchanged, so truly unauthorized/stale attachments still get the pre-send rejection. Socket/Router authority is untouched;#renewTopicLease→topics.acquireLeaseremains the single mutation path (another unexpired host is still never displaced).The reporter's suggested mechanism ("renew an owned topic lease even after expiry, then re-check") was correct but incomplete: the incident's durable record sits in
disconnect_grace, and#leaseAllowswould still reject an owned-but-grace topic even with TTL ignored. The implemented invariant is exactlyacquireLease's admission rule (owned-active OR owned-grace-in-window), documented in-code.Regression coverage (
notifications-telegram-topic-lease-renewal.test.ts, 6 cases)leaseOwner: \"another-installation\") is never re-armed; pre-send rejection; durable record untouchedactive, fresh lease) by its still-attached ownerarchive_pendingValidation
bun test packages/coding-agent/test/notifications-telegram-topic-lease-renewal.test.ts— 6 pass (3 fail pre-fix)notifications-telegram-topic-lease-renewal.test.ts6/6 (3 fail with the fix reverted),notifications-topic-registry+notifications-live-stream+session-manager-resident-cachegreen (89 pass across the four), daemon + fault-containment + baseline-manifest 44 passtelegram-daemon-generation-guard:v51 required generation bump verifiedagainst27afb732b3...ee1896b8f7(DAEMON_GENERATION 169→170)bun run --cwd=packages/coding-agent check:typesclean;biome checkclean on touched files;git diff --checkcleanverify-gjc-state-writers --fail(0 unsanctioned writes) andchangelog-history-guard(no released sections removed) PASSBinary diff SHA-256
27afb732b3d25632d44176687d5bdd78d3419bb3ee1896b8f737c5ff3142f77ca88d1c53bad6f3dd(three-dot binary diff SHA-256b12f972b2dacf3ebf0c2796f9b29463603db253f1feb2a03d373edde77399fa9)GJC verdict
—
[repo owner's gaebal-gajae (clawdbot) 🦞]