fix(telegram): release timed-out spooled lanes#93040
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 4:50 PM ET / 20:50 UTC. Summary PR surface: Source +3, Tests +9. Total +12 across 2 files. Reproducibility: yes. at source level: current main and the current PR head keep update 43 pending when update 42 times out and the old same-lane handler ignores reply abort. I did not run live Telegram fault injection in this read-only review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Rebase and implement a maintainer-approved hard cancellation or process-restart boundary so a timed-out failed update cannot continue concurrently while later same-lane updates can eventually drain. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main and the current PR head keep update 43 pending when update 42 times out and the old same-lane handler ignores reply abort. I did not run live Telegram fault injection in this read-only review. Is this the best way to solve the issue? No for the current head. It codifies the existing blocked-lane outcome, while the earlier release-after-grace approach was already rejected without a hard cancellation or process-restart boundary. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 44e88f550b69. Label changesLabel justifications:
Evidence reviewedPR surface: Source +3, Tests +9. Total +12 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Proof update: I tried the native Telegram proof path, but this specific recovery cannot be triggered by the current visible-chat Mantis runner. What ran:
Result:
I also reran the focused production-path regression locally after updating the PR body: The specific passing case is Given the native runner limitation, I think this PR needs maintainer acceptance of the focused production-path synthetic proof, or an explicit proof-gate override, rather than another rerun of the same Mantis path. |
|
@clawsweeper re-review |
obviyus
left a comment
There was a problem hiding this comment.
Thanks for pushing on this. I do not think this is safe to land in its current shape.
The patch releases the Telegram lane after the timeout grace even when the old same-lane handler is still running. That fixes the permanent stuck-lane symptom, but it can let update N and update N+1 execute concurrently in the same DM/topic lane if the old handler ignores abort. That breaks the serialization invariant and can produce late replies, duplicate/out-of-order sends, or stale session writes.
What I would expect from a root-cause fix: fail the timed-out update, abort reply work, then establish a hard cancellation boundary for the old handler before releasing the lane. In practice that means making the handler execution killable or forcing a clean gateway/process restart boundary, so later same-lane updates only drain after the previous handler is actually dead. A TTL-style lane release is a mitigation, not the root fix.
|
Closing this branch. I compared the current head ( So the current head is a no-op relative to Closing this PR rather than leaving a stale/no-op branch open. Leaving #91456 open for a follow-up design that implements an actual hard-stop boundary. |
Summary
AI-assisted: prepared with Codex.
Change Type
Scope
Linked Issue/PR
Motivation
Telegram isolated ingress already detects spooled handler timeouts, marks the timed-out update failed, and aborts active reply work for the lane. However, if the handler ignores that abort, the lane remains present in
activeSpooledUpdateHandlersByLane, so every later same-lane update stays blocked. The issue report includes a live repro where this persisted across provider auto-restarts for many hours and only cleared after a full gateway restart.Real behavior proof
Observed result after fix: the regression test fails update
42, releases the old lane guard after the 100 ms abort grace window, restarts isolated ingress, drains later same-lane update43, and leaves no pending spooled updates.What was not tested: a live Telegram Bot API outage/timeout. The covered behavior is the isolated-ingress lane registry and spool-drain recovery path with synthetic spooled updates.
Native Telegram proof attempt: Mantis Telegram Desktop Proof ran against baseline
6134c00657a24and candidatef30bbd80ecbd, but the runner could not capture this recovery from visible chat input. Run: https://github.com/openclaw/openclaw/actions/runs/27507856635Mantis evidence summary: the generated manifest says this recovery requires a timed-out Telegram reply handler that keeps running after abort, which the native proof runner cannot trigger from chat input.
Local re-check on June 14, 2026: reran
node scripts/run-vitest.mjs extensions/telegram/src/polling-session.test.ts --reporter=verbose; result wasTest Files 1 passed (1),Tests 58 passed (58), with the newreleases a timed-out lane when the old handler ignores reply abortcase passing.Root Cause
restart: falsewhen the handler ignored abort, intentionally keeping the active handler registered as the lane guard.Regression Test Plan
extensions/telegram/src/polling-session.test.tsUser-visible / Behavior Changes
Security Impact
Verification
Notes:
pnpm installinside the fresh worktree and aborted because there was no TTY for module purge confirmation, so the commit was made with hooks bypassed after the explicit focused test andgit diff --checkboth passed.