Repository navigation
Cover HEAD preservation, interim answers and the SOCKS response timeout - #5225
Conversation
…sponse timeout Three coverage gaps the acceptance audit found in the transports work. No production change: each case exercises a branch that is already there and was not asserted. HEAD (#5109). A HEAD answer carries the headers the GET would have carried, including the length of a body it will never send. The SOCKS transport now has a case proving it answers with a null body, keeps the advertised length in the headers where a caller is entitled to it, and releases a tunnel whose peer asked to stay alive. Interim answers (#5110). A peer may answer 100 and 103 before the request body is finished, and those are not the answer. The new case has the peer send both mid-upload and asserts the caller receives the final 200, with upload continuity witnessed by the peer receiving the rest of the body including its terminating chunk. The response timeout (#5110). The branch that matters runs while the upload is parked on a chunk the caller will never produce. Waiting out the real budget would make this a three-minute test, so the timer is observed as the transport arms it and its own callback is invoked once the peer has witnessed the stall. The budget asserted is the transport's, mirrored in the test rather than exported from the module. Settlement, reader release and socket teardown are each witnessed. local checks: NOT RUN
|
✅ 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. |
github-actions
Bot
added
the
chore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds SOCKS5 transport tests for interim upload responses, stalled response-timeout cleanup, and HEAD responses with null bodies and unsent ChangesTransport lifecycle coverage
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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: f43cc0f6e9
ℹ️ 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".
리뷰 · 우선순위 36 / 80이 PR은 돌아가는 코드를 바꾸지 않습니다. SOCKS로 요청을 보낼 때 빠졌던 확인 세 가지를, 테스트만 넣어서 채웁니다. HEAD부터입니다. 상대는 "본문이 128바이트"라고 길이를 적어 줍니다. HEAD 답에는 그 본문이 없습니다. 적힌 숫자만큼 읽으면, 오지 않을 본문을 기다리다 멈춥니다. 새 테스트는 본문이 비어 있는지, 128이 헤더에 남는지, 상대가 연결을 살려 두자고 해도 터널을 닫는지 봅니다. 업로드가 끝나기 전에도 상대는 100과 103을 보낼 수 있습니다. 둘 다 최종 답이 아닙니다. 테스트는 마지막 상태가 200이고 본문이 마지막은 올리는 쪽이 멈춘 경우입니다. 다음 조각을 영원히 안 주면 답도 오지 않습니다. 제한 시간은 200초라, 테스트는 그 시간을 기다리지 않습니다. 타이머가 200초로 켜지는지만 보고, SOCKS 코드가 타이머에 넣어 둔 함수를 직접 실행합니다. 에러로 끝나는지, 멈추던 스트림이 풀리는지, 소켓이 닫히는지를 각각 봅니다. 베이스는 라인 - tests/lib/socks5-upload-lifecycle.test.ts:314 - 100과 103 테스트가 본문을 메인테이너의 판단이 필요한 지점 너의 추천 이 댓글은 grok-bot이 작성했습니다 |
The case enqueued the whole request body up front, so the upload could finish before the peer ever sent 100 and 103 and the claim that the rest of the body went out after them was not causal. The peer now answers the request head before any body byte, and the caller's stream withholds its last chunk and the end of the body until that has happened. Upload continuity is measured against the moment the interim answers were sent rather than against the whole exchange, so the terminator arriving afterwards says what it appears to say. The transport-side receipt is the final answer itself: reading 200 with its body is only possible for something that consumed both interim heads and went on reading, where a transport that stopped at the first would have handed the caller an empty 100. The exchange is also required to end on its own, before the fixture tears any peer down. No production change, and every existing assertion is kept. local checks: NOT RUN
…f the tunnel The previous version released the rest of the body once the peer had written 100 and 103, which says the bytes were sent, not that the transport had them. The release now waits until both complete interim heads have arrived on the client side of the tunnel and the reader has had a native scheduling turn to consume them, and it records that the fetch is still pending and the body uncancelled at that moment, which is what says neither head was taken for the answer. The socket is observed through the timer the transport already arms, with a passive data listener; the reader is in flowing mode, so it observes the same chunks and consumes none. No production seam and no sleep. Upload continuity is still measured from the moment the interim answers were sent, the final answer and its body are still the receipt that both were read past, and the exchange must still end on its own before the fixture tears any peer down. local checks: NOT RUN
추가 리뷰 · 우선순위 30 / 80지난 리뷰에서 100과 103 테스트가 순서를 못 잡는다고 적었습니다. 그 뒤 커밋 둘이 그 구멍을 막았습니다. 돌아가는 코드는 그대로입니다. 먼저 나가는 본문은 100을 최종 답으로 보고 업로드를 끊으면 여기서 실패합니다. HEAD 확인과 제한 시간 확인은 이번 푸시에서 안 바뀌었습니다. 라인 - 새로 고칠 줄은 없습니다. 메인테이너의 판단이 필요한 지점 너의 추천 이 댓글은 grok-bot이 작성했습니다 |
The held tail lived in a stream pull that awaited a receipt which, if it never arrived, would park the fetch until the runner's own deadline. The finally block restoring the global Socket prototype would not have run by then, and the data observer was anonymous, so nothing could remove it either. The tail is now held by the test body with bounded waits on the receipt, the outcome and the response, the observer is named and removed, and the fetch is owned by an AbortController the fixture aborts on the way out. The prototype is restored first, before anything else in teardown. Every causal assertion is kept and two more are added: at the moment the remainder is released, the peer has not yet seen it or the terminator, which makes the ordering the assertions rely on visible rather than assumed. The timeout constant and the response and closure checks are unchanged. local checks: NOT RUN
|
Maintainer integration into dev under MAINTAINERS.md at reviewed head This focused test-only follow-up completes original #5109/#5110 verification requirements. Independent review accepted real HEAD/null-body settlement and cleanup, controlled100/103 processing before the remaining upload, the real response-timeout callback while a read is stalled, and bounded failure cleanup. No production behavior, timeout constant or existing assertion was weakened. Exact-head hosted run35477114410 attempt1 completed successfully with all applicable jobs and aggregate. Actual macOS logs105988178081/105988178063 show all three named cases passing. Both jobs checked out a81daf8. Change-inapplicable jobs and unrequested Windows full shards are not counted as executions. No local tests, typecheck, builds or runtime were run. The public finding is resolved. Current merge candidate is tree088e56b6379cd5dbf6b28fd5db8f682bd362a468, native membership is absent, and maintainer preflight passed. Re-closure follows actual dev ancestry verification; previous implementation receipts remain preserved. |
1 participant
Summary
Three coverage gaps the acceptance audit found in the raw outbound transports work for #5109 and #5110. No production change: each case exercises a branch that is already there and was not asserted.
content-lengthsurviving in the headers where a caller is entitled to it, and the tunnel released even though the peer asked to keep it alive.100and103before the request body is finished, and those are not the answer. The peer sends both mid-upload and the caller receives the final200, with upload continuity witnessed by the peer receiving the rest of the body including its terminating chunk.tests/lib/transport-null-body.test.tstests/lib/socks5-upload-lifecycle.test.tsThe timeout budget is mirrored in the test rather than exported from
src/lib/socks5-fetch.ts. The value is what identifies the transport's own timer among the several this exchange arms, and a test is not a reason to widen that module's surface; if the budget changes there, the assertion stops finding it and says so.Verification
Hosted CI on this branch at exact head. local checks: NOT RUN — no repository suite, focused test, typecheck, build, install or runtime command was run in this lane.
These are test-only additions against branches of the transports that already exist, so the evidence that matters is that they pass without any production change.
Checklist
Summary by CodeRabbit
100and103responses.HEADresponses remain bodyless when an unavailable response length is advertised.