[WRONG BRANCH] fix(responses): bound combo recall model retention - #4755
Conversation
|
✅ Deterministic PR hygiene checks passed. |
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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
리뷰 · 우선순위 76 / 80이 PR은 이슈 #4525를 현재 고치는 축은 세 가지입니다. (1) 모델 id 하나당 1 KiB, 저장소 전체 64 KiB. 크기 검사는 UTF-8로 인코딩하기 전에 코드 유닛으로 먼저 보고, UTF-8이 코드 유닛보다 작아질 수 없으니 한도를 지키려고 할당을 먼저 쓰지 않습니다. (2) 집계가 넘치면 Map 맨 앞(가장 오래 안 쓰인 쓰기)부터 지웁니다. 쓰기마다 같은 레인을 뒤로 다시 넣기 때문에 앞쪽이 곧 LRU이고, 한 항목이 집계보다 훨씬 작아서 자기 자신을 쫓아내지는 않습니다. (3) 삭제는 모두 정렬(ordering) 선택이 중요합니다. 원본 #4525는 너무 긴 모델 id면 레인을 파일은 경로 base 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 9a50b858e3
ℹ️ 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".
| { name: "combo-session-recall", reconcileGeneration: reconcileComboRecall }, | ||
| { | ||
| name: "combo-session-recall", | ||
| sweepExpired: sweepExpiredComboRecall, |
There was a problem hiding this comment.
The source-to-doc map in structure/INDEX.md shows that both changed areas, src/lib/ and src/server/, are described by multiple structure documents, but this commit updates only structure/transports/responses.md. Add the bounded-retention and periodic-sweep contract to every document mapped to those areas, or correct overly broad manifest ownership, so the repository’s architecture references do not remain incomplete.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
9a50b85 to
ce4ad8d
Compare
The remembered model id is provider-reported and arrives on the response, so nothing upstream of the recall store bounds its length. Lane keys are already SHA-256 digests, which means the 256-lane cap bounded the number of entries but not the bytes those entries held. A long-running process could accumulate arbitrarily large remembered strings. Bound retention on two more axes: 1 KiB per remembered model id and 64 KiB in aggregate. The size test runs on code units before encoding, because a UTF-8 encoding is never smaller than its code-unit count, so the bound never pays the allocation it exists to prevent. Aggregate eviction drops the least recently written lane, which is the front of the map because every write re-inserts its own lane at the back. A single entry is capped far below the aggregate budget, so a write can never evict itself. Every removal now goes through one helper that releases the entry's bytes, so the counter cannot drift from the map through the read-time invalidation path, the reconciliation path, or a lane rewrite. An unretainable model id DECLINES the write rather than clearing the lane. That is the ordering-sensitive part. This callback carries a config generation, not a request order, so two accepted completions on one lane under the same generation can arrive out of order; a clearing branch would let the older one erase the newer selection. Declining matches how every other rejection in rememberComboForLane already returns, and leaves the established contract intact: an older response never overwrites or clears a newer one. Register the store for periodic expiry as well. The TTL was previously evaluated only on read or on a generation change, so a lane that is never read again held its entry until the process exited. Closes #4525 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
ce4ad8d to
246d703
Compare
|
Landing the combo fallback lane, top layer first. Evidence at the exact head 246d703 (tree
Chained-child stacks merge top-down, so this lands in the parent branch and cascades to Maintainer integration decision under MAINTAINERS.md / AGENTS.md, recorded with the exact-head evidence above. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
1 participant
Summary
Carries #4525 by @luvs01 onto current
dev, stacked on #4750. Base iscodex/cf2-output-headroom; retarget todevonce the parents land.The remembered model id is provider-reported and arrives on the response, so nothing upstream of the recall store bounds its length. Lane keys are already SHA-256 digests, which means the existing 256-lane cap bounded the number of entries but not the bytes those entries held, and a long-running process could accumulate arbitrarily large remembered strings.
src/server/responses/combo-session-recall.tsnow bounds retention on two further axes: 1 KiB per remembered model id and 64 KiB in aggregate. The size test runs on code units before encoding, because a UTF-8 encoding is never smaller than its code-unit count, so the bound never pays the allocation it exists to prevent. Aggregate eviction drops the least recently written lane — the front of the map, because every write re-inserts its own lane at the back — and a single entry is capped far below the aggregate budget, so a write can never evict itself. Every removal path now goes through one helper that releases the entry's bytes, so the counter cannot drift from the map through read-time invalidation, reconciliation, or a lane rewrite.The store is also registered for periodic expiry. The 30-minute TTL was previously evaluated only on read or on a generation change, so a lane that is never read again held its entry until the process exited.
The ordering change, and why it went the other way
The original patch cleared the lane when a model id was too large to retain. That is unsafe here. The publication callback carries a config generation, not a request order, so two accepted completions on the same lane under the same generation can arrive out of order — and a clearing branch lets the older one erase the newer selection. There is no monotonic request token in this path to compare against, and introducing one would mean threading ordering state through
core-combo.tsbefore async dispatch, which is well outside a retention-bounding change.So an unretainable model id declines the write instead. That matches how every other rejection in
rememberComboForLanealready returns (stale generation, unknown owner, blank model), and it preserves the established contract exactly: an older response never overwrites or clears a newer one. The docs were written to that behavior rather than to the stronger claim.The 16 mechanical cross-link sentences the original patch appended to unrelated
structure/documents are dropped.structure:checkdoes not read the git diff and cannot require them; onlystructure/transports/responses.md, which owns this contract, is updated.Verification
No local suite, no focused test file, no typecheck, no build and no dependency install was run — this lane is under an explicit owner instruction forbidding local execution. Evidence is static source reading plus hosted CI.
Static checks performed:
sessionLaneIdFromRequestcomposesnormalizeLogConversationIdvalues, which are SHA-256 digests truncated toLOG_CONVERSATION_ID_LEN. The model id really is the only field of unbounded length, so byte accounting is scoped to it.rememberComboForLane.global fake-clock sweep invokes every production clock registrationderives its expectation fromSTATE_STORE_REGISTRATIONSitself, so addingsweepExpiredis self-adjusting;a sweeper tick expires continuation and Antigravity rowsregisters only those two stores, so itsrowsRemoved: 2is unaffected.tests/responses/responses-compaction-routing.test.ts(TTL boundary at 30 minutes, 257-write lane eviction keeping lanes 1 and 256, selector precedence, invalidation on delete/rename/disable). All use short model ids well inside the 1 KiB bound and lane counts inside the 64 KiB budget, so none changes.combo-session-recall.ts89 → 149 lines, no baseline entry, global threshold 2,000.structure/transports/responses.mdis already ingrace.oversizeDocs, which is a boolean allowlist with no recorded size, so the addition does not trip the budget check.Hosted CI: this is the tip of the
cf1 → cf2 → cf3stack, so its run gates all three layers. Run URL and conclusion are recorded in a comment on this PR.Hosted CI evidence
Head
246d703aec275b3c7267ebde9a004334614b905e. Run: https://github.com/lidge-jun/opencodex/actions/runs/35049570323The four
testshards are reported explicitly rather than relying on the rollup, because a stacked tip can have its heavy jobs filtered out by thechangespath filter and still aggregate green, and because the rollup does not reflect shard failures while the run is in progress.Also success on this head:
changes,gates,storage policy,api usage,hygiene,macos 1/2,macos 2/2,keyring ubuntu|windows|macos,docker smoke,npm-global ubuntu|windows|macos,react-doctor,label,resolve-pr,select windows runner. The Windows shard matrix and the macOS control job are skipped by the workflow's own selection, not failed.Checklist