Skip to content

perf: collapse per-request authority round trips and add load harness - #268

Merged
steipete merged 3 commits into
mainfrom
perf/hot-path-load-harness
Oct 12, 2026
Merged

steipete merged 3 commits into
mainfrom
perf/hot-path-load-harness

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Summary

Production measurements showed every authenticated request paying ~70–85 ms per sequential call to the global ACCESS_CONTROL authority, with five calls per inference and four per catalog read. That added ~270 ms to a catalog read and ~530–650 ms to time-to-first-token versus direct Anthropic, and capped authenticated throughput at ~100 req/s (98 rps at concurrency 100, 102 rps at 200, p99 9.8 s), all with zero errors.

  • Combined admission read (/admission/resolve): credential, policy, owner, connections and grant-pool membership/runtime state in one authority invocation, with the same migration fences, revocation/generation checks and error outcomes as the single-purpose resolvers. The Worker still verifies the secret digest in constant time; nothing authorization-related is cached across requests. Selection stays a separate atomic call (counter now via RETURNING).
    • Inference (native, unified, manifest, WebSocket create): 5 → 2 authority calls; principal-owned keys 6 → 2.
    • /v1/models / /v1/catalog: 4 (or 3+N) → 1. Key inspection, usage, pool: 2–3 → 1.
  • Pre-auth hardening: the Worker sends the secret digest with admission; the authority compares it in constant time and returns before policy, connection or pool reads, so a known key id cannot drive pool work on the singleton.
  • No deep prompt copies: top-level request-field renames no longer structuredClone the whole prompt/tool tree (256 KiB transform: 55.85 → 0.167 ms per 1,000 calls).
  • Load harness: pnpm loadtest (live, key from a 0600 file, latency/TTFB/lane/failover/stream-abort stats) and pnpm loadtest:local (workerd + fake Anthropic upstream, 7-grant subscription pool mirroring production). Findings and traces in docs/hot-path-performance.md.

Verification

  • pnpm check, pnpm worker:e2e; 1,253 Worker tests incl. 30 admission parity tests (revoked/stale/invalid keys, disabled policies/users, absent owners/connections, model denial, unavailable pools, environment fallback, migration races, wrong-secret short-circuit).
  • Local A/B on an idle 32-core Linux host, interleaved 2×, before (b827638) → after (78c04be), 200 requests per cell, zero errors in 7,200 requests:
Scenario @ concurrency TTFB p50 Throughput
catalog @ 200 546–591 → 426–475 ms 335–363 → 408–438 rps
messages @ 100 254–266 → 195–199 ms 360–379 → 451–460 rps
stream @ 100 222–224 → 128–178 ms 308–318 → 358–376 rps

Local workerd has no network between Worker and Durable Objects, so it understates the edge gain; production before/after numbers follow the deploy.

Remaining (design, not in this PR)

Global singleton still serializes selection writes and admission for all tenants; per-grant credential owners serialize materialization; budget ledgers are shared. See docs/hot-path-performance.md ranked findings 1–4, 6–7.

@clawsweeper

clawsweeper Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Oct 12, 2026
@clawsweeper

clawsweeper Bot commented Oct 12, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge.

What this changes

This PR combines proxy admission reads, reduces request copying, and adds live and local load-testing commands.

Example: A warmed Anthropic inference request uses a principal-owned proxy key.

  • Before: Credential, policy, owner, connection, pool resolution, and selection require six authority calls.
  • After: Combined admission and atomic selection require two authority calls.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) Useful runtime measurements support the optimization, but the introduced provider-enforcement bypass blocks merging.
Proof confidence 🦐 gold shrimp (3/6) Real behavior proof is necessary before merge. See Before merge.
Patch quality 🧂 unranked krab (1/6) Security review found an item that needs attention.

Product

Kind: Performance · Worth it: Yes · Fix scope: Complete
User problem: Authenticated inference and discovery pay repeated round trips to the global authority, increasing latency and limiting throughput.
Reason: The recorded owner direction and measured round-trip reduction justify the focused optimization and load tooling.

Merge readiness

⛔ Blocked before merge - 4 items remain

Keep open: the optimization has clear value, but reusing provider state captured before body upload introduces a kill-switch and budget-enforcement regression.

Priority: P2
Reviewed head: 56fbddd30595b0fc6ed1dd15538770725a50c509

Before merge

  • Add real behavior proof - Authority-chain proof required: supplied workerd HTTP results demonstrate successful combined admission, selection, and accounting against a synthetic upstream, but do not show a delayed upload rejected after provider disablement or a zero-budget update before final provider I/O; provide redacted terminal output or logs for that scenario. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Recheck provider authority after consuming request bodies (P1) - Native and manifest requests now capture the provider connection during preauthentication, before awaiting their JSON body. This line then skips the later canonical lookup. Start POST /v1/messages while Anthropic is enabled with monthlyBudgetMicros=null, pause the upload after admission, commit enabled=false or monthlyBudgetMicros=0, and finish a valid body. Preparation accepts the earlier connection and can dispatch upstream; with the earlier null limit, budget planning also omits the provider reservation. Main reads the connection after body parsing and rejects these cases. Refresh or defer this observation until after body consumption before using it for authorization and reservation.
  • Resolve security concern: Stale provider authority reaches upstream dispatch - The affected boundary is operator-controlled provider access and spending; request-local storage alone does not make the captured state current after caller-controlled upload waits.
  • Explain or restore the original intent of Provider connection and budget enforcement - feat: per-provider monthly budget limits #109: Provider connection state supplies the kill switch and monthly budget used during preparation and reservation.

