Conversation
…#511) Pod aurora-oss-server-789fd5b9c4-cz9qs was evicted at 2026-06-17 ~17:06 UTC due to OOM (1481496Ki used vs 512Mi request). Root cause is the unbounded all_channels.extend() loop in list_bot_channels() introduced by aadafae. PR #511 (original fix) was closed without merging on 2026-06-17. This PR re-applies the same bounded pagination fix against the current main branch (659f7ec — includes PR #505 health endpoint pool fix). Changes: - list_bot_channels() now accepts max_channels=5000 cap with early-exit guard - New iter_bot_channel_pages() generator for callers that can process pages lazily - _sleep() static method replaces time.sleep() in _make_request() to avoid blocking gunicorn gthread workers during rate-limit retries Incident: https://infrapoo.org/incidents/2f9b42d9-f2ef-44e9-b378-3e5639c83130
Walkthrough
ChangesSlackClient async-aware retry and paginated channel listing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/connectors/slack_connector/client.py`:
- Line 10: Update the import statement to use modern type hints instead of
deprecated typing module equivalents. Replace the imports of Dict, List, and
Iterator from the typing module with their built-in counterparts: use dict and
list directly in type annotations, and import Iterator from collections.abc
instead. After updating the imports, ensure all usages of Dict, List, and
Iterator throughout the file are adjusted to use the new syntax (dict, list, and
collections.abc.Iterator respectively).
- Around line 47-74: The _sleep method has a deadlock vulnerability: calling
asyncio.run_coroutine_threadsafe() from the same thread that owns the running
event loop causes future.result() to block that thread, preventing the loop from
executing the scheduled asyncio.sleep(). This defeats the purpose of the async
optimization and falls back to synchronous sleep anyway. To fix this, first
determine if _sleep is actually called from an async context—if only sync worker
threads call this method (the typical gunicorn gthread scenario), remove the
entire async branch (the try-except that gets the loop and the subsequent if
loop is not None check) and keep only the else: time.sleep(seconds) path. If
async callers do exist, convert _sleep to an async method and use await
asyncio.sleep(seconds) directly, requiring all callers to be async. Document the
expected calling context clearly in the method docstring to prevent future
misuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b7c8645-5378-4d8b-890b-2af0159c42c6
📒 Files selected for processing (1)
server/connectors/slack_connector/client.py
| import time | ||
| import requests | ||
| from typing import Dict, Any, List, Optional | ||
| from typing import Dict, Any, Iterator, List, Optional |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using modern type hints.
typing.Dict, typing.List, and typing.Iterator are deprecated in favor of built-in dict, list, and collections.abc.Iterator (or just Iterator from collections.abc). This is a minor modernization.
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 10-10: Import from collections.abc instead: Iterator
Import from collections.abc
(UP035)
[warning] 10-10: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 10-10: typing.List is deprecated, use list instead
(UP035)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/connectors/slack_connector/client.py` at line 10, Update the import
statement to use modern type hints instead of deprecated typing module
equivalents. Replace the imports of Dict, List, and Iterator from the typing
module with their built-in counterparts: use dict and list directly in type
annotations, and import Iterator from collections.abc instead. After updating
the imports, ensure all usages of Dict, List, and Iterator throughout the file
are adjusted to use the new syntax (dict, list, and collections.abc.Iterator
respectively).
Source: Linters/SAST tools
| @staticmethod | ||
| def _sleep(seconds: float) -> None: | ||
| """ | ||
| Sleep without blocking gunicorn gthread workers. | ||
|
|
||
| In gunicorn's gthread mode each worker thread handles one request at a | ||
| time. A bare time.sleep() on a 429 retry holds the thread for up to | ||
| 30 s, causing request queuing that amplifies memory pressure. | ||
|
|
||
| When called from inside a running asyncio event loop (e.g. an async | ||
| route or background task) we schedule the coroutine on that loop so | ||
| other coroutines can run during the wait. In a pure-sync context we | ||
| fall back to time.sleep(). | ||
| """ | ||
| try: | ||
| loop = asyncio.get_event_loop() | ||
| except RuntimeError: | ||
| loop = None | ||
|
|
||
| if loop is not None and loop.is_running(): | ||
| future = asyncio.run_coroutine_threadsafe(asyncio.sleep(seconds), loop) | ||
| try: | ||
| future.result(timeout=seconds + 5) | ||
| except Exception: | ||
| # Fallback: if the future fails for any reason, just sleep | ||
| time.sleep(seconds) | ||
| else: | ||
| time.sleep(seconds) |
There was a problem hiding this comment.
run_coroutine_threadsafe from the same thread will deadlock, negating the intended benefit.
asyncio.run_coroutine_threadsafe() is designed to schedule coroutines from a different thread onto an event loop. When called from the same thread that owns the running loop, future.result() blocks that thread—preventing the event loop from executing the scheduled asyncio.sleep(). This results in:
- The call blocks for
seconds + 5until the timeout fires TimeoutErroris caught, falling back totime.sleep(seconds)- Net effect: the worker is still blocked for the full duration (or longer)
In gunicorn's gthread mode, request handlers typically run in sync worker threads without an active asyncio loop, so the fallback path (else: time.sleep()) executes and behavior is unchanged. The async path only triggers if someone explicitly starts a loop in that thread, in which case it deadlocks.
Options to address:
- If async callers are expected: Make
_sleepanasyncmethod andawait asyncio.sleep()directly, requiring callers to be async. - If this class remains sync-only: Remove the async path entirely and document that blocking is expected. The event-loop detection adds complexity without benefit for sync callers.
- If cross-thread scheduling is the goal: Ensure
_sleepis only called from threads that are not running the target loop (e.g., a background thread submitting to a main-thread loop).
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 70-70: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/connectors/slack_connector/client.py` around lines 47 - 74, The _sleep
method has a deadlock vulnerability: calling asyncio.run_coroutine_threadsafe()
from the same thread that owns the running event loop causes future.result() to
block that thread, preventing the loop from executing the scheduled
asyncio.sleep(). This defeats the purpose of the async optimization and falls
back to synchronous sleep anyway. To fix this, first determine if _sleep is
actually called from an async context—if only sync worker threads call this
method (the typical gunicorn gthread scenario), remove the entire async branch
(the try-except that gets the loop and the subsequent if loop is not None check)
and keep only the else: time.sleep(seconds) path. If async callers do exist,
convert _sleep to an async method and use await asyncio.sleep(seconds) directly,
requiring all callers to be async. Document the expected calling context clearly
in the method docstring to prevent future misuse.
![aurora-test-app1[bot]](https://avatars.githubusercontent.com/in/3646679?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by updated review
Aurora Risk Review
Verdict: RISKY
The memory fix (generator + 5000-channel cap) correctly addresses the OOM root cause and is safe to ship. However, the _sleep() non-blocking retry implementation contains two operational defects: (1) in the actual gunicorn gthread deployment it silently falls back to the original time.sleep() on every 429 retry, meaning the thread-blocking problem it claims to fix is not actually fixed; (2) if _sleep() is ever called from a running asyncio event loop thread (e.g. future MCP server usage), future.result() will deadlock until the seconds+5 timeout fires, causing ~35s hangs per retry. The fix is half-correct — merge it for the OOM protection, but the _sleep() implementation needs a follow-up.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | MEDIUM | server/connectors/slack_connector/client.py:47 |
_sleep() falls back to time.sleep() in gunicorn gthread — thread-blocking problem not actually fixed |
| 2 | MEDIUM | server/connectors/slack_connector/client.py:62 |
run_coroutine_threadsafe + future.result() deadlocks if called from an asyncio event loop thread |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
There was a problem hiding this comment.
Aurora Risk Review
Verdict: RISKY
This PR correctly fixes the unbounded memory accumulation that caused the OOM eviction (the max_channels cap and generator are sound). However, the _sleep() implementation introduced to fix thread-blocking on 429 retries is unchanged from the prior review and remains broken in both production execution contexts: in gunicorn gthread workers it always falls back to time.sleep() (the problem it claims to solve), and in the asyncio MCP server it will deadlock if a 429 is hit. The OOM fix is safe to ship; the _sleep() fix is not.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | MEDIUM | server/connectors/slack_connector/client.py:47 |
_sleep() always falls back to time.sleep() in gunicorn gthread — thread-blocking problem not fixed |
| 2 | MEDIUM | server/connectors/slack_connector/client.py:62 |
run_coroutine_threadsafe + future.result() deadlocks when called from the asyncio MCP server |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
|
2 participants
Root Cause
Commit
aadafae("fix: Slack OAuth token loss for large workspaces #508") introduced an unboundedall_channels.extend()loop inlist_bot_channels(). For large Slack workspaces this accumulates the full channel list in the gunicorn worker's heap before returning, causing OOM eviction.Evidence from incident
2f9b42d9-f2ef-44e9-b378-3e5639c83130(2026-06-17 ~17:06 UTC):Pod
aurora-oss-server-789fd5b9c4-cz9qs(imagesha-ec11e5c) evicted with 4 restarts (exit code 137). Uptime metrics absent for 5+ minutes → "Aurora Prod - API Server Down" alert fired.History: PR #511 was the original fix for this same root cause (first observed 2026-06-15). PR #511 was closed without merging on 2026-06-17. This PR re-applies the identical fix rebased against current
main(commit659f7ec, which includes the PR #505 health endpoint pool fix).Two compounding issues fixed
1. Unbounded in-memory accumulation in
list_bot_channels()For large Slack workspaces (thousands of channels), this grows to 1.5–2.2 GiB in the gunicorn worker heap, triggering kubelet node-pressure eviction.
2.
time.sleep()blocking gunicorn gthread workers_make_request()calledtime.sleep()on 429 rate-limit retries. In gunicorn'sgthreadmode each worker thread handles one request at a time —time.sleep()blocks the thread for up to 30 s, causing request queuing that amplifies memory pressure.Fix
iter_bot_channel_pages()— new generator (lazy, zero accumulation)list_bot_channels()— bounded with early-exit cap_sleep()— non-blocking sleep in async/gthread contextsRate-limit retries now yield control back to the event loop instead of blocking the gunicorn worker thread.
Testing
list_bot_channels()returns ≤ 5000 channels and logs a warning when the cap is hitIncident References
cz9qsevicted, uptime absent 5 min)99553103-1b40-45e8-96b3-6d385abfb199(podxz9zv, 2026-06-15)01KV61A7RM3WQYM6NBDDF5V6TT(2026-06-15 ~16:19 UTC)aadafae("fix: Slack OAuth token loss for large workspaces fix: Slack OAuth token loss for large workspaces #508")Summary by CodeRabbit
New Features
Performance & Stability