fix(slack): surface retryable + Retry-After on send() rate-limit errors (#46762) - #52436
fix(slack): surface retryable + Retry-After on send() rate-limit errors (#46762)#52436srojk34 wants to merge 1 commit into
Conversation
…rs (NousResearch#46762) Slack's send() caught all exceptions and returned a bare SendResult(success=False) — never setting retryable=True or extracting the server's Retry-After header. When Slack returned a 429 rate-limit error, the base _send_with_retry() layer saw retryable=False and did not retry, silently dropping remaining message chunks. Reuse the existing _is_retryable_upload_error() helper (which already detects 429, 500+, and connection-type errors) to set retryable=True, and extract the Retry-After header from the SlackApiError response when present so the base retry layer honors Slack's backoff schedule instead of its own default. Sibling of the Telegram FloodWait fix (PR NousResearch#46762 / commit 404b06a) which added the SendResult.retry_after plumbing to the base layer. Adds five regression tests covering 429 with/without Retry-After, 500 server errors, 403 non-retryable errors, and connection errors.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the missing Slack rate-limit classification; current main still returns a bare failed SendResult from SlackAdapter.send() at plugins/platforms/slack/adapter.py:1435-1437.
Problems
- The new
retryable=Truepath would causeBasePlatformAdapter._send_with_retry()to callsend()again with the complete original content (gateway/platforms/base.py:4148).SlackAdapter.send()posts split responses sequentially (plugins/platforms/slack/adapter.py:1396-1410), so a 429 on a middle chunk would replay already delivered prefix chunks and duplicate them.
Suggested changes
- Retry only the failed Slack chunk, honoring
Retry-After, instead of returning a retryable whole-message failure to the base retry wrapper. - Add a multi-chunk regression where chunk one succeeds and chunk two receives 429; assert chunk one is emitted once.
Automated hermes-sweeper review.
| except Exception as e: # pragma: no cover - defensive logging | ||
| logger.error("[Slack] Send error: %s", e, exc_info=True) | ||
| return SendResult(success=False, error=str(e)) | ||
| _retryable = self._is_retryable_upload_error(e) |
There was a problem hiding this comment.
Marking a failure from this whole-message send() retryable makes _send_with_retry() invoke send() again with the full original content. Since send() posts chunks sequentially, a 429 after earlier chunks succeeded will replay those prefixes and duplicate them. Retry the failed chunk locally (and honor its Retry-After) rather than escalating this result to the whole-message retry layer.
|
Merged via #69479 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your retryable + Retry-After surfacing was cherry-picked directly. Thanks for the contribution! |
Summary
404b06ac4) extractsretry_afterfrom FloodWait errors and setsretryable=Trueso the base_send_with_retry()layer can honor the server-requested backoff.send()caught all exceptions and returned a bareSendResult(success=False)— never settingretryable=Trueor extracting theRetry-Afterheader. When Slack returned a 429 rate-limit error, the base layer sawretryable=Falseand did not retry, silently dropping remaining message chunks.hermes sendto Discord drops remaining chunks on a 429 — no retry in the standalone send path (Telegram has one) #44468 / PR fix(discord): retry 429s in the standalone send path, honoring retry_after #44488).Repro scenario
ratelimitedon a middle chunk withRetry-After: 30send()catches the exception →SendResult(success=False, retryable=False)← no retry_send_with_retry()seesretryable=False, "ratelimited" not in_RETRYABLE_ERROR_PATTERNS→ gives upFix
Reuse the existing
_is_retryable_upload_error()helper (which already detects 429, 500+, and connection errors for the upload path) to setretryable=Trueon thesend()path. Extract theRetry-Afterheader from theSlackApiErrorresponse when present.Test plan
test_429_returns_retryable_with_retry_after— 429 + Retry-After: 30 → retryable=True, retry_after=30.0test_429_without_retry_after_header— 429 without header → retryable=True, retry_after=Nonetest_500_is_retryable_no_retry_after— 500 → retryable=Truetest_403_is_not_retryable— 403 → retryable=False (unchanged)test_connection_error_is_retryable— ConnectionError → retryable=True