Findings

  • [P1] Recheck provider authority after consuming request bodies — worker/providers.ts:210-212
  • [high] Stale provider authority reaches upstream dispatch — worker/providers.ts:212

Provenance

  • Provider connection and budget enforcement changes intended behavior without addressing why it exists (feat: per-provider monthly budget limits #109: Provider connection state supplies the kill switch and monthly budget used during preparation and reservation.)

Tests

  • Missing end-to-end proof: Add a recorded workerd HTTP scenario that pauses body upload, changes canonical provider enablement or its budget, and proves rejection before upstream I/O; show the regression failing on the head and passing after repair.
Agent review details

How this fits together

The Worker authenticates incoming inference requests, selects provider credentials, reserves budgets, dispatches upstream, and publishes usage.

flowchart LR
  Client --> Worker
  Worker --> AdmissionAuthority
  AdmissionAuthority --> RequestIdentity
  RequestIdentity --> ProviderPreparation
  ProviderPreparation --> BudgetLedger
  BudgetLedger --> ProviderDispatch
  ProviderDispatch --> UsageAccounting
Loading

Technical review

Best possible solution:

Retain combined admission while checking provider enablement and budget limits after request-body consumption.

Do we have a high-confidence way to reproduce the issue?

The source establishes the failing sequence: capture an enabled, unmetered Anthropic connection, hold the Messages body open, disable the provider or set its budget to zero, then finish the upload; no target code was executed during this review.

Is this the best way to solve the issue?

Combining authority reads and avoiding deep copies are appropriate optimizations, provided they preserve the existing provider-enforcement point after body parsing.

Full review comments:

  • [P1] Recheck provider authority after consuming request bodies — worker/providers.ts:210-212
    Native and manifest requests now capture the provider connection during preauthentication, before awaiting their JSON body. This line then skips the later canonical lookup. Start POST /v1/messages while Anthropic is enabled with monthlyBudgetMicros=null, pause the upload after admission, commit enabled=false or monthlyBudgetMicros=0, and finish a valid body. Preparation accepts the earlier connection and can dispatch upstream; with the earlier null limit, budget planning also omits the provider reservation. Main reads the connection after body parsing and rejects these cases. Refresh or defer this observation until after body consumption before using it for authorization and reservation.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 6e265875a714.

Provenance checked

Testing

Proof path: shipped entry point.

Security

Needs attention: Provider authority captured before upload can outlive an operator's disable or budget change.

Evidence

Security concerns:

  • [high] Stale provider authority reaches upstream dispatch — worker/providers.ts:212
    The affected boundary is operator-controlled provider access and spending; request-local storage alone does not make the captured state current after caller-controlled upload waits.
    Confidence: 0.97

What I checked:

  • Introduced authority reuse: The pinned introduced diff replaces the provider connection lookup with the connection stored in the request's admission result. (worker/providers.ts:212, 56fbddd30595)
  • Caller-controlled delay before provider preparation: Native requests resolve provider admission at line 215, await the complete JSON body at line 226, and subsequently reuse the earlier connection; the public Messages endpoint follows this native path. (worker/proxy.ts:215, 56fbddd30595)
  • Budget consequence: Budget planning consumes the supplied connection limit; an earlier null limit omits the provider reservation even if the operator has since set the canonical limit to zero. (worker/accounting.ts:55, 56fbddd30595)
  • Original enforcement contract: Current main reads the connection during provider preparation, after body parsing; feat: per-provider monthly budget limits #109 introduced provider-budget reservation and the provider_budget_exhausted response. (worker/providers.ts:209, 6e265875a714)
  • Runtime evidence and its boundary: The supplied workerd measurements exercise successful HTTP admission, selection, and accounting through real Worker and Durable Object owners with a synthetic upstream; the documented parity tests do not cover a provider-state change during body upload. (docs/hot-path-performance.md:379, 56fbddd30595)
  • Policy and provenance: The full root AGENTS.md requires authoritative revocation and hard-budget enforcement; no nested AGENTS.md or maintainer-notes directory exists. Historical blob reads encountered the checkout's lazy-fetch limit; relevant current-main source, history metadata, and original PR bodies remained available. (AGENTS.md:14, 56fbddd30595)

Likely related people:

  • Peter Steinberger: Raw commit 98e78e8 adds worker/providers.ts:192 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 98e78e8709f3; files: worker/providers.ts)
  • Vincent Koc: Raw commit 34b9759 adds worker/proxy-auth.ts:17 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 34b97591e353; files: worker/proxy-auth.ts)

Labels

Label changes:

  • add P2: A useful performance improvement requiring correction before release; finding severity is assessed separately.
  • add merge-risk: 🚨 security-boundary: An authenticated caller can retain earlier provider authority across a delayed upload and dispatch after the provider kill switch or hard budget changes.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Label justifications:

  • P2: A useful performance improvement requiring correction before release; finding severity is assessed separately.
  • merge-risk: 🚨 security-boundary: An authenticated caller can retain earlier provider authority across a delayed upload and dispatch after the provider kill switch or hard budget changes.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 11, 2026, 11:26 PM ET / October 12, 2026, 03:26 UTC.

@steipete
steipete merged commit 311b4e5 into main Oct 12, 2026
14 checks passed
@steipete
steipete deleted the perf/hot-path-load-harness branch October 12, 2026 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant