alibaba / alibaba/open-code-review

Add a provider-wide per-attempt concurrency gate

Open
#787 0 comments 2 reactions 0 assignees View on GitHub
enhancement
Dominant language
Go
Stars
24.4k
Forks
1.8k
Avg merge
2d 6h
Merged PRs (30d)
105

Description

### Problem Statement

Part of #368

OCR uses the official LLM SDK retry loops with `WithMaxRetries(5)`. Those SDKs already own request-local retry eligibility, retry count, backoff, server retry hints, sequential attempts, and cancellation.

OCR has concurrency limits today, but they do not form one provider-wide limit. The current review topology is:

- A file worker holds one `--concurrency` slot while it runs the optional plan request, the main loop (including any grace round), waits for that file's comment jobs, and then runs the review filter. Those foreground requests are serial within the file slot.
- The main loop may start one asynchronous memory-compression job per file conversation. That request can overlap the file's foreground main request.
- Comment post-processing runs in a separate worker pool derived from `--concurrency`. A worker may call the model for re-location after deterministic in-file resolution and cross-file relocation both fail.

Plan and review-filter requests are therefore not independent overflow sources. The aggregate excess comes from overlapping foreground file work, asynchronous compression, and the separate comment pool. Lowering `--concurrency` reduces pressure on more than one path, but it does not establish a single provider-wide maximum across them.

The missing capability is provider-wide admission, not another retry loop.

### Proposed Solution

Keep the official SDK as the sole retry owner and add one provider-scoped, context-aware concurrency gate shared by all `ocr review` LLM request paths in a run.

#### Scope review requests explicitly

- Mark the review run with a dedicated admission-scope context value and preserve it through child contexts, including `context.WithoutCancel` paths.
- Do not use retry-report `RequestMeta` as the admission switch. Retry identity and admission scope are separate concerns, and the existing grace-round request has no `RequestMeta`.
- Gate plan, main-task, grace-round, memory-compression, re-location, and review-filter requests.
- Requests from `ocr scan` and `ocr llm test` do not carry the review admission scope and remain unchanged.

#### Admit one real attempt at a time

Install the gate at the shared SDK middleware boundary, inside the SDK retry loop:

1. Before an eligible attempt reaches the retry observer and transport, acquire one provider permit using the request context.
2. If acquisition is cancelled, return without calling the transport and without leaking waiter or permit state.
3. If the transport returns an error, no response, or a response without a body, release the permit immediately.
4. If a response body is returned, transfer permit ownership to an idempotent body wrapper and release it on EOF or `Close`.
5. Retryable responses are closed by the SDK before it enters backoff, so SDK backoff holds no provider permit.
6. Streaming responses keep their permit until the stream body reaches EOF or is closed; receiving response headers alone does not release capacity.

This defines an in-flight provider attempt as one whose transport has not returned or whose response body remains open.

#### Preserve retry observability

Place admission outside the retry observer so `duration_to_headers_ms` still measures transport time from request send to response headers and excludes admission waiting.

Keep the `ocr.llm-retry-report/v1` schema unchanged. `observed_backoff_ms` remains the measured interval between two real attempts, not the SDK's planned sleep, so the interval may include provider-admission waiting. Document that interpretation rather than adding or changing report fields.

The provider configuration field, default value, and compatibility behavior must be agreed before implementation. File-worker concurrency and provider HTTP concurrency control different resources and must not be treated as interchangeable without an explicit decision.

## Acceptance criteria

- A configured provider can define a fixed maximum number of in-flight attempts for an `ocr review` run.
- Plan, main-task, grace-round, memory-compression, re-location, and review-filter requests share the same provider gate.
- An active attempt includes both transport execution and an open response body; mixed request paths never exceed the configured provider maximum.
- Admission is applied per real SDK attempt, not around the whole logical LLM request.
- Permit acquisition is context-aware. Cancellation while queued starts no HTTP request and leaves no waiter or permit behind.
- Transport errors and body-less responses release capacity immediately.
- Response-body EOF and `Close` release capacity exactly once.
- Streaming responses remain admitted until the stream ends or closes.
- Retryable responses release capacity before SDK backoff, allowing another eligible request to use the permit during the wait.
- The official SDK remains the only retry owner. OCR adds no outer retry loop and does not change retry eligibility, retry count, backoff, or server-hint behavior.
- Admission scope is independent of retry-report identity: every review request, including the grace round, is gated, while `scan` and `llm test` remain ungated.
- `ocr.llm-retry-report/v1` remains backward-compatible. Admission waiting is excluded from `duration_to_headers_ms` and may be included in the measured `observed_backoff_ms` interval.
- One logical request still has at most one active attempt, and admission creates no duplicate request.
- Tests cover mixed foreground/compression/re-location concurrency, plan/grace/filter scope, concurrent 429/529 retries, release before backoff, cancellation while queued, transport failure, body EOF/close idempotence, a held streaming body, scan/llm-test bypass, and retry-report timing compatibility.

## Dependencies

- #785 provides real-attempt observation and `ocr.llm-retry-report/v1`; this issue preserves that schema and its real-attempt ordering.
- This issue owns the fixed provider-wide admission limit required by #368. Repository workflows may configure or display the limit but must not implement provider admission themselves.

## Out of scope

- An OCR-owned retry loop or changes to official SDK retry policy.
- A total retry-count budget beyond the SDK's existing bounded retry count.
- Adaptive concurrency, cooldown windows after 429/529, or dynamic rate estimation.
- Request prioritization or provider scheduling beyond the fixed admission limit.
- Trusted Resume, checkpoint reuse, or resume lineage.
- Provider ranking, automatic provider selection, or implicit cross-provider failover.
- Behavior changes to `scan` or `llm test`.
- Changes to the `ocr.llm-retry-report/v1` schema.

## Status

The issue was re-scoped after verifying the official SDK retry behavior, OCR's concurrency topology, retry-observer timing, the grace-round identity gap, and streaming response lifetime. The remaining gap is a shared provider-wide limit on live review attempts.

Design and implementation have not started. The configuration surface and default semantics must be aligned before code changes.

### Alternatives Considered

#### Gate the whole logical LLM request

Rejected because the permit would remain occupied while the SDK sleeps in backoff, allowing background work to block otherwise eligible foreground requests.

#### Release the permit when response headers arrive

Rejected because an OpenAI streaming response remains active after headers. Releasing at the middleware return would allow live streams to exceed the configured provider maximum.

#### Use retry-report `RequestMeta` as the admission switch

Rejected because admission and observability are separate contracts, and the existing grace-round request carries no retry-report identity.

#### Reuse `--concurrency` as the provider limit

Not selected by default because file-worker concurrency and provider HTTP concurrency control different resources. Whether to derive one from the other remains part of the configuration decision.

### Affected Area

Review Agent / LLM interaction

### Additional Context

_No response_

Contributor guide

Open the contributing guide

Research direction

Start with the shared SDK middleware boundary and trace the request paths for `ocr review`, including plan, main-task, grace-round, memory-compression, re-location, and review-filter requests. Compare them with `ocr scan` and `ocr llm test`, which should bypass admission. Done means the acceptance criteria pass, including context-aware cancellation, response-body lifetime, retry backoff release, mixed-path limits, and unchanged `ocr.llm-retry-report/v1` timing.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, distributed-systems
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.