Skip to content

perf(pagerduty): parallelize permission checks with asyncio.gather#320

Merged
sylvainkalache merged 1 commit intomainfrom
perf/pagerduty-parallel-permissions
Feb 6, 2026
Merged

perf(pagerduty): parallelize permission checks with asyncio.gather#320
sylvainkalache merged 1 commit intomainfrom
perf/pagerduty-parallel-permissions

Conversation

@sylvainkalache
Copy link
Copy Markdown
Contributor

Summary

  • check_permissions() was making 4 sequential HTTP calls to PagerDuty API (users, incidents, services, oncalls)
  • Now uses asyncio.gather() to run all 4 checks in parallel within the same aiohttp.ClientSession
  • Extracted a _check_endpoint() helper to eliminate code duplication (4 nearly-identical blocks → 1 reusable function)

Expected Impact

  • Average response time: ~2.3s → ~0.6-0.8s (limited by the slowest single endpoint)
  • Error handling: unchanged - each endpoint still reports its own status independently

Test plan

  • Added 6 new tests covering: all-accessible, 401, 403 with permission hints, connection errors, session failure, result shape
  • All existing tests pass (no regressions)

The check_permissions() method was making 4 sequential HTTP calls
(users, incidents, services, oncalls), each waiting for the previous
to complete. Now uses asyncio.gather() to run all 4 in parallel.

Expected improvement: ~2.3s avg → ~0.6-0.8s (limited by slowest call).
@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 6, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
rootly-burnout-detector-dev Ready Ready Preview, Comment Feb 6, 2026 1:51am
rootly-burnout-detector-web Ready Ready Preview, Comment Feb 6, 2026 1:51am

Request Review

@claude
Copy link
Copy Markdown

claude Bot commented Feb 6, 2026

Claude finished @sylvainkalache's task —— View job


Code Review Findings

Bug & Logic Error:

  • Race condition in session sharing: Lines 344-375 in check_permissions() - All parallel _check_endpoint() calls share the same aiohttp.ClientSession instance, but if one endpoint call fails and raises an exception, it could potentially affect other concurrent requests since they're using the same session connection pool.

Security Issue:

  • API token logging: Lines 32-33 and 282-283 - The code logs the last 4 characters of the API token for debugging. While this seems minimal, it still exposes partial sensitive information that could aid in token reconstruction attacks when combined with other data leaks.

Code Quality Issues:

  1. Missing error handling in _check_endpoint(): Line 344 - The nested helper function doesn't validate that required parameters (session, name, url, params) are not None before using them.

  2. Inconsistent timeout handling: The check_permissions() method sets a 30-second timeout on the ClientSession (line 342), but individual _check_endpoint() calls don't have their own timeouts, relying solely on the session-level timeout.

  3. Hard-coded error messages: Lines 351-355 - Error messages are hard-coded strings that don't account for potential variations in PagerDuty's error responses or future API changes.

  4. No validation of gather() results: Line 361-366 - The code doesn't validate that asyncio.gather() returns the expected number of results (4) before unpacking them.

Performance Concerns:

  • Redundant session creation: While the parallel approach is good, the session is created even when all endpoints might fail due to connection issues. Consider adding a quick connectivity test before creating the session for all 4 endpoints.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 6, 2026

Greptile Overview

Greptile Summary

Replaced sequential HTTP calls with parallel execution using asyncio.gather() in check_permissions(), reducing API response time from ~2.3s to ~0.6-0.8s.

Key changes:

  • Extracted _check_endpoint() helper function to eliminate code duplication (4 nearly-identical blocks consolidated)
  • All 4 PagerDuty endpoints (users, incidents, services, oncalls) now checked concurrently within the same aiohttp.ClientSession
  • Error handling preserved: each endpoint continues to report its own status independently
  • Added comprehensive test coverage with 6 new tests validating success, various error scenarios, and result structure

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The refactoring is clean and well-tested: parallel execution is a straightforward optimization that doesn't change the logic, error handling remains robust, and the new test suite validates all critical paths including edge cases
  • No files require special attention

Important Files Changed

Filename Overview
backend/app/core/pagerduty_client.py Refactored check_permissions() to use asyncio.gather() for parallel API checks, extracted reusable _check_endpoint() helper
backend/tests/test_pagerduty_client.py Added 6 comprehensive tests covering success cases, error responses (401, 403), connection failures, and session creation errors

@sylvainkalache sylvainkalache merged commit 716d2dc into main Feb 6, 2026
10 checks passed
@sylvainkalache sylvainkalache deleted the perf/pagerduty-parallel-permissions branch February 6, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant