fix(telegram): durably retry inbound media dropped during restart (#98076)#98102
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 4:00 PM ET / 20:00 UTC. Summary PR surface: Source +36, Tests +173, Generated 0. Total +209 across 5 files. Reproducibility: yes. source-level only: current main catches single-message media fetch failure, sends a best-effort warning, releases dedupe keys, and returns while bot-core completes the update unless failed-retryable is recorded. A live Telegram restart-window reproduction is still missing. 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:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land a rebased Telegram-local retry fix that reverts the stale generated baseline, keeps the linked issue open until merge, and includes redacted live Telegram restart proof for the document retry path. Do we have a high-confidence way to reproduce the issue? Yes, source-level only: current main catches single-message media fetch failure, sends a best-effort warning, releases dedupe keys, and returns while bot-core completes the update unless failed-retryable is recorded. A live Telegram restart-window reproduction is still missing. Is this the best way to solve the issue? Mostly yes for owner boundary: the latest head keeps the retry policy Telegram-local and uses the existing failed-retryable spooled replay contract. It is not merge-ready until the stale generated baseline is reverted and real restart proof confirms the observed loss path. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 44cb3f7b980f. Label changesLabel justifications:
Evidence reviewedPR surface: Source +36, Tests +173, Generated 0. Total +209 across 5 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
|
openclaw#98076) Review on openclaw#98102: the durable-retry classifier treated every MediaFetchError `fetch_failed` as retryable, but `fetch_failed` also covers permanent failures (SSRF/guard denials, local Bot API path/read errors). On spooled replay those would requeue forever with the user-facing warning suppressed. Move the policy to its owner: add `isDurablyRetryableMediaFetchError` to `src/media/fetch.ts`, defined as `shouldRetryMediaFetch` (the canonical in-loop transient policy) plus shutdown/abort `fetch_failed`. A restart-window abort is the primary inbound-media loss vector and is recoverable on replay, whereas in-loop retry mid-shutdown is futile; permanent `fetch_failed`, other 4xx, and size limits stay non-retryable so they cannot loop in the spool. Telegram reuses the shared helper instead of a local classifier (`isAbortError`/`isTransientNetworkError` live in core; the classification must not drift). Adds permanent-`fetch_failed` regression coverage at the unit and behavioral level. Media-group partial delivery (openclaw#55216) unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Addressed the P2 finding on The policy is moved to its owner and narrowed. New
Telegram now reuses the shared core helper instead of a local classifier, since On proof: a live Telegram restart-during-upload rig and the agentic native Telegram proof job need maintainer/write perms I don't have as a contributor. The behavior is asserted at the source level against the real handler + bot-core completion frame; a maintainer can capture visible proof via Mantis if wanted. @clawsweeper re-review 🤖 Addressed by Claude Code |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…enclaw#98076) A single-message inbound media fetch failure was handled best-effort: warn and let the update complete, so bot-core acked the spooled update. During a restart window the warning send also failed and the document was silently lost with no durable retry. Classify transient media fetch failures (network/abort fetch_failed and 408/429/5xx HTTP) and record them as failed-retryable, so spooled-replay updates are kept and re-driven by the ingress spool with backoff instead of acked. Live updates still ack and warn; permanent failures (size limit, 4xx) stay best-effort. The retry notice is suppressed on spooled replay to avoid repeating it on each replay. The durable-retry classifier deliberately diverges from media/fetch.ts shouldRetryMediaFetch on shutdown aborts: in-loop retry of a shutdown abort is futile, but durable re-spool must retry it -- that abort is the primary restart-window loss vector. Media-group partial delivery (openclaw#55216) is left unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
openclaw#98076) Review on openclaw#98102: the durable-retry classifier treated every MediaFetchError `fetch_failed` as retryable, but `fetch_failed` also covers permanent failures (SSRF/guard denials, local Bot API path/read errors). On spooled replay those would requeue forever with the user-facing warning suppressed. Move the policy to its owner: add `isDurablyRetryableMediaFetchError` to `src/media/fetch.ts`, defined as `shouldRetryMediaFetch` (the canonical in-loop transient policy) plus shutdown/abort `fetch_failed`. A restart-window abort is the primary inbound-media loss vector and is recoverable on replay, whereas in-loop retry mid-shutdown is futile; permanent `fetch_failed`, other 4xx, and size limits stay non-retryable so they cannot loop in the spool. Telegram reuses the shared helper instead of a local classifier (`isAbortError`/`isTransientNetworkError` live in core; the classification must not drift). Adds permanent-`fetch_failed` regression coverage at the unit and behavioral level. Media-group partial delivery (openclaw#55216) unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
0e76275 to
391f80f
Compare
#98076) Review on #98102: the durable-retry classifier treated every MediaFetchError `fetch_failed` as retryable, but `fetch_failed` also covers permanent failures (SSRF/guard denials, local Bot API path/read errors). On spooled replay those would requeue forever with the user-facing warning suppressed. Move the policy to its owner: add `isDurablyRetryableMediaFetchError` to `src/media/fetch.ts`, defined as `shouldRetryMediaFetch` (the canonical in-loop transient policy) plus shutdown/abort `fetch_failed`. A restart-window abort is the primary inbound-media loss vector and is recoverable on replay, whereas in-loop retry mid-shutdown is futile; permanent `fetch_failed`, other 4xx, and size limits stay non-retryable so they cannot loop in the spool. Telegram reuses the shared helper instead of a local classifier (`isAbortError`/`isTransientNetworkError` live in core; the classification must not drift). Adds permanent-`fetch_failed` regression coverage at the unit and behavioral level. Media-group partial delivery (#55216) unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Landed via rebase onto main.
Thanks @luoyanglang! |
openclaw#98076) Review on openclaw#98102: the durable-retry classifier treated every MediaFetchError `fetch_failed` as retryable, but `fetch_failed` also covers permanent failures (SSRF/guard denials, local Bot API path/read errors). On spooled replay those would requeue forever with the user-facing warning suppressed. Move the policy to its owner: add `isDurablyRetryableMediaFetchError` to `src/media/fetch.ts`, defined as `shouldRetryMediaFetch` (the canonical in-loop transient policy) plus shutdown/abort `fetch_failed`. A restart-window abort is the primary inbound-media loss vector and is recoverable on replay, whereas in-loop retry mid-shutdown is futile; permanent `fetch_failed`, other 4xx, and size limits stay non-retryable so they cannot loop in the spool. Telegram reuses the shared helper instead of a local classifier (`isAbortError`/`isTransientNetworkError` live in core; the classification must not drift). Adds permanent-`fetch_failed` regression coverage at the unit and behavioral level. Media-group partial delivery (openclaw#55216) unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
openclaw#98076) Review on openclaw#98102: the durable-retry classifier treated every MediaFetchError `fetch_failed` as retryable, but `fetch_failed` also covers permanent failures (SSRF/guard denials, local Bot API path/read errors). On spooled replay those would requeue forever with the user-facing warning suppressed. Move the policy to its owner: add `isDurablyRetryableMediaFetchError` to `src/media/fetch.ts`, defined as `shouldRetryMediaFetch` (the canonical in-loop transient policy) plus shutdown/abort `fetch_failed`. A restart-window abort is the primary inbound-media loss vector and is recoverable on replay, whereas in-loop retry mid-shutdown is futile; permanent `fetch_failed`, other 4xx, and size limits stay non-retryable so they cannot loop in the spool. Telegram reuses the shared helper instead of a local classifier (`isAbortError`/`isTransientNetworkError` live in core; the classification must not drift). Adds permanent-`fetch_failed` regression coverage at the unit and behavioral level. Media-group partial delivery (openclaw#55216) unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
openclaw#98076) Review on openclaw#98102: the durable-retry classifier treated every MediaFetchError `fetch_failed` as retryable, but `fetch_failed` also covers permanent failures (SSRF/guard denials, local Bot API path/read errors). On spooled replay those would requeue forever with the user-facing warning suppressed. Move the policy to its owner: add `isDurablyRetryableMediaFetchError` to `src/media/fetch.ts`, defined as `shouldRetryMediaFetch` (the canonical in-loop transient policy) plus shutdown/abort `fetch_failed`. A restart-window abort is the primary inbound-media loss vector and is recoverable on replay, whereas in-loop retry mid-shutdown is futile; permanent `fetch_failed`, other 4xx, and size limits stay non-retryable so they cannot loop in the spool. Telegram reuses the shared helper instead of a local classifier (`isAbortError`/`isTransientNetworkError` live in core; the classification must not drift). Adds permanent-`fetch_failed` regression coverage at the unit and behavioral level. Media-group partial delivery (openclaw#55216) unchanged. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Fixes #98076
What Problem This Solves
A single inbound media message (e.g. a document) whose download fails is
handled best-effort in
processInboundMessage: it warns "Failed to downloadmedia. Please try again." and lets the update complete.
bot-corethen marksthe spooled update
completed: true(acked). During a gatewayrestart/upgrade window the warning send also fails, so the document is
silently dropped — never written to
media/inbound/, no retry, no notice tothe user or the assistant. The issue confirms a re-send after the restart landed
correctly, so the loss is restart-window-specific.
Why This Change Was Made
Inbound media that fails transiently around a restart should be durably re-driven
after the gateway comes back, not acked-and-lost. The spooled-replay completion
contract already keeps
failed-retryableupdates; the single-message media catchjust never recorded that outcome, so the spool acked the update and dropped it.
The retry decision must be precise.
fetch_failedis the catch-all media errorcode and covers both transient failures (network, shutdown/abort) and
permanent ones (SSRF/guard denials, local Bot API path/read errors). Durably
retrying a permanent failure would loop it in the spool with the user warning
suppressed. The correct transient/abort determination already lives in
src/media/fetch.ts(isAbortError/isTransientNetworkError), so the policybelongs there rather than re-derived in the channel.
User Impact
gateway comes back instead of silently lost.
durable spooled replay (we are retrying); live failures and permanent failures
still warn exactly as before.
requeued — they stay best-effort (acked + warned) and cannot loop the spool.
skip-the-unfetchable-photo + partial-delivery warning ([Bug] Telegram media group: silent image loss when one photo fails to download #55216).
Evidence
Source-level regression proof, run locally on macOS against this patch (this issue
is
needs-live-repro; a live restart-during-upload rig and the agentic nativeTelegram proof job require maintainer/write perms, so the mechanism is proven
against the real handler +
bot-corecompletion frame):src/media/fetch.ts—isDurablyRetryableMediaFetchError=shouldRetryMediaFetch(the canonical in-loop transient policy) plusshutdown/abort
fetch_failed. Truth-table unit tests insrc/media/fetch.test.tscover: transient-network + 408/5xx → retry;shutdown/abort
fetch_failed→ retry; permanentfetch_failed(guard/SSRF,local path), other 4xx, and size limits → no retry.
bot.create-telegram-bot.channel-post-media.test.tsdrive a real single-message document fetch failure through the actual handler
inside
runWithTelegramUpdateProcessingFrame+withTelegramSpooledReplayUpdate:{ kind: "failed-retryable" }(kept) andthe retry notice is not sent;
fetch_failed(guard/SSRF) → acked (nofailed-retryable result) and the user is warned;
fetch,bot-core,bot-update-tracker,polling-session,bot-message, media-groupskip-warning, classifier + behavioral): 264 tests pass.
pnpm tsgo:core+tsgo:core:test+tsgo:extensions+tsgo:extensions:test,pnpm plugin-sdk:api:check,oxfmt --check,node scripts/run-oxlint.mjs, and stagedgitleaks protect --staged --redactall clean.
Not Covered
A live Telegram account with a gateway restart triggered mid-upload is not
reproduced here; the durable-keep / ack / warning-suppression behavior is asserted
at the source level. A maintainer can capture visible proof via Mantis if desired.