Skip to content

runtime: make Pingora service worker topology explicit - #73

Open
seonghobae wants to merge 55 commits into
test/parked-read-shutdown-red-v1from
feat/runtime-service-threads-v1
Open

runtime: make Pingora service worker topology explicit#73
seonghobae wants to merge 55 commits into
test/parked-read-shutdown-red-v1from
feat/runtime-service-threads-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Runtime Isolation/Admin Config follow-on for #46, stacked on exact shutdown-correctness candidate #70 (ebdb2c4091aec206fb30174eee9c6c86160bb047).

Problem and authority boundary

Both production composition roots previously inherited Pingora 0.9.0 ServerConf::threads = 1. A many-core/NUMA host therefore did not make the gateway multi-worker merely by exposing CPUs. This PR makes data-plane worker topology explicit without moving product authentication/business logic, Keyverse identity, Wardnet/EgressWeave policy, routing authority, supplier source, or deployment authority into the gateway.

Pingora's exact 0.9.0 source defines ServerConf::threads as the global worker count each service follows unless it supplies a service-level override. HttpProxy also consumes that same global value when constructing its sharded graceful-shutdown notifier. The global value therefore needs to stay aligned with the proxy data plane, but scaling the Prometheus listener by the same factor is neither necessary nor desirable.

Selected contract

  • generic and pg-erd Admin Config admit explicit service_threads;
  • omission preserves the unreleased one-proxy-worker compatibility topology;
  • admitted data-plane range is 1..=256; zero and 257 fail closed;
  • both production composition roots propagate the validated value into Pingora ServerConf::threads, which the proxy follows and HttpProxy uses for shutdown-notifier sharding;
  • both production composition roots override the low-volume Prometheus service to exactly one worker;
  • worker count is never inferred from visible host CPU count;
  • the lower-level explicit-thread ServerConf builder is crate-private;
  • public runtime-composition paths revalidate direct/programmatic or raw-deserialized aggregates before listener activation;
  • the ceiling is a safety bound, not a production recommendation or performance claim.

Proposed ADR 0012, AGENTS, examples, executable contracts and API_CONFIG_CONTRACT.md record these semantics.

Repair history

Hosted review/execution first repaired a stale direct GatewayConfig fixture, a public activation-boundary validation escape, Rust formatter deltas, missing exact-ceiling admission coverage, and a missing Pingora Peer trait import. Exact cd24ee84aac9e44c0228b82bdde10f47206fbc3a then passed formatting, cargo test --all-targets --locked --no-fail-fast, and the normal load contract.

Exact 4ee0657697b4e2003eca95e674597381726e641f made bounded-origin capacity run service_threads: 4 and record configured worker count plus CPU/node/socket topology. That head was terminal GREEN across CI 34467396577, Supply Chain 34467396702, and bounded-origin capacity 34467396707; capacity artifact 10148210022 recorded 1,600 iterations, 3,200/3,200 checks, zero HTTP failures and aggregate/backend/frontend p95 3.50637775/3.39606430/3.56507600 ms on four visible CPUs in one NUMA node/socket.

A subsequent #46 evidence audit found that receipt did not explicitly record the number of registered Pingora services. Forward commits 0b8d6d0cebc3dcb1aa679f25501f5928e63f1eac and 7e41948373a3ad140c25f4dcaf89f8edef77be0b added registered-service evidence and a Rust drift contract tied to the real production composition roots.

That review exposed a deeper performance defect before the new evidence could be promoted: leaving every service on the global worker count would make a high data-plane setting multiply Prometheus workers as well. Pingora 0.9.0 exposes Service::threads: Option<usize> specifically so a service can override the global count, while HttpProxy sizes shutdown sharding from ServerConf::threads. The selected topology is therefore global/data-plane workers for the proxy plus a fixed one-worker metrics override, rather than service_threads × all registered services.

Ordinary forward commits b29d7fb242828df41b47fe7fad83d477b1438e89 and 831eff48d343b53665654797a97d73618563773f apply metrics_service.threads = Some(1) in the generic and pg-erd production composition roots. 50df413faafff138e76477155766ec7ae2e7524d changes capacity evidence to record proxy threads, metrics threads, registered service count and derived configured service-worker slots separately; with the current characterization value this is 4 proxy workers + 1 metrics worker = 5 configured service-worker slots, not a total OS process-thread claim. fce1f50c43f7e086735c9603d92e27898c248da6 fail-closes the regression contract against service-registration or metrics-override drift in both production binaries. b93f6b1c7e8bd063909e15fc8d70d42bb6d48928, 4bc66c3e9c958fc0210186562cbeea0d2883c154, and 144234189374afc845f03d8f2c4d60d21c4eac50 bring ADR 0012, the configuration contract, and AGENTS into the same data-plane-versus-metrics topology.

A final code-current sweep found older rustdoc/error wording in edge_contract and migration_admin still described the field as multiplying every registered service. Ordinary descendants 7690d1743bc92996d579280a9963c72381a8a93a and 08ad492503399115a74b1b0a1f183e882dcd3950 repair those executable-contract descriptions to the actual global/data-plane proxy semantics plus service override. 625cae4f156366bc39d6782161a4a5f336d58624 also renames the two ceiling-test cases from the misleading process_ceiling phrase to data_plane_ceiling. No validation threshold or runtime behavior changed in those final three commits.

No route, timeout, request/response policy, supplier identity, traffic sample count/concurrency, p95 threshold, product-domain policy or security authority was changed by this follow-on.

Current exact head / evidence closure

Current exact head is 625cae4f156366bc39d6782161a4a5f336d58624, a strict ordinary-forward descendant of exact base ebdb2c4091aec206fb30174eee9c6c86160bb047. It has independently reacquired all normal current-head evidence after the final source/test/documentation movement; no predecessor GREEN is transferred.

  • CI 34478471643 is terminal GREEN. load-contract, test, and oci-runtime all passed. The test lane passed exact checkout, Rust 1.98.0 formatting, locked compile/tests, Clippy, warnings-denied public rustdoc, pinned coverage tooling, complete owned-production coverage enforcement, and resolved dependency-lock verification. Both admitted OCI profiles were rebuilt and exercised under declared non-root/read-only least-privilege constraints.
  • Supply Chain 34478471699 is terminal GREEN through exact checkout, committed dependency audit, both admitted candidate-image builds, SPDX dependency SBOM generation, both exact image scans, exact-source binding, and evidence upload.
  • PgErd bounded-origin capacity 34478471632 is terminal GREEN. Exact-source artifact 10152648179, digest sha256:b91a3786daea4786af334fd31b7b0ab19d828d8b6d178685215ebe26a3dcdf34, is bound to this head. The evidence model records configured_proxy_service_threads, configured_metrics_service_threads, registered_service_count, configured_service_worker_slots, online CPUs and CPU→node/socket mapping. For the current two-service characterization, the configured worker-slot arithmetic is 4 proxy workers plus the one metrics worker; this is deliberately not described as total OS process threads.
  • Exact-current technical reviews 5167316759 and terminal follow-up 5167331074 re-read the complete current range and found no additional actionable writer-safe source/test/documentation/DDD or authority-boundary defect. They are technical COMMENT evidence only, not independent APPROVED governance credit.

This closes the branch-local implementation/execution gate for independent review, so the PR is Ready rather than Draft.

Remaining performance and promotion boundary

#46 still requires the largest representative Linux/NUMA-capable environment with the exact configured proxy worker count, registered service count/service overrides, CPU/socket/NUMA topology, high parked/reused HTTP/1 keep-alive pressure, repeated shutdown jitter, zero correctness survivors, shutdown wall-clock distribution/tail, CPU/scheduler behavior, and where available off-CPU/futex evidence. No worker/sample/connection reduction is admissible merely to remove contention. The ordinary GitHub-hosted low-core result cannot supply commercial NUMA scaling closure.

No independent APPROVED governance review is claimed. #70 correctness, #54/#889 derivative removal, #71/#447 whole-header lifetime, #72/#1000 parser admission, H2→H1 Cookie normalization, zero-length framing, #56 approval/governance, immutable release, shadow/canary, rollback, cutover, and legacy removal remain separate dependency-ordered gates. No protected merge, release, canary/shadow, rollback, cutover, or Nginx/OpenResty removal credit is claimed.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-current technical review on 4ee0657697b4e2003eca95e674597381726e641f: re-read the 16-path #70#73 range, with focused source inspection of the generic/pg-erd Admin Config validation, runtime composition, crate-private Pingora ServerConf builder, both production binary composition changes, exact worker-boundary regressions, ADR 0012, and the bounded-origin harness. Authoritative Pingora 0.9.0 source defines ServerConf::threads as the worker count for each service and defaults it to 1; this candidate makes that topology explicit without deriving it from host CPU count or moving product/security authority into the gateway. The current capacity harness exercises service_threads: 4 and records its topology rather than claiming a process-wide four-thread total.

