feat(performance): P02 异步转发队列 + e2e 设计 spec#1
Conversation
- ClientConn 增加 bounded SendQueue 与 writeLoop 单写者 - 转发路径改为 Enqueue，避免并发写 WebSocket 与慢连接阻塞读循环 - 队列满时按 drop_tail 丢包并统计 DropPackets / QueueDepth / QueueMaxDepth - ConnStats 暴露 drop_packets / queue_depth - writeLoop 对 nil Conn 防御，Close 幂等 - 补充 5 个单测覆盖 Enqueue / drop / Close / Stats Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
覆盖 3 容器 Linux 拓扑、TUN/tc 模拟、3 个场景（连通性/性能/故障）、 软+可选硬门槛、CI 集成。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces an async per-connection send-queue model for tunnel WebSocket writes: a new ChangesAsync send queue for tunnel connections
Estimated code review effort: 3 (Moderate) | ~30 minutes E2E Docker Test Design Spec
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Forwarder as routePacket/clientReadLoop
participant ClientConn
participant SendQueue
participant WriteLoop as writeLoop goroutine
participant WebSocket
Forwarder->>ClientConn: Enqueue(pkt)
ClientConn->>SendQueue: non-blocking send
alt queue full
ClientConn->>ClientConn: increment DropPackets
else accepted
ClientConn->>ClientConn: update QueueDepth/QueueMaxDepth
end
WriteLoop->>SendQueue: drain packet
WriteLoop->>WebSocket: Write(pkt.Data)
WebSocket-->>WriteLoop: write result
WriteLoop->>ClientConn: RecordTx on success
Note over ClientConn,WriteLoop: Close() signals Done, writeLoop drains and exits
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
🤖 CodeRabbit Review Request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
docs/superpowers/specs/2026-07-08-e2e-docker-design.md (1)
257-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep report/artifact collection running after a failed e2e step.
If
./tests/e2e/run.sh --allexits non-zero, the job will stop before--reportandupload-artifact, so the logs and JSON you need for debugging never get published. Gate those steps withif: always()or split collection from the exit status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-07-08-e2e-docker-design.md` around lines 257 - 268, The e2e workflow in the GitHub Actions `e2e` job stops after `./tests/e2e/run.sh --all` fails, preventing report generation and artifact upload. Update the steps in the workflow so the `run.sh --report` and `actions/upload-artifact@v4` steps still run even when the main e2e command exits non-zero, using `if: always()` or an equivalent split between test execution and result collection. Keep the changes localized to the `e2e` job steps that reference `./tests/e2e/run.sh` and `upload-artifact`.internal/tunnel/router_test.go (1)
206-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise
Closeagainst a running writer.These tests use
NewClientConn(nil, ...), butwriteLoopreturns immediately for a nil connection, so they can pass even ifClosedoes not stop an active writer. Use a real test WebSocket pair for the close/idempotency path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tunnel/router_test.go` around lines 206 - 236, The Close tests are only exercising a ClientConn with a nil connection, so they do not prove that Close stops an active writer. Update TestClientConnCloseStopsWriteLoop and TestClientConnCloseIdempotent to use a real test WebSocket pair so the writeLoop is actually running, then verify Close shuts it down, Enqueue is rejected afterward, and calling Close twice remains safe. Use the ClientConn, Close, Enqueue, and writeLoopDone symbols to locate and adjust the test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-07-08-e2e-docker-design.md`:
- Around line 271-273: The documented CI mode is inconsistent with the workflow
example: the risk table says CI should run in quick mode, but the example still
uses the default all-scenarios path. Update the workflow example to use the
quick flag consistently, and ensure the surrounding CI mode descriptions in the
spec align with the intended behavior of the e2e runner (the run.sh invocation
and the push/merge/release trigger notes should match the same mode names).
- Around line 69-73: The host support statement in this spec is inconsistent
with the earlier macOS TUN e2e out-of-scope note, so update the wording in the
design section to make the supported matrix explicit. Adjust the references
around the compose-based workflow and host environments so it only claims
support for the hosts that are actually in scope, and clearly exclude macOS if
TUN-based e2e is not supported there. Keep the phrasing aligned with the rest of
the document’s terms like Docker bridge, server/client, and the compose file.
- Around line 115-118: The readiness probe currently uses plain HTTPS against
the server while TLS is switched to self-signed via MESH_TEST_TLS=off, so the
check will fail before join succeeds. Update the wait_for_server flow so the
probe either accepts self-signed certs with a trusted CA bundle or uses an
insecure HTTPS option, and make the change in the readiness-check logic that
calls curl against server:443/api/devices. Ensure the same fix is applied to the
duplicated readiness example elsewhere in the spec.
In `@internal/tunnel/router.go`:
- Around line 97-103: `router.go`’s `writeLoop` send path is updating
`QueueDepth` after the packet is enqueued, which can race with an immediate
receive/decrement and briefly undercount depth and max depth. Move the
`QueueDepth` increment to happen before sending on `cc.SendQueue` in the
`writeLoop` path, and adjust the drop path so the depth decrement happens there
instead; keep the `QueueMaxDepth` CAS loop tied to the updated depth value.
- Around line 91-121: ClientConn.Enqueue and ClientConn.Close are racing:
Enqueue can see closed as false and still enqueue after Close has flipped
shutdown and stopped the reader, leaving packets stranded. Coordinate shutdown
and enqueue with the same lock or equivalent serialization so Enqueue cannot
accept into SendQueue once Close begins, and make sure writeLoop termination
happens only after that coordination is in place.
- Around line 132-149: The drain loop inside the case <-cc.Done block is using
the same ctx that is canceled when cc.Done closes, causing all drain writes to
fail immediately with context.Canceled. Create a fresh background context (or
context with timeout) specifically for the drain loop inside the case <-cc.Done
block where packets from cc.SendQueue are being written via cc.Conn.Write, while
keeping the existing ctx for the normal message handling path outside this case.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-07-08-e2e-docker-design.md`:
- Around line 257-268: The e2e workflow in the GitHub Actions `e2e` job stops
after `./tests/e2e/run.sh --all` fails, preventing report generation and
artifact upload. Update the steps in the workflow so the `run.sh --report` and
`actions/upload-artifact@v4` steps still run even when the main e2e command
exits non-zero, using `if: always()` or an equivalent split between test
execution and result collection. Keep the changes localized to the `e2e` job
steps that reference `./tests/e2e/run.sh` and `upload-artifact`.
In `@internal/tunnel/router_test.go`:
- Around line 206-236: The Close tests are only exercising a ClientConn with a
nil connection, so they do not prove that Close stops an active writer. Update
TestClientConnCloseStopsWriteLoop and TestClientConnCloseIdempotent to use a
real test WebSocket pair so the writeLoop is actually running, then verify Close
shuts it down, Enqueue is rejected afterward, and calling Close twice remains
safe. Use the ClientConn, Close, Enqueue, and writeLoopDone symbols to locate
and adjust the test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf5646ec-20e5-416d-932a-11a3bfa889eb
📒 Files selected for processing (5)
docs/superpowers/specs/2026-07-08-e2e-docker-design.mdinternal/tunnel/packet.gointernal/tunnel/router.gointernal/tunnel/router_test.gointernal/tunnel/server.go
| - 全部 `privileged: true` + `NET_ADMIN/RAW`，TUN + tc 才能用。 | ||
| - 通过 Docker bridge 网络通信，模拟"跨主机"。 | ||
| - server / client / iperf3/nuttcp 都跑在容器内，host 不需装额外工具。 | ||
| - host 是 macOS / Linux / CI ubuntu-runner 都能跑同一份 compose。 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the host support matrix.
This section says the same compose file runs on macOS/Linux/CI Ubuntu runners, but Line 30 already says macOS TUN e2e is out of scope. That contradiction makes the required host environment unclear.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-07-08-e2e-docker-design.md` around lines 69 - 73,
The host support statement in this spec is inconsistent with the earlier macOS
TUN e2e out-of-scope note, so update the wording in the design section to make
the supported matrix explicit. Adjust the references around the compose-based
workflow and host environments so it only claims support for the hosts that are
actually in scope, and clearly exclude macOS if TUN-based e2e is not supported
there. Keep the phrasing aligned with the rest of the document’s terms like
Docker bridge, server/client, and the compose file.
| - 引入 `MESH_TEST_TLS=off` 开关（推荐在 `internal/config` 落地）： | ||
| - server 跳过 `acme/autocert`，改用自签证书。 | ||
| - client `mesh join` 端允许自签（`InsecureSkipVerify`，已在 `internal/client/peers.go` 使用过）。 | ||
| - 退出测试时无需清理证书目录，容器销毁即可。 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the readiness probe compatible with self-signed TLS.
The flow switches to MESH_TEST_TLS=off, but the example readiness check is plain curl https://server:443/api/devices. Unless wait_for_server adds -k or mounts a trusted CA bundle, this probe will fail before the client can join.
Also applies to: 153-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-07-08-e2e-docker-design.md` around lines 115 -
118, The readiness probe currently uses plain HTTPS against the server while TLS
is switched to self-signed via MESH_TEST_TLS=off, so the check will fail before
join succeeds. Update the wait_for_server flow so the probe either accepts
self-signed certs with a trusted CA bundle or uses an insecure HTTPS option, and
make the change in the readiness-check logic that calls curl against
server:443/api/devices. Ensure the same fix is applied to the duplicated
readiness example elsewhere in the spec.
| - push 触发软门槛（默认）。 | ||
| - merge to master 触发硬门槛（`--strict`）。 | ||
| - release tag 触发全量 + 严格。 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Align the documented CI mode with the workflow example.
The risk table says CI should use --quick, but the workflow example still runs ./tests/e2e/run.sh --all. As written, the long-running scenarios remain on the default path and the resource-saving mode is never used.
Also applies to: 305-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-07-08-e2e-docker-design.md` around lines 271 -
273, The documented CI mode is inconsistent with the workflow example: the risk
table says CI should run in quick mode, but the example still uses the default
all-scenarios path. Update the workflow example to use the quick flag
consistently, and ensure the surrounding CI mode descriptions in the spec align
with the intended behavior of the e2e runner (the run.sh invocation and the
push/merge/release trigger notes should match the same mode names).
router.go: - Enqueue 先 QueueDepth++ 再 send；drop 路径回滚 depth，避免与 writer 竞争短暂少算 - Enqueue/Close 协调：select 内监听 Done，Close 期间入队被识别为 drop - writeLoop drain 使用独立的 2s context，避免与已 cancel 的正常 ctx 撞车 router_test.go: - TestClientConnCloseStopsWriteLoop 改用真实 WebSocket pair 验证 writeLoop 真的在跑 - newTestWebSocketPair 返回 server+client 两侧 conn - 移除依赖 nil conn 触发 writeLoop 立即返回的旧实现 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 CodeRabbit Re-review Request |
|
✅ P02 CodeRabbit review 处理完毕 针对 router.go 的 3 条 actionable 评论已全部修复（commit fc0d9c6）：
针对 router_test.go 的 1 条评论已修复：
针对 e2e spec 的 3 条评论（host matrix / TLS probe / CI mode）属于 e2e 实施阶段 spec，将随 T00-T08 一起在 e2e 实施 PR 落地，不在本 PR 改 spec。 验证：
@coderabbitai 麻烦对 router.go 三条评论复核并标 resolved。 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kvmaker/mesh"
pr="1"
echo "== AST outline: internal/tunnel/router.go =="
ast-grep outline internal/tunnel/router.go --items all --view expanded || true
echo
echo "== AST outline: internal/tunnel/router_test.go =="
ast-grep outline internal/tunnel/router_test.go --items all --view expanded || true
echo
echo "== internal/tunnel/router.go relevant slice =="
sed -n '1,260p' internal/tunnel/router.go
echo
echo "== internal/tunnel/router_test.go relevant slice =="
sed -n '1,320p' internal/tunnel/router_test.go
echo
echo "== Commit fc0d9c6 touched files =="
git show --stat --oneline --name-only fc0d9c6
echo
echo "== Commit fc0d9c6 diff for router.go and router_test.go =="
git show --unified=40 --format=medium fc0d9c6 -- internal/tunnel/router.go internal/tunnel/router_test.go
echo
echo "== Unresolved review threads on PR `#1` (with comment excerpts) =="
gh api graphql -F owner="kvmaker" -F name="mesh" -F number="$pr" -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
originalLine
comments(first: 20) {
nodes {
id
author { login }
body
url
createdAt
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| "THREAD\t\(.id)\tpath=\(.path // "")\tline=\(.line // 0)\toriginal=\(.originalLine // 0)\toutdated=\(.isOutdated)\n"
+ (
.comments.nodes
| map(" COMMENT\t\(.id)\tauthor=\(.author.login // "")\turl=\(.url)\n BODY:\n" + (.body | gsub("\r"; "") | split("\n")[:12] | join("\n")) + "\n")
| join("\n")
)
'Length of output: 36165
另外 我也查了 PR 所以这 3 条 ✏️ Learnings added
|
2 participants
概要
验证
go build ./...通过go vet ./...无告警go test ./... -race全部通过关联
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests