Enhance SlackWebMessagingIntegration to include a timeout feature for… #1990
Conversation
… channel iteration, raising an error if the timeout is exceeded.
WalkthroughAdds timeout-aware pagination to Slack channel iteration in SlackWebMessagingIntegration._iter_channels. Introduces a timeout parameter with remaining-time propagation across paginated calls, validates next_cursor type, raises on non-positive remaining time, and imports time. Public API behavior otherwise unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant S as SlackWebMessagingIntegration
participant A as Slack Web API
Note over C,S: List channels with timeout-aware pagination
C->>S: _iter_channels(cursor=None, only_public, timeout=300s)
loop While next_cursor and time remains
S->>S: record start_time
S->>A: conversations.list(cursor, types)
A-->>S: channels page + next_cursor
S-->>C: yield channels
S->>S: elapsed = time() - start_time
S->>S: remaining = timeout - elapsed
alt next_cursor not str
S-->>C: raise ValueError
else remaining <= 0
S-->>C: raise MessagingIntegrationError (timeout)
else
S->>S: recurse with cursor=next_cursor, timeout=remaining
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
MikaKerman
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
|
👋 @MikaKerman |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
elementary/messages/messaging_integrations/slack_web.py (5)
131-136: Make timeout wall-clock accurate and resilient to decorator sleeps (use absolute deadline + monotonic).Right now, timeout_left only subtracts the API call duration measured with time.time(), which:
- ignores any sleep added by @limits/@sleep_and_retry wrappers,
- ignores any time spent between yields (caller processing),
- is sensitive to system clock adjustments.
Switching to an absolute deadline computed with time.monotonic() at the first invocation and propagating it through recursion makes the timeout enforce true wall-clock time regardless of sleeps or clock changes.
Apply this diff:
def _iter_channels( self, cursor: Optional[str] = None, only_public: bool = False, - timeout: float = 300.0, + timeout: float = 300.0, + _deadline: Optional[float] = None, ) -> Iterator[dict]: - if timeout <= 0: - raise MessagingIntegrationError("Channel iteration timed out") + if _deadline is None: + _deadline = time.monotonic() + timeout + if time.monotonic() >= _deadline: + raise MessagingIntegrationError("Channel iteration timed out") - call_start = time.time() response = self.client.conversations_list( cursor=cursor, types="public_channel" if only_public else "public_channel,private_channel", exclude_archived=True, limit=1000, ) - call_duration = time.time() - call_start channels = response["channels"] yield from channels response_metadata = response.get("response_metadata") or {} next_cursor = response_metadata.get("next_cursor") if next_cursor: if not isinstance(next_cursor, str): raise ValueError("Next cursor is not a string") - timeout_left = timeout - call_duration - yield from self._iter_channels(next_cursor, only_public, timeout_left) + yield from self._iter_channels(next_cursor, only_public, timeout, _deadline)To ensure semantics match your intent (overall wall-clock timeout vs. API-call-only timeout), please confirm:
- Timeout should bound total elapsed time of iteration (including rate-limit sleeps and caller processing between yields). If yes, the above change is strongly recommended.
- If you intentionally wanted API-call-time only, we can keep the current approach but still switch to time.monotonic().
Also applies to: 137-139, 140-148, 156-157
140-148: Use time.monotonic() for duration measurement.Even if you keep per-call timing, prefer time.monotonic() to avoid issues with system clock changes.
Apply this minimal diff if you don’t adopt the deadline refactor:
- call_start = time.time() + call_start = time.monotonic() ... - call_duration = time.time() - call_start + call_duration = time.monotonic() - call_start
154-155: Raise MessagingIntegrationError for consistency with the rest of the integration.Other failures here raise MessagingIntegrationError. Switching from ValueError keeps error handling uniform across the integration layer.
- if not isinstance(next_cursor, str): - raise ValueError("Next cursor is not a string") + if not isinstance(next_cursor, str): + raise MessagingIntegrationError("Invalid next_cursor type from Slack API")
2-2: Import is fine; ensure we consistently use monotonic clock where measuring time.Given the new timeout logic, prefer time.monotonic() everywhere durations/deadlines are computed.
131-136: Default timeout looks reasonable; consider making it configurable from callers if needed.Today, _get_channel_id relies on the default 300s. If large workspaces or strict SLAs require tuning, consider allowing a timeout parameter to be threaded from the public call path.
If you want, I can propose a non-breaking change to allow callers to pass a timeout into _get_channel_id and through to _iter_channels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/slack_web.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
elementary/messages/messaging_integrations/slack_web.py (1)
elementary/messages/messaging_integrations/exceptions.py (1)
MessagingIntegrationError(1-2)
🔇 Additional comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
137-139: Timeout pre-check occurs inside the decorated function body.Because @limits/@sleep_and_retry wrappers may sleep before the function body runs, the actual elapsed time can exceed the intended timeout before this check executes. The “absolute deadline + monotonic” refactor above mitigates this by failing fast when the body starts if the deadline has already passed.
Please confirm that potentially exceeding the limit by up to one decorator sleep interval is acceptable. If not, consider moving the iteration loop out of the decorated function or implementing the deadline approach suggested above.
… channel iteration, raising an error if the timeout is exceeded.
Summary by CodeRabbit