Skip to content

perf: admit and select inference grants in one authority call - #269

Merged
steipete merged 1 commit into
mainfrom
perf/single-hop-inference
Oct 12, 2026
Merged

steipete merged 1 commit into
mainfrom
perf/single-hop-inference

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #268. Warm subscription inference made two sequential calls to the global ACCESS_CONTROL authority plus a KV read before the credential owner (/admission/resolve → KV grant projections + local filters → /grant-pools/select). In production each authority hop costs ~70–85 ms.

  • Admit-and-select in one authority invocation. When the Worker passes a selection request, admission filters eligibility and performs the atomic selection (strategies, stickiness, priority tiers, quota reserve, model-scoped exhaustion, cooldowns, stale-state rule, exclusions, environment-fallback signalling) and increments counters in the same synchronous transaction. Eligibility and selection share the existing functions — one owner, no drift.
  • Secretless routing projections in the authority, written by the credential owner's publication path (additive table, revision/marker checks so a delayed KV read cannot overwrite an owner publication; bounded reads ≤66 keys). Rows without a projection keep the previous two-step path until the owner publishes.
  • Credential cache intentionally not added: revocation deletes owner secrets even when authority publication fails, so the authority can briefly report an old active generation; a generation-keyed cache could bypass revocation. A regression reproduces that state and proves materialization rejects it.

Warm proxy-key HTTP inference: 2 authority calls + KV + credential owner → 1 authority call + credential owner. Access-session, pinned continuations and WebSocket paths unchanged.

Verification

  • pnpm check: 1,301 Worker / 224 admin / 343 script tests; pnpm worker:e2e; autoreview clean through P2 (fixed legacy-failover scope/stickiness races found in review).
  • Selection-parity and hop-count tests; revoke, reauth_required, policy disable and rotation tests.
  • Local A/B on an idle 32-core host, interleaved 2×, main (311b4e5) → this branch (5316377), zero errors in 7,200 requests:
Scenario @ concurrency TTFB p50 Throughput
messages @ 100 207–209 → 171–177 ms 446–462 → 536–548 rps
stream @ 100 203–236 → 157–159 ms 326–363 → 392–445 rps
messages @ 50 129–133 → 101–117 ms 359–370 → 394–442 rps
catalog unchanged unchanged

Local workerd has no Worker↔DO network hop; production saves one more authority round trip on top.

Publish secretless routing projections with credential-owner attachment state
and select eligible grants in the admission transaction. Share eligibility,
strategy and sticky-input behavior with the legacy path; fence bounded KV
backfills and retain fresh failover authorization.

Keep per-request credential-owner materialization: an owner can revoke secrets
while authority publication is unavailable, so the authority's old generation
cannot safely authorize cached tokens. Document this work-order escape hatch.

Validate with pnpm check, pnpm worker:e2e, selection/migration/revocation
regressions and the synthetic local load sweep. Independent autoreview is clean.
@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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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-key HTTP admission and grant selection in one authority call using persisted, secretless routing projections.

Example: A warm POST /v1/messages request for anthropic/claude-sonnet-5 using a subscription grant.

  • Before: Admission and selection require two foreground authority calls, a KV projection read, and credential-owner materialization.
  • After: Admission and selection require one foreground authority call and credential-owner materialization.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful runtime-backed optimization with strong source and parity coverage; authority-boundary and persisted-upgrade gates remain unresolved.
Proof confidence 🦐 gold shrimp (3/6) Real behavior proof is necessary before merge. See Before merge.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Product

Kind: Performance · Worth it: Yes · Fix scope: Complete
User problem: Authenticated inference pays an additional sequential authority hop and KV read before reaching its credential owner.
Reason: The maintainer-authored direction has concrete latency value, preserves routing policy and defaults, and avoids an unsafe credential cache.

Merge readiness

⛔ Blocked before merge - 4 items remain

Keep this PR open: it delivers a useful, distinct performance improvement beyond current main, with no definite introduced code defect found.

Priority: P2
Reviewed head: 53163776ccd83330fc5badac504baaad6e386954

Before merge

  • Add real behavior proof - Authority-chain proof required: the supplied workerd artifact exercises real HTTP ingress, authority selection, credential ownership, and synthetic provider transport successfully; nearest-forbidden caller evidence is confined to tests, and the stale-publication revocation case stops at helper rejection rather than an observed HTTP-to-provider boundary. Supply redacted runtime logs or terminal output showing allowed dispatch and denied dispatch, including revocation during failed authority publication; redact API keys, IPs, and non-public endpoints. 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.
  • Resolve security review attention item - No concrete bypass was found, but the persisted authority cutover needs final-effect evidence under revocation and publication failure.
  • Resolve merge risk - Before merge, establish at the provider dispatch boundary that stale active authority projections cannot bypass credential-owner revocation during an authority-publication outage.
  • Add data-model compatibility proof - The review found that existing stored data may not work after upgrade. Show that existing data still loads and works with this change.

Findings

None.

Tests

  • Missing end-to-end proof: Supply an allowed-versus-forbidden HTTP dispatch trace covering failed revocation publication, plus a persisted-state upgrade from v0.4.0 with settings, credentials, and routing intact.
Agent review details

How this fits together

The Worker prepares an inference operation, the access authority validates its caller and selects a scoped grant, and the credential owner supplies current secrets before budgeted provider dispatch.

flowchart LR
A[HTTP inference request] --> B[Worker operation preparation]
B --> C[Access authority admission and selection]
D[Secretless routing projections] --> C
C --> E[Credential owner materialization]
E --> F[Budget and retention checks]
F --> G[Provider dispatch and accounting]
Loading

Technical review

Best possible solution:

Land the combined selection path with per-request credential materialization after final-effect authority evidence and retained-state upgrade validation.

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

This is a performance optimization; current-main source independently confirms the extra selection call and KV read targeted by the change.

Is this the best way to solve the issue?

Combining selection with admission is a sound approach because both paths share eligibility rules and the existing selector, while credential materialization remains authoritative for revocation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 311b4e5bc2b5.

Merge-risk options

Maintainer options:

  1. Complete authority-boundary evidence (recommended)
    Record real HTTP requests through combined admission for an allowed caller and the nearest forbidden caller, including owner revocation while authority publication fails, with an upstream trace showing rejection before provider I/O.
  2. Pause the persisted selection cutover
    Retain the existing main path until the authority-boundary evidence is available.

Provenance checked

Testing

Proof path: shipped entry point.

Security

Needs attention: No concrete bypass was found, but the persisted authority cutover needs final-effect evidence under revocation and publication failure.

Evidence

What I checked:

  • Repository policy applied: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory exists. Applied provider-neutral ownership, secret handling, and authoritative revocation guidance; the read-only review instruction supersedes running tests or autoreview. (AGENTS.md:1, 53163776ccd8)
  • Distinct work beyond main: The pinned introduced diff moves selection into admission; the merged predecessor explicitly retained a separate selection call. This PR remains necessary beyond perf: collapse per-request authority round trips and add load harness #268. (worker/authority-grant-selection.ts:52, 53163776ccd8)
  • Runtime evidence supports warm-path behavior: The supplied workerd artifact records 800 inference requests with 800 selections and usage events, zero unexpected errors, and one foreground admission call per request. Inspection of the load driver confirms HTTP ingress into the real Worker and Durable Objects with synthetic upstream transport. (docs/performance/single-authority-selection-local.json:1, 53163776ccd8)
  • Revocation boundary retained: Selection still reaches credential-owner materialization on every request. The publication-outage regression demonstrates stale active authority metadata after secret deletion, but ends at a rejected materialization helper rather than observing HTTP dispatch and final provider I/O. (worker/test/single-hop.test.mjs:119, 53163776ccd8)
  • Persisted cutover needs upgrade evidence: The additive SQLite projection table and durable absence/legacy markers change persisted routing state. Supplied fixtures exercise backfill and concurrent mutation, but no retained-state upgrade from v0.4.0 is recorded. (worker/authority.ts:130, 53163776ccd8)
  • Prior ownership contract respected: fix(grants): sequence pool attachments through credential owners #210 established owner-sequenced publication, revision fences, and secret deletion before derived index writes; this PR extends that publication path and deliberately avoids credential caching. (worker/grant-credentials.ts:468, 032b0dfa9aa9)

Likely related people:

  • Peter Steinberger: Raw commit 311b4e5 adds worker/proxy-auth.ts:23 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 311b4e5bc2b5; files: worker/proxy-auth.ts)
  • Vincent Koc: Raw commit 51a2742 adds worker/grant-credentials.ts:186 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 51a2742a3ae8; files: worker/grant-credentials.ts)
  • Jason (Json): Raw commit d798102 adds worker/grant-credentials.ts:173 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: d798102e4af7; files: worker/grant-credentials.ts)

Review metrics

Metric Value Why it matters
Diff composition Production +365/-168; tests +298/-31; documentation and artifact +990/-3 Production growth adds fenced persisted projections and transactional selection while extracting existing eligibility logic; most total growth is evidence and tests.

Labels

Label changes:

  • add P2: Reduces inference admission overhead without establishing an urgent user-facing outage.
  • add merge-risk: 🚨 security-boundary: Persisted routing projections now participate in grant selection, and the supplied outage regression does not observe rejection at final provider I/O.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Label justifications:

  • P2: Reduces inference admission overhead without establishing an urgent user-facing outage.
  • merge-risk: 🚨 security-boundary: Persisted routing projections now participate in grant selection, and the supplied outage regression does not observe rejection at final provider I/O.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add final-effect proof for the nearest unauthorized caller and prove owner revocation during failed authority publication blocks provider I/O.
  • Record an upgrade from v0.4.0 on retained storage with existing settings and credentials intact.

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 12, 2026, 12:58 AM ET / 04:58 UTC.

@steipete
steipete merged commit 3e31abb into main Oct 12, 2026
15 checks passed
@steipete
steipete deleted the perf/single-hop-inference branch October 12, 2026 05:12
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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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