Skip to content

perf(sse): scan SSE data payloads in place and share the empty byte view - #4979

Merged
lidge-jun merged 5 commits into
devfrom
codex/carry-4731-sse-inplace-scan
Sep 18, 2026
Merged

lidge-jun merged 5 commits into
devfrom
codex/carry-4731-sse-inplace-scan

Conversation

@lidge-jun

Copy link
Copy Markdown
Owner

Carries #4731 by @chilung-cgu, reconciled against current dev. Original commit authorship is preserved.

Summary

sseDataPayload split every complete SSE block with a regular expression and built an intermediate array of lines before joining them, on every block of every active stream. It now walks the block once with indexOf, slicing only the values it keeps. src/server/relay.ts and src/server/sse-frame-buffer.ts also allocated a fresh new Uint8Array(0) at roughly twenty sites that all mean "nothing here"; those share one immutable zero-length view.

Event-stream semantics are unchanged: colonless data fields, the one optional ASCII space after the colon, multiline joining, UTF-8 text including astral characters, LF and CRLF input, and near-prefix fields such as database: that must not match. Non-empty frame ownership, frame limits, cancellation, terminal detection and wire bytes are untouched.

The conflict this carry reconciles. The branch was cut before two changes landed in relay.ts: bounded non-stream response logging, and the hard-cancel guard that stops a partial terminal from being flushed. A textual rebase applies cleanly here — which is the problem, because "applies cleanly" and "preserves intent" are different questions, and green CI on the old base could not tell them apart. The carry was rebuilt against the current file rather than replayed onto it, and the resulting diff against dev touches only the new Uint8Array(0) sites and the extractor export. Neither landed contract is reverted or reworded.

One deliberate behavior change, carried from the original. relay.ts had its own copy of sseDataPayload that ignored a colonless data line; the canonical extractor in sse-payload-rewrite.ts treats it as an empty value, which is what the event-stream specification says. relay.ts now re-exports the canonical one rather than keeping a second implementation, so a bare data line contributes an empty string on that path too. The two copies disagreeing was the real defect; the specification decides which one wins. A trailing lone CR is covered by the same rule and is pinned as a test.

On the structure docs. The contract is written once, in the byte-accounting.md section that owns stream-buffer accounting. The other docs that link that section already carry a cross-reference line to it, and restating the contract in each of them is the two-statements-that-drift case structure/AGENTS.md asks to avoid.

Verification

Local verification was not run: this lane forbids running any local suite, typecheck, build, or install. Hosted CI on this PR head is the executable verification.

Static checks performed in place of local execution:

  • relay.ts was diffed between the original branch's merge base and current dev to enumerate what landed in between, then the final origin/dev..HEAD diff was read line by line to confirm that none of it is removed, reordered, or rephrased.
  • The new scanner was traced against every branch of the old implementation for the cases the tests now assert, including the trailing-lone-CR and colonless-data boundaries where the two differ.
  • The shared zero-length view is only ever read or replaced, never written through, so sharing it cannot alias state between streams; a zero-length view has nothing to write.
  • The new byte-accounting.md paragraph names only paths this tree has, which is what the structure gate checks.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Co-authored-by: chilung b0423031@gmail.com

chilung-cgu and others added 5 commits September 18, 2026 08:19
… array allocations

- Replace RegExp split with in-place character index scanner (indexOf/charCodeAt).
- Eliminate intermediate string array allocation and regex overhead in hot SSE payload parsing.
- Unify relay.ts and sse-payload-rewrite.ts onto single canonical implementation.
- Maintain identical RFC event-stream semantics for colonless data fields and multi-line data.

(cherry picked from commit b890c92)
…nd expand boundary tests

(cherry picked from commit 6349418)
Co-authored-by: chilung <b0423031@gmail.com>
(cherry picked from commit 1527754)
The in-place scanner, the canonical extractor relay.ts now re-exports,
and the shared zero-length view are a stream-buffer accounting contract,
so they are written once in the doc that owns that section.

The docs that merely link that section already do so from their own
cross-reference line; restating the same contract in each of them is the
two-statements-that-drift case structure/AGENTS.md asks to avoid.

Co-authored-by: chilung <b0423031@gmail.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 17, 2026 23:32
@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: b83e4d5c-624f-4160-9b73-7bccce7139e3

📥 Commits

Reviewing files that changed from the base of the PR and between 61ee647 and 36ecb5a.

📒 Files selected for processing (6)
  • src/server/relay.ts
  • src/server/sse-frame-buffer.ts
  • src/server/sse-payload-rewrite.ts
  • structure/transports/byte-accounting.md
  • tests/responses/sse-client-frame-bounds.test.ts
  • tests/responses/sse-payload-rewrite.test.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-17T23:36:44.948596Z 36ecb5a PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 77 / 80