All normal exact-head execution is terminal GREEN: CI 34467396577 passed load-contract, OCI runtime, locked tests, Clippy, warnings-denied rustdoc, complete owned-production coverage and lock verification; Supply Chain 34467396702 passed dependency audit, candidate builds, SPDX SBOM, both image scans and exact-source binding; capacity 34467396707 passed 1,600 iterations / 3,200 checks with zero HTTP failures. The source-bound artifact records 4 configured workers per service on a 4-CPU, single-node/single-socket hosted runner and p95 3.50637775 ms aggregate, 3.39606430 ms backend, 3.56507600 ms frontend. That is valid multi-worker composition/traffic evidence but not representative NUMA scaling or contention closure for #46.

I found no additional actionable writer-safe source/test/documentation/DDD or authority-boundary defect in this current range. This COMMENT is technical evidence only, not an independent APPROVED governance review, and it does not grant protected merge, release, canary/shadow, cutover, or legacy-removal credit.

@seonghobae
seonghobae marked this pull request as ready for review September 10, 2026 11:23
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@seonghobae
seonghobae marked this pull request as draft September 10, 2026 12:25

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-current technical re-read on 625cae4f156366bc39d6782161a4a5f336d58624: reviewed the 16-path #70#73 range after the later registered-service/evidence repairs, both production composition roots, Admin Config validation/revalidation, crate-private ServerConf construction, runtime-composition boundary, capacity harness and drift contract. Fresh Pingora 0.9.0 source confirms a service-level threads() value overrides ServerConf::threads and None falls back to the global value, so keeping the proxy on validated global service_threads while fixing Prometheus at Some(1) matches supplier runtime semantics rather than multiplying every registered service. The current regression also fail-closes service-registration/metrics-override drift. No additional actionable gateway-local source/test/documentation/DDD or authority-boundary defect is identified in this exact range.

Current exact CI 34478471643 is terminal GREEN: load-contract, locked compile/test, Clippy, warnings-denied rustdoc, complete owned-production coverage, resolved-lock verification and both least-privilege OCI profiles passed. Exact bounded-origin capacity 34478471632 is also GREEN and source-bound to artifact 10152648179 (sha256:b91a3786daea4786af334fd31b7b0ab19d828d8b6d178685215ebe26a3dcdf34). Supply Chain 34478471699 is still executing after exact checkout, toolchain setup and committed dependency audit passed, so this COMMENT does not transfer predecessor Supply Chain credit or authorize Ready/merge before it terminates. Representative many-core/NUMA contention evidence remains a separate #46 requirement. Technical COMMENT only; not an independent APPROVED review, protected merge, release, canary/shadow, cutover, or legacy-removal credit.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terminal exact-current follow-up on 625cae4f156366bc39d6782161a4a5f336d58624: the Supply Chain lane that was still executing during review 5167316759 has now terminated GREEN without head movement. CI 34478471643 is GREEN across load-contract, locked compile/test, Clippy, warnings-denied public rustdoc, complete owned-production coverage, resolved-lock verification and both least-privilege OCI profiles. Supply Chain 34478471699 is GREEN through committed dependency audit, both admitted image builds, SPDX SBOM generation, both exact image scans and exact-source binding. Bounded-origin capacity 34478471632 remains GREEN with exact-source artifact 10152648179, digest sha256:b91a3786daea4786af334fd31b7b0ab19d828d8b6d178685215ebe26a3dcdf34.

The exact source range is unchanged since the prior re-read and no review thread exists, so no further writer-safe source/test/documentation repair is justified. This closes the #73 branch-local technical/execution gate and makes it suitable for independent review. It does not close #46 representative many-core/NUMA contention acceptance and is not an independent APPROVED review, protected-merge, release, canary/shadow, cutover, or legacy-removal credit.

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