<:robot:> ## The problem `PoolConfig` and `RetryPolicy` describe behavior that upstream contracts already describe, in some cases better. DM-6's definition of done calls for reconciling them, "duplicated knobs removed rather than re-specified." This issue does that reconciliation. It is the companion to [bench#85](https://gitlab.com/gitlab-org/lab-bench/bench/-/work_items/85), which removed Postgres *identity* fields and deliberately left pool and retry alone because they are driver behavior, not identity. ## `PoolConfig` is a strict subset of upstream `gitlab.infrastructure.v1.PostgreSQL` covers every field bench has, and three it does not: | bench `PoolConfig` | upstream equivalent | |---|---| | `max_open` | `max_conns` — maps to `pgxpool.Config.MaxConns` | | `max_idle` | `min_conns` — maps to `pgxpool.Config.MinConns` | | `idle_timeout` | `conn_max_idle_time` | | `max_lifetime` | `conn_max_lifetime` | | — | `query_exec_mode` | | — | `pool_host` | | — | `pool_port` | The three bench lacks are not incidental. `query_exec_mode` defaults to `SIMPLE_PROTOCOL`, which upstream documents as *"required for PgBouncer in transaction-pooling mode (the standard deployment at GitLab.com)."* So bench's schema cannot describe the configuration most GitLab services actually run under, and `pool_host`/`pool_port` mean it also cannot express a pooler endpoint distinct from the database. `max_idle` is worth a second look during implementation: it maps to `min_conns`, but the names imply different intents — "maximum idle connections retained" versus "minimum connections kept warm." Anything relying on bench's reading needs checking before the field disappears. ## `RetryPolicy` is a partial overlap, not a subset Different situation, and it should not get the same treatment reflexively. `v2/httpclient`'s `DoWithRetry` already takes configurable retryable status codes for HTTP. But `RetryPolicy` also reaches `GRPCBackendConfig`, and `Outbound.default_retry` applies before a backend type is known. So deferring wholesale to the HTTP client would leave gRPC and the assembly-level default with nothing. ### The field that is wrong regardless `retryable_status_codes` is a single `repeated int32` documented as "HTTP / gRPC status codes". Those are different numeric spaces: HTTP `429` is Too Many Requests, gRPC `14` is `UNAVAILABLE`, and HTTP `14` is meaningless. A spec containing `14` cannot be read without knowing the backend type, and a policy meaning "retry on HTTP 429 and gRPC UNAVAILABLE" cannot be written at all. `docs/design/schema-change-process.md` already uses this exact field as its worked breaking-versus-additive example and prescribes the fix: > **Additive:** keep `retryable_status_codes` (mark it deprecated in a > comment), add `retryable_http_status_codes` and > `retryable_grpc_status_codes`. Old configs still parse; the validator > enforces the new rule (e.g. "do not set both the old and new fields"). > `buf breaking` passes; no version bump needed. So that part needs no design, only doing. The schema currently carries a pointer to this in a proto comment; this issue is where it gets resolved. ## Scope - Delete `PoolConfig`'s four fields with reserved numbers and names, leaving Postgres pool tuning to `gitlab.infrastructure.v1.PostgreSQL` - Confirm the `max_idle` / `min_conns` semantic difference before removing it - Decide `RetryPolicy`'s fate per protocol rather than wholesale: HTTP may defer to `DoWithRetry`, gRPC has no upstream counterpart - Split `retryable_status_codes` by protocol, following the additive path already documented - Keep the assembly-level `Outbound.default_retry` meaningful without knowing the backend type - Remove the `RetryPolicy` proto comment flagging the ambiguity once resolved ## Not in scope `CircuitBreaker`, which has no upstream counterpart at all and belongs to [bench#78](https://gitlab.com/gitlab-org/lab-bench/bench/-/work_items/78). That issue also carries the `failure_threshold` float-precision change. Non-Postgres pool config. Redis and object storage have upstream contracts but no bench backend type yet, and both are out of scope for Phase 1. ## Acceptance criteria - [ ] `PoolConfig`'s fields are removed with numbers and names reserved, or each retained field has a documented reason upstream cannot express it - [ ] The `max_idle` / `min_conns` semantic difference is checked, and any example relying on the old reading is migrated - [ ] `RetryPolicy`'s disposition is decided per protocol, with gRPC and `Outbound.default_retry` still expressible - [ ] `retryable_status_codes` is unambiguous about protocol, via the additive path in `docs/design/schema-change-process.md` - [ ] The validator rejects setting both the deprecated field and its replacements - [ ] The ambiguity comment on `RetryPolicy` in `outbound.proto` is removed - [ ] `docs/reference/proto.md` is regenerated and `buf breaking` outcomes are deliberate and documented Part of [DM-6: Phase 1 Outbound](https://gitlab.com/groups/gitlab-org/lab-bench/-/epics/3). </:robot:>