Repository navigation
docs(devlog): assess what the dev tip closes of the pooled-routing cost incident (#4546) - #4969
Conversation
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
리뷰 · 우선순위 76 / 80이 PR은 코드나 설정을 바꾸지 않습니다. 다시 열린 이슈 #4546이 지금 지금 토큰을 태우는 쪽은 아직 안 닫혔고, 이유는 라우팅이 아닙니다. 문서가 결함이 아니라고 적은 세 가지도 현재 트리와 맞습니다. 코호트가 팬아웃을 인터랙티브 계정에 모으는 것, 라인 27–36 근처 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629a9ee47d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `x-codex-parent-thread-id` has no root, and `admitWorkflowTurn` returns early on | ||
| `if (!rootId) return undefined` (`src/lib/workflow-budget.ts:425`), so unparented | ||
| one-off traffic sits outside every ceiling. And the lane is decided purely by header shape | ||
| at `src/server/index.ts:505`, so a client that omits `thread-id` on its children has | ||
| them admitted as interactive. |
There was a problem hiding this comment.
Move the live budget-bypass analysis to scratch
When this assessment is published, these lines provide concrete bypasses for still-unfixed cost protections: omitting the parent header skips every workflow ceiling, while omitting thread-id gives child traffic interactive treatment. This exposes operator quota/cost and is precisely unfixed bypass reasoning, so keep it in .tmp/ until the fixes ship rather than committing it to the public devlog/.
AGENTS.md reference: AGENTS.md:L124-L131
Useful? React with 👍 / 👎.
| | `maxPhysicalSends` | 256 / 10 min | ~61 / 10 min | no | | ||
| | `maxDistinctChildren` | 64 / 10 min | ~61 / 10 min | no, by three | | ||
| | `maxConcurrentChildren` | 8 (7 for workers) | throttles shape, not volume | n/a | | ||
| | token ceiling | unset and unsettable | ~8.7M uncached input / 10 min | **cannot** | |
There was a problem hiding this comment.
Use windowed data before declaring the count caps inactive
Dividing 732 calls across the full two-hour interval yields only an average; it cannot establish that no ten-minute window exceeded either cap. At the assessed SHA, src/lib/workflow-budget.ts:65-75 explicitly describes this incident as a 700-send one-minute burst and says the ten-minute limits were chosen to refuse it, in which case the 64-child and 256-send caps would fire. Use timestamped per-root window data or qualify the conclusion that the measured incident passes every live ceiling.
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
Summary
#4546 has been reopened once already for being closed against partial work, and the record
of what it still covers is now spread across five issue comments and two devlog docs written
before cohort keying landed. This adds the closure assessment against the current
devtip sothe next person to look at the issue does not have to re-derive it.
The finding, in short: the cache-destruction half of #4546 is closed, and the token-burn half
is not — but not for the reason the issue says. Routing can no longer move a live conversation
for a quota preference (
mayRebindAffinityForQuotadefaults to a genuine-exhaustion bar), there-score interval is no longer short-circuited in the 80-99% band, preview agrees with resolve,
and cohort keying puts a whole conversation tree on one account. What remains is the reporter's
Phase 1: a fan-out of one-off requests whose prefixes are cold because each is a distinct
prompt, which no account-affinity rule can warm.
The doc names the remaining mechanism precisely.
DEFAULT_SPEND_RESERVATION_POLICYleaves everytoken ceiling undefined by design, and
sharedSpendLedger()constructs the process-wide ledgerwith no
policyargument at all. There is noconfigureSharedSpendLedgerand no spend key insrc/types/config.ts, so aSpendReservationPolicycarrying a realmaxTokensis built inexactly two places in the tree, both tests. Token enforcement cannot be switched on by any
configuration an operator can write, so the ledger measures the volume and never refuses it.
What is enforced is counts, and the doc checks those against the reporter's own figures: at
roughly 61 sends and 61 distinct children per ten-minute window, the measured incident passes
the 256-send and 64-child caps and is bounded by nothing on tokens.
It also records three things that are true and easy to mistake for defects: cohort keying now
deliberately concentrates a fan-out onto the interactive account (correct for cache locality,
and
interactiveReservereserves a concurrency slot rather than quota), the wpc domainclassifier in
src/routing/identity-domains.tsis landed with config validation as its onlyproduction consumer, and the synthetic
cached_tokens: 0on the bridged wire is a requiredcompatibility behaviour rather than dishonest accounting.
Docs only. No source, test, or configuration file changes.
Verification
Local verification was not run — no suite, focused test, typecheck, build, install, or
ocxinvocation — because this lane forbids it. Hosted CI at the exact head of this branch is the
executable verification.
Every claim in the doc is a static source citation at
dev@61ee64747b, and each one names thefile and line it rests on so a reviewer can check it by reading rather than by running anything.
The two load-bearing negative claims were established by exhaustive search rather than by
inspection of a single site: that no production caller passes a
policytocreateSpendReservationLedger, and that the only production importer ofsrc/routing/identity-domains.tsissrc/config/schema/leaf-validators.ts.Checklist