feat(lib): reserve tokens and output before dispatch, and keep the ledger across restart (#4546) - #4625
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request adds a durable, salted spend-reservation ledger with journal replay, retention, corruption handling, and token limits. Workflow admission integrates reservations with existing count caps and dispatch outcomes. New tests cover ledger, workflow, filesystem, privacy, and layout behavior. ChangesSpend and workflow controls
Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WorkflowClient
participant WorkflowBudget
participant SpendReservationLedger
participant SpendJournal
WorkflowClient->>WorkflowBudget: submit workflow turn and spend request
WorkflowBudget->>SpendReservationLedger: reserve input plus output ceiling
SpendReservationLedger->>SpendJournal: persist reservation
WorkflowBudget-->>WorkflowClient: return workflow admission
WorkflowClient->>WorkflowBudget: mark dispatched or release
WorkflowBudget->>SpendReservationLedger: settle, abandon, or mark lost
SpendReservationLedger->>SpendJournal: persist final state
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef3eb736cf
ℹ️ 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".
| // fresh under the same id resets the very ceiling that already fired. | ||
| if (state.active > 0) continue; | ||
| if (state.sends >= policy.maxPhysicalSends) continue; | ||
| if (spendLedger?.exhausted("root", key) === true) continue; |
There was a problem hiding this comment.
When every entry at maxTrackedRoots is idle but exhausted, this skip leaves oldestKey unset, yet admitWorkflowTurn still inserts the new root unconditionally. A caller that repeatedly exhausts fresh root IDs can therefore grow roots beyond the advertised bound indefinitely; retain bounded tombstones or deny/evict safely when no full state is removable.
Useful? React with 👍 / 👎.
| /** | ||
| * Durable token spend reservation, above the send-count workflow guard (#4546). |
There was a problem hiding this comment.
This adds a persistent src/lib subsystem and changes workflow admission architecture, but the commit updates none of the structure documents assigned to src/lib/ in structure/INDEX.md. Add the durable reservation, replay, retention, and failure invariants to the applicable owning documents so the architecture source of truth does not contradict the runtime.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
| if (state.reserved > 0 || state.lastSeenAt >= cutoff) continue; | ||
| if (isExhausted(scope, state)) continue; | ||
| scopes.delete(key); |
There was a problem hiding this comment.
When prune() removes a dormant under-limit scope, the deletion is only in memory; the append-only journal retains every reserve and settlement. After a restart, replay recreates the supposedly expired scope and its spend, so admission depends on whether the process restarted, while the journal and replay maps also grow without respecting retentionMs. Record pruning/expiry or compact the journal consistently with the in-memory deletion.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 70 / 80설명 이 PR(#4625, 새 모듈
점수를 70으로 둔 이유: 재시작 세탁을 막는 계약이 명확하고 기본값이 안전하며 테스트가 핵심을 고정한다. 다만 배선·설정 표면·다중 프로세스·settle이 예약보다 커질 때의 정책은 다음 층/운영 판단이 남는다. 라인 applySettle / src/lib/spend-reservation-ledger.ts - settle 시 실제 usage가 예약 ceiling보다 커도 settled에 그대로 더한다. 의도는 맞지만, 한 번 초과 settle이 나면 다음 reserve가 바로 거절될 수 있다. 호출부가 outputCeiling을 모델 cap으로 채우는지 배선 PR에서 고정해야 한다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head ef3eb736cf94f73a1833fead62fff4a34e474500. The reservation model is the right direction, but three persistence boundaries are not safe enough for a ledger that later layers will use to authorize costly sends.
-
createSpendReservationLedgercasts every parsed JSON line toJournalRecordand immediately reads/applies it. Valid JSON such asnull, or{ "v":1, "kind":"reserve", "sendId":"s", "scopes":null, ... }, can throw during hydration; malformed timestamps/scopes/ids can also poison unbounded state. Add a strict bounded runtime decoder for every record and tests for valid-but-wrong JSON, not only an unparsable torn line. -
reserve()mutates memory before journal append, andappend()swallows persistence failure. It still returnsreserved:true; after a restart that admitted send disappears. ApersistFailurescounter observed later cannot prove that this specific authorization was durable. When any applicable token limit is enforcing, persistence failure must refuse/roll back before dispatch (with a typed outcome). Observe-only accounting may degrade if that choice is documented. -
The journal appends two records per settled send forever, and startup uses unbounded
readFileSync(..., "utf8").prune()only trims memory; it never compacts disk. Add an explicit file/record ceiling and a crash-safe checkpoint/compaction contract that retains open reservations and exhausted scopes, with restart tests at the boundary.
The exact-head hosted matrix is green, but its tests do not cover these counterexamples. The PR also remains stacked on #4624, which currently has requested changes. Please keep the single-process topology statement, observe-only default, and three-scope atomic admission while fixing the persistence contract.
ef3eb73 to
56cd54a
Compare
…dger across restart (#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…rite, and bound retention (#4546) Review findings on the reservation ledger: a reused send id authorised a free dispatch, a failed journal append still admitted the request, replay parsed unvalidated JSON, retention was unbounded, an undispatched reservation booked phantom debt, and raw account identifiers reached disk. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
56cd54a to
1d5d299
Compare
2 participants
Summary
The workflow guard added in #4614 counts sends, so a 1k-token turn and a 150k-token turn cost the same unit, and its ledger lives in a process-memory map, so a restart silently resets an exhausted root. Cleanup made that worse: it protected only roots with an active request, so an exhausted-but-idle root could be evicted and then recreated fresh under the same id.
src/lib/spend-reservation-ledger.tsreplaces counting with reservation. Before dispatch a send reserves its whole input plus its enforceable output ceiling —max_output_tokens, or the model's documented cap when the caller sent none. Never an optimistic estimate, and never shrunk by a cache-hit expectation: a prefix that was supposed to hit and did not is exactly the case the reservation exists to survive. Cache-hit projections remain useful for efficiency reporting and have no effect on the safety number.Admission requires
checked at three scopes at once: root workflow, authenticated identity, and account pool. The last two are what stop a caller from minting fresh root ids to mint fresh budget.
Settlement books real usage and is idempotent per send id, because double settlement is as wrong as none. A send whose usage frame is lost does not release its reservation — it moves to unresolved spend, since the work may well have been billed. Reservations, settlements and losses append to a JSONL journal under the opencodex home directory and replay on startup, so an exhausted scope is still exhausted after a restart. Root eviction now requires a root to be both inactive and not exhausted inside the retention window.
Default policy is unchanged behaviour: count caps only, with token accounting observed but not enforcing. Real token limits are opt-in, so no existing install is newly refused by this upgrade. The guarantee is one proxy process against its own journal; a second process sharing the same pool is stated as out of scope in the module header rather than implied away.
Stacked on #4624. Roadmap and stack order:
devlog/_plan/260914_cost_guard_stabilization/090_remaining_stack.md.Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The verification posture for this unit is hosted CI at the exact final head SHA and nothing else; this push used--no-verify.New coverage:
tests/lib/spend-reservation-ledger.test.tsandtests/lib/workflow-budget.test.tspin refusal at the boundary, all three scopes, idempotent settlement, lost usage becoming unresolved spend rather than a release, and an exhausted root that stays exhausted across a simulated restart. Both are registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.Known open, stated rather than hidden: the Responses call sites do not yet pass a reservation.
admitWorkflowTurntakes it as optional trailing parameters, so callers keep today's count-only behaviour until the wiring layer higher in this stack computes the enforceable output ceiling and callssettleWorkflowSpendwith real usage. A ledger nobody calls protects nothing; that obligation belongs to the later layers of this stack.Checklist
Summary by CodeRabbit
New Features
Bug Fixes