이 PR(#4979)은 기여자 chilung-cgu의 #4731을 현재 dev 위에 다시 올린 캐리(carry) 입니다. 작성자는 lidge-jun, 브랜치는 codex/carry-4731-sse-inplace-scan이고, 원본 커밋 저자(chilung) 정보는 cherry-pick으로 유지합니다. 바꾸는 파일은 src/server/sse-payload-rewrite.ts(추출기), src/server/sse-frame-buffer.ts(EMPTY_BYTES), src/server/relay.ts(공유 빈 바이트 + 캐노니컬 재수출), 테스트 두 개(tests/responses/sse-payload-rewrite.test.ts, tests/responses/sse-client-frame-bounds.test.ts), 계약 문서 structure/transports/byte-accounting.md입니다. types.ts/config.ts 분할 캠페인과 무관하므로 close-don't-rebase 대상이 아닙니다.

지금 dev HEAD는 61ee64747 (패키지 2.59.0, tip #4948 cold status setup)입니다. 그 HEAD에서 sseDataPayload는 두 곳에 있습니다. sse-payload-rewrite.ts 쪽은 콜론 없는 data 줄을 빈 문자열로 취급하고, relay.ts 406–414줄 쪽은 data:로 시작하는 줄만 모읍니다. 같은 이름인데 규약이 달라서, 릴레이 경로와 리라이트 경로가 같은 SSE 블록을 다르게 읽을 수 있습니다. 이번 캐리는 릴레이 쪽 복제를 지우고 sse-payload-rewrite의 구현을 재수출합니다. 그래서 의도적으로 한 가지 행동이 바뀝니다. 릴레이가 보던 맨 data 줄도 이제 빈 값을 기여합니다. 이벤트 스트림 규약이 그쪽을 고릅니다.

성능 쪽은 핫 경로입니다. 예전 구현은 완성된 SSE 블록마다 split(/\r?\n/)으로 줄 배열을 만들고, data:만 골라 다시 join했습니다. 활성 스트림마다 블록마다 배열·정규식 비용이 납니다. 새 스캐너는 indexOf와 charCodeAt으로 블록을 한 번만 걷고, 남길 값만 slice합니다. 콜론 뒤 선택 ASCII 공백 하나, 여러 data 줄의 \n 접합, UTF-8/이모지, LF·CRLF, database: 같은 비슷한 접두어 거절은 그대로입니다. 테스트가 콜론 없는 data, 꼬리 단독 CR, 탭 보존, 연속 빈 data까지 잠급니다.

두 번째 축은 빈 바이트입니다. relay.ts와 sse-frame-buffer.ts는 “여기 아무것도 없음”을 뜻하는 new Uint8Array(0)을 스무 곳 가까이에서 새로 만들었습니다. 캐리는 EMPTY_BYTES 하나를 공유합니다. 길이 0이라 쓰기를 할 내용이 없고, 반환·대입만 하므로 스트림끼리 상태가 섞이지 않습니다. 비어 있지 않은 프레임 소유권, 프레임 한도, 취소, 터미널 판별, 와이어 바이트는 손대지 않습니다. 본문이 말한 충돌도 중요합니다. 원본 브랜치는 바운디드 non-stream 응답 로그와 hard-cancel(부분 터미널 flush 차단)이 relay.ts에 들어오기 전에 잘렸습니다. 글자 그대로 rebase하면 깨끗해 보이지만 의도 보존과 다릅니다. 캐리는 현재 파일을 기준으로 다시 쌓아서, dev 대비 diff가 빈 바이트 자리와 추출기 수출만 건드리게 맞췄습니다.

문서는 byte-accounting.md 한곳에만 적었습니다. 스트림 버퍼 회계를 소유한 절이라서, 링크만 하는 다른 문서에 같은 계약을 다시 쓰면 나중에 어긋납니다. structure/AGENTS.md가 피하라고 하는 두-문장-표류 패턴입니다. 로컬 스위트는 이 레인 규칙상 안 돌렸고, 호스티드 CI가 검증입니다. 리뷰 시점에는 react-doctor·CodeRabbit만 초록이고 changes/hygiene/label/resolve-pr/Windows runner는 pending입니다. base SHA는 61ee64747와 같아 tip-rebase는 당장 필요 없어 보입니다. mergeable은 MERGEABLE이지만 체크 대기로 blocked 상태입니다. 원본 #4731은 아직 open입니다.

라인 src/server/relay.ts 406-414(현재 dev) - 여기 로컬 sseDataPayload는 data:만 본다. 캐리는 이 함수를 지우고 sse-payload-rewrite를 재수출한다. relay-eager.ts가 relay에서 다시 내보내므로, 그 경로의 콜론 없는 data 의미가 스펙 쪽으로 맞춰진다. 의도된 수정이나, “릴레이만의 옛 무시”에 의존하던 호출이 있다면 빈 문자열이 생긴다.
라인 src/server/sse-payload-rewrite.ts sseDataPayload - indexOf/슬라이스 한 바퀴로 바꿨다. 여러 data 줄이면 result += "\n" + value로 이어 붙여 중간 문자열은 남는다. 정규식·줄 배열보다는 가볍지만, 초대형 멀티라인 페이로드에서는 접합 할당이 남을 수 있다. 핫 경로 최적화의 합리적 타협이다.
라인 src/server/sse-frame-buffer.ts EMPTY_BYTES - 공유 길이 0 뷰다. 쓰기를 하면 모든 소비자가 같은 버퍼를 보게 되지만, 길이 0이라 쓸 칸이 없고 코드는 대입·반환만 한다. 테스트가 joinSseFrameBytes([])와 finish()가 같은 참조를 쓰는지 toBe로 잠근다.
경로/심볼 tests/responses/sse-payload-rewrite.test.ts - 콜론 없는 data, 꼬리 단독 CR, 탭, UTF-8, database:/date: 거절, 연속 빈 줄까지 넓게 잠근다. 릴레이 재수출 경로를 따로 import해 돌리는 테스트는 없다(재수출이라 동일 함수).
경로/심볼 #4731 원본 - 기여자 PR은 여전히 open이다. 이 캐리가 머지되면 Landed via #4979 at <commit> + landed-via-maintainer로 닫아야 open PR 수가 부풀지 않는다.

메인테이너의 판단이 필요한 지점

  • 릴레이 경로의 콜론 없는 data → 빈 문자열 정렬을 스펙 수정으로 확정할지(본문·테스트는 이미 그쪽)
  • #4731을 머지 직후 landed-via로 닫을지, 기여자 chilung-cgu에게 한 줄 감사를 남길지
  • CI exact-head 초록 전에 다른 tip이 relay.ts를 건드리면 빈 바이트 자리만 다시 맞출지
  • EMPTY_BYTES를 다른 서버 모듈(비-SSE)까지 넓힐지, 이번처럼 SSE 프레임/릴레이에만 둘지

너의 추천
호스티드 CI가 exact-head로 초록이면 머지한다. 머지 직후 #4731에 Landed via #4979 at <commit>를 남기고 landed-via-maintainer 라벨을 붙인 뒤 completed/superseded로 닫는다. in-place 스캔·캐노니컬 추출기 통일·공유 EMPTY_BYTES·경계 테스트·byte-accounting.md 한곳 기록이 원본 의도(할당 줄이기 + 두 구현 불일치 제거)를 현재 dev 계약(바운디드 로그·hard-cancel)을 깨지 않고 올린 캐리다. types/config 분할과 무관하다.

이 댓글은 grok-bot이 작성했습니다

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36ecb5a490

ℹ️ 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".

failed enqueue and cancellation release the reservation without re-entering a disposed rewrite.
Old/new buffer overlap remains charged against the same translator cap.

Complete SSE blocks extract `data` fields with one indexed pass over the block rather than a

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Update every document mapped to src/server

This change modifies the shared src/server/ area but updates only byte-accounting.md, while structure/manifest.json also maps that area to documents such as runtime.md, transports/responses.md, and data-planes/images.md. Update every mapped document in this change—preferably with a reference to this canonical contract rather than duplicated prose—because the scoped repository rule explicitly requires all listed documents to remain synchronized.

AGENTS.md reference: structure/AGENTS.md:L49-L50

Useful? React with 👍 / 👎.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the enhancement New feature or request label Sep 17, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Merging with macOS legs outstanding, and recording why rather than leaving it implicit.

At this exact head the full Linux suite (test 1/4 through 4/4), gates, storage policy, enforce-target, the docs build, and the keyring and npm-global smokes are green. The macOS legs are queued behind a saturated hosted-runner pool shared by several concurrent lanes, and the sharded macOS legs are separately known to go silent mid-suite and be cancelled at their job budget — a long-standing defect recorded with six occurrences in #4956, including two from the 2.58.0 round that were previously written off as capacity.

This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform lane=all dispatch at the frozen release candidate, which is held until #4956 has a named cause. Nothing is promoted on the strength of this merge.

Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here.

@lidge-jun
lidge-jun merged commit 6467235 into dev Sep 18, 2026
26 of 27 checks passed
@lidge-jun
lidge-jun deleted the codex/carry-4731-sse-inplace-scan branch September 18, 2026 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants