fix(telegram): add max retries and backoff to sendChatAction#56153
fix(telegram): add max retries and backoff to sendChatAction#56153Pick-cat wants to merge 2 commits into
Conversation
openclaw-barnacle
Bot
added
channel: telegram
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08c05e5695
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| retry: typingRetryConfig, | ||
| verbose: opts.verbose, | ||
| shouldRetry: (err) => isRecoverableTelegramNetworkError(err, { context: "send" }), | ||
| shouldRetry: shouldRetrySendChatAction, | ||
| }); |
There was a problem hiding this comment.
sendTypingTelegram passes shouldRetrySendChatAction, but it does not set strictShouldRetry, so createTelegramRetryRunner still ORs in TELEGRAM_RETRY_RE (see src/infra/retry-policy.ts), which re-enables retries for 429 messages even when your predicate returns false. In rate-limit scenarios this causes the new 5-attempt backoff path to run before continuing, so typing can still stall outbound flow instead of immediately skipping 429/529 as intended.
Useful? React with 👍 / 👎.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Greptile SummaryThis PR fixes issue #56096 by adding bounded retries (max 5 attempts), exponential backoff with jitter, and a All three previously flagged P1 issues from prior review threads have been addressed:
The only minor note is in Confidence Score: 5/5Safe to merge — all prior P1 findings are resolved and only one minor redundant guard remains as a P2 style note. All three previously flagged blocking issues (missing strictShouldRetry, silent error swallowing, misleading constant name) are now fully addressed. The remaining observation is a P2 style point with no correctness impact. No files require special attention.
|
| Filename | Overview |
|---|---|
| extensions/telegram/src/send.ts | Adds MAX_SEND_CHAT_ACTION_ATTEMPTS, shouldRetrySendChatAction, strictShouldRetry: true, exponential backoff config, and non-blocking catch. All three previously flagged P1 issues are fully resolved. |
| extensions/telegram/src/sendchataction-401-backoff.ts | Adds is429Or529Error helper and early-return guard in the catch block. The guard is functionally redundant but harmless. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/telegram/src/sendchataction-401-backoff.ts
Line: 130-138
Comment:
**`is429Or529Error` guard is redundant here**
The `createTelegramSendChatActionHandler` doesn't perform any retry logic — it only tracks `consecutive401Failures`. Since a 429/529 error message won't satisfy `is401Error` (no `"401"` / `"unauthorized"` substring), the counter would never be incremented even without this early-return guard. The `throw error` at the bottom of the `catch` block already re-throws for non-401 errors.
The added guard isn't harmful, but the comment "Re-throw without incrementing consecutive401Failures" implies there's a risk of that happening, which there isn't. Consider simplifying to remove it, or adjust the comment to clarify that this is just defensive/intent-documenting code.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(telegram): address PR review comment..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const requestWithDiag = createTelegramRequestWithDiag({ | ||
| cfg, | ||
| account, | ||
| retry: opts.retry, | ||
| retry: typingRetryConfig, | ||
| verbose: opts.verbose, | ||
| shouldRetry: (err) => isRecoverableTelegramNetworkError(err, { context: "send" }), | ||
| shouldRetry: shouldRetrySendChatAction, | ||
| }); |
There was a problem hiding this comment.
strictShouldRetry: true missing — 429 errors are still retried
The createTelegramRetryRunner in src/infra/retry-policy.ts resolves the final shouldRetry predicate like this (line 27-28):
// strictShouldRetry not set → falls here
return (err: unknown) =>
params.shouldRetry?.(err) || TELEGRAM_RETRY_RE.test(formatErrorMessage(err));And TELEGRAM_RETRY_RE is defined as:
const TELEGRAM_RETRY_RE = /429|timeout|connect|reset|closed|unavailable|temporarily/i;It explicitly matches "429". So even though shouldRetrySendChatAction returns false for a 429 error, the fallback regex test will return true, and the OR makes the combined predicate return true — the request is retried anyway.
The core goal of this PR ("Never retries on 429/529 errors") is not achieved for 429 errors without strictShouldRetry: true. Compare with createTelegramNonIdempotentRequestWithDiag at line 597, which correctly passes strictShouldRetry: true to opt out of the regex fallback.
| const requestWithDiag = createTelegramRequestWithDiag({ | |
| cfg, | |
| account, | |
| retry: opts.retry, | |
| retry: typingRetryConfig, | |
| verbose: opts.verbose, | |
| shouldRetry: (err) => isRecoverableTelegramNetworkError(err, { context: "send" }), | |
| shouldRetry: shouldRetrySendChatAction, | |
| }); | |
| const requestWithDiag = createTelegramRequestWithDiag({ | |
| cfg, | |
| account, | |
| retry: typingRetryConfig, | |
| verbose: opts.verbose, | |
| shouldRetry: shouldRetrySendChatAction, | |
| strictShouldRetry: true, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/telegram/src/send.ts
Line: 1070-1076
Comment:
**`strictShouldRetry: true` missing — 429 errors are still retried**
The `createTelegramRetryRunner` in `src/infra/retry-policy.ts` resolves the final `shouldRetry` predicate like this (line 27-28):
```ts
// strictShouldRetry not set → falls here
return (err: unknown) =>
params.shouldRetry?.(err) || TELEGRAM_RETRY_RE.test(formatErrorMessage(err));
```
And `TELEGRAM_RETRY_RE` is defined as:
```ts
const TELEGRAM_RETRY_RE = /429|timeout|connect|reset|closed|unavailable|temporarily/i;
```
It **explicitly matches `"429"`**. So even though `shouldRetrySendChatAction` returns `false` for a 429 error, the fallback regex test will return `true`, and the OR makes the combined predicate return `true` — the request is retried anyway.
The core goal of this PR ("Never retries on 429/529 errors") is not achieved for 429 errors without `strictShouldRetry: true`. Compare with `createTelegramNonIdempotentRequestWithDiag` at line 597, which correctly passes `strictShouldRetry: true` to opt out of the regex fallback.
```suggestion
const requestWithDiag = createTelegramRequestWithDiag({
cfg,
account,
retry: typingRetryConfig,
verbose: opts.verbose,
shouldRetry: shouldRetrySendChatAction,
strictShouldRetry: true,
});
```
How can I resolve this? If you propose a fix, please make it concise.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| } catch (err) { | ||
| // Log but don't fail - typing indicator is non-critical | ||
| // This ensures the main message flow continues even if typing fails | ||
| if (opts.verbose) { | ||
| sendLogger.warn( | ||
| `sendTypingTelegram failed after ${MAX_SEND_CHAT_ACTION_RETRIES} retries, continuing: ${formatErrorMessage(err)}`, | ||
| ); | ||
| } | ||
| // Return success anyway - typing indicator is best-effort | ||
| } |
There was a problem hiding this comment.
The catch block only logs when opts.verbose is true. In a typical production deployment without verbose mode, persistent typing failures (even after all 5 attempts) produce no log output at all, making it very difficult to notice if sendChatAction is systematically broken.
Consider logging unconditionally at the warn level:
| } catch (err) { | |
| // Log but don't fail - typing indicator is non-critical | |
| // This ensures the main message flow continues even if typing fails | |
| if (opts.verbose) { | |
| sendLogger.warn( | |
| `sendTypingTelegram failed after ${MAX_SEND_CHAT_ACTION_RETRIES} retries, continuing: ${formatErrorMessage(err)}`, | |
| ); | |
| } | |
| // Return success anyway - typing indicator is best-effort | |
| } | |
| } catch (err) { | |
| // Log but don't fail - typing indicator is non-critical | |
| // This ensures the main message flow continues even if typing fails | |
| sendLogger.warn( | |
| `sendTypingTelegram failed after ${MAX_SEND_CHAT_ACTION_RETRIES} retries, continuing: ${formatErrorMessage(err)}`, | |
| ); | |
| // Return success anyway - typing indicator is best-effort | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/telegram/src/send.ts
Line: 1089-1098
Comment:
**Errors silently swallowed when not in verbose mode**
The `catch` block only logs when `opts.verbose` is true. In a typical production deployment without verbose mode, persistent typing failures (even after all 5 attempts) produce no log output at all, making it very difficult to notice if `sendChatAction` is systematically broken.
Consider logging unconditionally at the `warn` level:
```suggestion
} catch (err) {
// Log but don't fail - typing indicator is non-critical
// This ensures the main message flow continues even if typing fails
sendLogger.warn(
`sendTypingTelegram failed after ${MAX_SEND_CHAT_ACTION_RETRIES} retries, continuing: ${formatErrorMessage(err)}`,
);
// Return success anyway - typing indicator is best-effort
}
```
How can I resolve this? If you propose a fix, please make it concise.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| // Max retry attempts for sendChatAction to prevent infinite loops (issue #56096) | ||
| const MAX_SEND_CHAT_ACTION_RETRIES = 5; |
There was a problem hiding this comment.
retryAsync treats the attempts field as total call count (initial + retries), not the number of retries. With MAX_SEND_CHAT_ACTION_RETRIES = 5 passed as attempts: 5, the function is called 5 times total (4 retries), not 5 retries. The PR description's claimed backoff sequence "1s → 2s → 4s → 8s → 16s" has 5 delays, which would require attempts: 6.
Consider renaming to MAX_SEND_CHAT_ACTION_ATTEMPTS to match the semantics of the RetryConfig.attempts field.
| // Max retry attempts for sendChatAction to prevent infinite loops (issue #56096) | |
| const MAX_SEND_CHAT_ACTION_RETRIES = 5; | |
| const MAX_SEND_CHAT_ACTION_ATTEMPTS = 5; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/telegram/src/send.ts
Line: 1031-1032
Comment:
**Variable name says "retries" but the value is used as total "attempts"**
`retryAsync` treats the `attempts` field as total call count (initial + retries), not the number of retries. With `MAX_SEND_CHAT_ACTION_RETRIES = 5` passed as `attempts: 5`, the function is called 5 times total (4 retries), not 5 retries. The PR description's claimed backoff sequence "1s → 2s → 4s → 8s → 16s" has 5 delays, which would require `attempts: 6`.
Consider renaming to `MAX_SEND_CHAT_ACTION_ATTEMPTS` to match the semantics of the `RetryConfig.attempts` field.
```suggestion
const MAX_SEND_CHAT_ACTION_ATTEMPTS = 5;
```
How can I resolve this? If you propose a fix, please make it concise.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
08c05e5 to
2b04aee
Compare
- Add MAX_SEND_CHAT_ACTION_RETRIES (5) to prevent infinite loops - Add shouldRetrySendChatAction to skip 429/529 errors that cause loops - Add is429Or529Error check to sendChatActionHandler (issue openclaw#56096) - Add exponential backoff (1s to 30s) with jitter - Gracefully handle sendTypingTelegram failures (non-critical) Fixes openclaw#56096 Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b04aee418
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| api_base="${api_base%/}" | ||
|
|
||
| curl -sS "${api_base}/audio/transcriptions" \ | ||
| curl -sS "${OPENAI_BASE_URL}/audio/transcriptions" \ |
There was a problem hiding this comment.
Building the endpoint as "${OPENAI_BASE_URL}/audio/transcriptions" without normalizing OPENAI_BASE_URL regresses behavior when users set a base URL with a trailing slash (for example .../v1/), producing .../v1//audio/transcriptions. Several OpenAI-compatible gateways/proxies route on exact paths and will return 404/route-miss for the double-slash path, so transcription can fail for a common configuration format.
Useful? React with 👍 / 👎.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
2b04aee to
726d88f
Compare
- Add strictShouldRetry: true to prevent 429 errors from being retried via TELEGRAM_RETRY_RE fallback regex - Rename MAX_SEND_CHAT_ACTION_RETRIES to MAX_SEND_CHAT_ACTION_ATTEMPTS since 'attempts' represents total call count, not retry count - Always log typing failures (remove opts.verbose guard) so production issues are visible Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Thanks @greptile-apps for the review! The issues have been addressed in the latest commit:
Please re-review when you have a chance. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f26eb963e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| cfg, | ||
| account, | ||
| retry: opts.retry, | ||
| retry: typingRetryConfig, |
There was a problem hiding this comment.
Using a fixed typingRetryConfig here bypasses both opts.retry and per-account retry settings, because createTelegramRetryRunner merges configRetry first and then lets this retry object win. Any deployment that configured low-attempt retries to keep typing non-blocking (e.g., attempts: 1) now gets forced into 5 attempts with exponential delays, which can stall approval notification flows by tens of seconds under network failure.
Useful? React with 👍 / 👎.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@yfge Thanks for the review! Those changes are already included in this PR (commit 8f26eb963de). The PR contains:
The fixes match what was reviewed and are ready for merge. |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close this PR as superseded by the newer canonical Telegram sendChatAction PR #55886. Current main still does not implement this PR's intended bounded/non-blocking typing behavior, but #55886 is open, maintainer-repaired, explicitly tracks the same remaining sendChatAction transient failure surface, and carries forward credit and references for #56096 and this PR. Best possible solution: Close this PR as superseded and keep the remaining Telegram sendChatAction transient-failure work on #55886. Maintainers should review and land #55886, or a direct successor, so the fix ships once in the dedicated handler with tests and contributor credit preserved. What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex review notes: model gpt-5.5, reasoning high; reviewed against 2ccdbc7dd93b. |
2 participants
Summary
Why
Issue #56096 reports that Telegram sendChatAction enters an infinite retry loop when the model returns 529 (overloaded). The bot becomes completely unresponsive. This fix:
Testing
Fixes #56096