perf(bitbucket): offload blocking HTTP from async handlers

[PeterDaveHello]

Problem

Two Bitbucket App code paths are declared async but perform synchronous requests calls directly on the event-loop thread:

• get_bearer_token() calls requests.request() while exchanging the JWT for an OAuth access token.
• _validate_time_from_last_commit_to_pr_update() calls requests.get() while fetching the latest commits for push validation.

When Bitbucket is slow, these calls block the FastAPI event loop for the duration of the network request and can delay unrelated webhook work handled by the same worker. Moving an unbounded request to the shared executor would also allow a stalled endpoint to occupy a worker thread indefinitely.

Changes

• Offload both blocking requests with asyncio.to_thread() while preserving method, URL, headers, payload, response handling, and exception behavior.
• Add bitbucket_app.request_timeout, defaulting to 30 seconds and requiring a positive finite host value.
• Keep the timeout in host-level settings so repository .pr_agent.toml cannot extend shared executor occupancy.
• Document the connection and response-read inactivity timeout semantics.
• Add focused tests for delegation, direct-call regression protection, timeout propagation, invalid values, host-versus-request-scoped settings, and timeout failure handling.

This intentionally does not introduce a new HTTP client/session, change retry policy, or modify other Bitbucket provider requests.

Validation

• PYTHONPATH=. pytest tests/unittest/test_bitbucket_app.py tests/unittest/test_bitbucket_fork_safe_secret_provider.py tests/unittest/test_bitbucket_provider.py -q — 54 passed
• Flake8 and Ruff passed for the changed test file.
• TOML parsing and git diff --check passed.
• Local and private exact-head reviews found no in-scope correctness issue.

[qodo-code-review]

PR Summary by Qodo

Offload blocking Bitbucket HTTP calls from async handlers

✨ Enhancement ⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Offloads OAuth and commit-validation HTTP calls to worker threads, preventing event-loop stalls.
• Bounds offloaded requests with a validated, host-controlled 30-second timeout.
• Documents timeout scope and adds focused delegation, validation, and failure tests.

Diagram

sequenceDiagram
    actor Event as Webhook Event
    participant Handler as Async Handler
    participant Config as Host Settings
    participant Worker as Worker Thread
    participant API as Bitbucket API
    Event->>Handler: Trigger webhook
    Handler->>Config: Read timeout
    Config-->>Handler: Positive seconds
    alt OAuth exchange
        Handler->>Worker: Offload token request
        Worker->>API: POST with timeout
    else Commit validation
        Handler->>Worker: Offload commits request
        Worker->>API: GET with timeout
    end
    API-->>Worker: HTTP response
    Worker-->>Handler: Return result

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt an async HTTP client

• ➕ Avoids worker-thread usage entirely
• ➕ Supports connection pooling across async requests
• ➖ Requires broader client lifecycle and session management
• ➖ Risks changing established request and exception semantics

2. Use a dedicated executor

• ➕ Isolates Bitbucket stalls from the shared thread pool
• ➕ Allows provider-specific concurrency limits
• ➖ Adds executor sizing, shutdown, and lifecycle complexity
• ➖ Still requires bounded synchronous network calls

Recommendation: Keep the PR's asyncio.to_thread approach for this targeted fix because it preserves existing requests behavior while removing event-loop blocking and bounding thread occupancy. Consider a managed async client only as part of a broader Bitbucket HTTP-layer migration.

Files changed (4) +225 / -2

Enhancement (1) +30 / -2

bitbucket_app.pyOffload blocking Bitbucket requests with bounded timeouts +30/-2

Offload blocking Bitbucket requests with bounded timeouts

• Moves OAuth token exchange and commit lookup requests from the event-loop thread into asyncio worker threads. Adds validation for a positive, finite host-controlled timeout while preserving existing response and exception handling.

pr_agent/servers/bitbucket_app.py

Tests (1) +192 / -0

test_bitbucket_app.pyCover HTTP offloading and timeout enforcement +192/-0

Cover HTTP offloading and timeout enforcement

• Adds tests for timeout validation, host-level precedence, worker-thread delegation, request argument preservation, and timeout outcomes in both affected async paths. Direct-call guards prevent regressions to event-loop-blocking requests.

tests/unittest/test_bitbucket_app.py

Documentation (1) +2 / -0

automations_and_usage.mdDocument Bitbucket App request timeout behavior +2/-0

Document Bitbucket App request timeout behavior

• Documents the timeout's connection and response-read inactivity semantics, 30-second default, environment override, and host-only scope. Clarifies that repository configuration cannot extend this resource limit.

docs/docs/usage-guide/automations_and_usage.md

Other (1) +1 / -0

configuration.tomlAdd the default Bitbucket App request timeout +1/-0

Add the default Bitbucket App request timeout

• Defines a 30-second default for connection and response-read inactivity timeouts on offloaded Bitbucket App requests.

pr_agent/settings/configuration.toml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

1. global_settings bypasses get_settings() 📘 Rule violation ⌂ Architecture

Description

_get_request_timeout() reads configuration directly from global_settings instead of obtaining it
through get_settings(). This violates the required single configuration-access path and couples
application code to the global settings object.

Code

pr_agent/servers/bitbucket_app.py[41]

+    timeout = global_settings.get("bitbucket_app.request_timeout")

Relevance

●●● Strong

Recent Bitbucket and configuration reviews consistently accept fixes enforcing get_settings() as the
centralized configuration access path.

PR-#2736
PR-#2490

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2694699 requires modified code to access configuration exclusively through
get_settings(). The added timeout lookup calls global_settings.get(...) directly, and
global_settings is the module-level Dynaconf object defined in pr_agent/config_loader.py.

Rule 2694699: Access configuration only via get_settings() from pr_agent.config_loader
pr_agent/servers/bitbucket_app.py[41-41]
pr_agent/config_loader.py[17-22]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_get_request_timeout()` accesses `global_settings` directly, contrary to the requirement that configuration be obtained through `get_settings()`.

## Issue Context
The timeout intentionally must remain host-controlled rather than request-scoped. Preserve that behavior by making the existing `get_settings(use_context=False)` path return host-level settings, then use that API instead of importing or reading `global_settings` directly.

## Fix Focus Areas
- pr_agent/servers/bitbucket_app.py[22-41]
- pr_agent/config_loader.py[47-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Timeout documentation exceeds line limit ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The newly added request_timeout documentation is written as a single physical line longer than 120
characters. This violates the repository-wide source line-length requirement and reduces
maintainability.

Code

docs/docs/usage-guide/automations_and_usage.md[308]

+For self-hosted BitBucket App deployments, `bitbucket_app.request_timeout` sets both the connection timeout and response-read inactivity timeout, in positive seconds, for offloaded BitBucket HTTP requests. It defaults to `30` and is read from host-level configuration or the `BITBUCKET_APP__REQUEST_TIMEOUT` environment variable; repository `.pr_agent.toml` overrides do not apply to this host resource limit.

Relevance

●● Moderate

Recent history is mixed: comparable long Markdown lines were both accepted and rejected, so team
behavior is uncertain.

PR-#2797
PR-#2817

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2694690 requires every non-generated modified source line to be no longer than 120
characters. Added line 308 is a several-hundred-character Markdown source line and is not generated.

Rule 2694690: Enforce maximum line length of 120 characters
docs/docs/usage-guide/automations_and_usage.md[308-308]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new Bitbucket timeout documentation exceeds the 120-character maximum line length.

## Issue Context
Keep the wording and Markdown rendering intact while splitting the paragraph across physical source lines of at most 120 characters.

## Fix Focus Areas
- docs/docs/usage-guide/automations_and_usage.md[308-308]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

• Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Context sources

✅ Compliance rules (platform): 34 rules

Review mode: ⚖️ Balanced: The push changes runtime async request behavior and host-level timeout handling across two independent Bitbucket paths, creating real correctness and operational risk; it is substantive but not dense enough to warrant redundant extended review.

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[IsmaelMartinez]

Thanks, and the offload does what it says: a one-second blocking call lets a 10ms ticker through 92 times on this branch and zero times on main, with the JWT check still ahead of it so nothing is spawned before auth.

One thing to fix before merge. The new file shares a basename with tests/e2e_tests/test_bitbucket_app.py, and with no __init__.py under tests/ pytest imports both as one module, so a bare pytest dies with import file mismatch before running anything. CI is unaffected because every workflow passes a scoped path. Renaming to test_bitbucket_app_offload.py clears it.

Qodo's global_settings finding I would leave. A repo .pr_agent.toml can override that key through get_settings(), and a repo should not be able to raise a host timeout. Worth knowing its suggested fix routes through get_settings(use_context=False), whose argument is ignored in the function body, so it would reopen the hole rather than close it.

One test gap inline.

[qodo-code-review]

Code review by qodo was updated up to the latest commit c904271

[naorpeled]

@IsmaelMartinez LGTM, let me know if you have any more feedback on this one 🙏

[IsmaelMartinez]

Thanks again, approving and merging!