fix(cli): shorten sandbox hostname for image-ID container names - #10624
Conversation
A regular container name derived from an image ID (sha256-<64hex> plus the -<8hex> suffix = 80 chars) exceeds Linux HOST_NAME_MAX (64), so `docker run` fails with `sethostname: invalid argument` (exit 125) whenever QWEN_SANDBOX_IMAGE is an image ID -- the documented shape after QwenLM#10605. Keep the full name for --name and derive --hostname from a short hash when the container name is too long. Follow-up to F1 in the QwenLM#10605 verification report. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR, @yiliang114! Template looks good ✓ Problem: an observed bug, not theory — verified against the base code. Direction: aligned — it restores behavior that the just-merged #10605 documents. It sits inside the sandbox area, though, so per gate policy this escalates for a maintainer's sign-off instead of continuing through automated review; stopping here after the gate. Claude Code's CHANGELOG has no direct counterpart for this, but the sandbox-orchestration area is actively evolving there too, so the area is relevant. Size: not a core-module path. 17 production lines (+15/−2 in Approach: the scope feels right — it matches what I'd have done. Keeping the full name for Risk: ⏸️ Escalating to a maintainer. Sandbox-machinery changes need a human direction sign-off before the bot takes them further. No owner resolved deterministically (the PR has no labels, and the owners map has no area covering 中文说明感谢贡献,@yiliang114! 模板完整 ✓ 问题:是已观测到的 bug,不是理论推演——已对照基线代码验证。 方向:对齐——恢复的是刚合并的 #10605 所承诺的行为。但改动位于 sandbox 领域,按门禁策略需升级给维护者确认方向,不再继续自动化审查;门禁之后到此为止。Claude Code 的 CHANGELOG 没有直接对应项,但那边 sandbox 编排也在持续演进,该领域是相关的。 规模:非核心模块路径。生产代码 17 行( 方案:范围合理——和我的独立方案一致。 风险: ⏸️ 升级给维护者。 sandbox 机制的改动在机器人继续推进前需要人工确认方向。本次未确定性地解析出归属人(PR 无标签,归属映射也没有覆盖 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Reviewed at head 685621561e (updated from the original head — the approach was simplified; re-reviewed in full).
Change summary: omit --hostname entirely when containerName.length > 64, instead of passing a hash-derived short name. Docker then defaults the hostname to the first 12 characters of the container ID, which is always a valid hostname. Simpler and strictly correct.
What was checked:
parseSandboxImageName("sha256:" + 64hex)→sha256-<64hex>(71 chars) →containerName = sha256-<64hex>-<8hex>= 80 chars. Trigger confirmed.- Threshold
containerName.length <= 64: Linuxsethostname(2)rejectslen > 64, so exactly-64-char names pass--hostname containerNameunchanged. Correct. - When
--hostnameis omitted, Docker defaults to the container's short ID (12 hex chars). Always valid. import { randomBytes } from "node:crypto"—createHashis NOT present (the first commit added it; the updated commit removed it, keeping the import clean).- Integration-test path (
qwen-code-integration-test-<8hex>= 35 chars):length <= 64, behavior unchanged. - Proxy container (
SANDBOX_PROXY_NAME = "qwen-code-sandbox-proxy", 24 chars): no--hostnamearg, Docker default, unaffected. - Existing test now also asserts
--hostname == --namefor normal short names — confirms no regression on the happy path. - New test: asserts
--name > 64and--hostnameabsent from args. Would fail if the fix were reverted.
CI at head 685621561e:
Test (ubuntu-latest, Node 22.x)— pending at review time (still running); the prior head's run of the same job passed 30/30.Test (windows-latest, Node 22.x)— SKIPPED (pre-existing matrix condition; no platform-specific code in this fix).Test (macos-latest, Node 22.x)— SKIPPED (same).Integration Tests (CLI, No Sandbox)— SKIPPED.- All other completed checks: success.
Unreviewable dimensions: Docker end-to-end not exercised; macOS/Windows unit jobs SKIPPED (pre-existing skip, not introduced by this PR).
Reviewed with AI assistance.
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed: no findings. The 64-char bound is the POSIX hostname limit that image-ID container names (sha256-<64hex>-<8hex>) exceed, while the container --name itself may be longer — omitting only --hostname and letting the runtime pick one is the right cut. Both cases are pinned: the existing regular-image test now asserts hostname==name, and the new image-ID test asserts no --hostname flag. CI green. (Self-PR: GitHub blocks self-approval, needs another maintainer.)
|
No blocking findings. The conditional preserves existing hostname behavior for valid container names and lets Docker select a valid default only for oversized image-ID-derived names. The updated tests cover both paths. |
|
@qwen-code-review-bot approve it if no blockers |
|
Approved — no blocking findings. |
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Gate APPROVE at head 68562156 on the review conclusion, per maintainer instruction to decide independently of CI. Code review is complete and clean: the final approach (omit --hostname when the container name exceeds the 64-byte HOST_NAME_MAX instead of passing it) is the strictly safer shape — the ≤64 path keeps hostname==name pinned by test, and the >64 image-ID path previously died with exit 125 (sethostname invalid argument), so no existing behavior depended on the flag there; no repo consumer reads the sandbox container's hostname (token salt / writer-lease / worktree paths all run outside this comparison or were equally broken pre-fix). 0 unresolved threads. Note for the record: Test (ubuntu-latest) was still running at submission time; Serve A/B and the E2E leg passed on the prior head.
中文:按维护者指示以自身 Review 结论投票,不再等待 CI。最终方案(容器名超 64 时省略 --hostname)经独立核对无行为回归:≤64 路径保持 name==hostname 且被测试钉住,>64 路径旧行为本就是 exit 125 全坏,无既有依赖;仓内无消费方读取沙箱容器 hostname。线程零未解决。记录在案:提交时 Test (ubuntu) 仍在运行,非红。
|
Released in v0.23.0. |
What this PR does
In
start_sandbox, the regular container name is derived from the image name. WhenQWEN_SANDBOX_IMAGEis an image ID,parseSandboxImageNameproducessha256-<64hex>, and the-<8hex>suffix brings the full container name to 80 characters. That name was passed todocker runas both--nameand--hostname. Since LinuxHOST_NAME_MAXis 64, the hostname is invalid and the container fails to start withsethostname: invalid argument(exit 125).This PR keeps the full name for
--name, and only when the container name exceeds 64 characters, derives--hostnamefrom a short hash (qwen-sandbox-<12hex>, 25 chars). Short names — including the integration-test container names — keep the previous behavior unchanged.Why it's needed
Follow-up to F1 in the #10605 verification report: after #10605 made image IDs a documented shape for
QWEN_SANDBOX_IMAGE, a regular (non-integration-test) sandbox launch with an image-ID image always failed, while CI stayed green because integration tests use a short generated name.Reviewer Test Plan
How to verify
Run the new unit test:
The new case feeds
sha256:<64hex>as the image and asserts the resulting--nameis longer than 64 chars while--hostnameis withinHOST_NAME_MAX. Full suite output:Test Files 1 passed (1),Tests 30 passed (30).Evidence (Before & After)
N/A (non-user-visible; unit-test covered). Before:
docker run --hostname <80-char-name>exits 125 withsethostname: invalid argument. After:--hostnameisqwen-sandbox-<12hex>and the container starts normally.Tested on
Environment (optional)
Unit tests only (vitest); no Docker runtime exercised locally.
Risk & Scope
scripts/tests/e2e-workflow.test.js) — separate follow-up.Linked Issues
Follow-up to F1 in the #10605 verification report.
中文说明
这个 PR 做了什么
start_sandbox中的普通容器名由镜像名派生。当QWEN_SANDBOX_IMAGE是镜像 ID 时,parseSandboxImageName得到sha256-<64hex>,再加-<8hex>后缀,完整容器名达到 80 个字符。此前这个名字同时作为--name和--hostname传给docker run。由于 Linux 的HOST_NAME_MAX是 64,hostname 非法,容器启动直接失败:sethostname: invalid argument(exit 125)。本 PR 保持
--name使用完整名字;仅当容器名超过 64 字符时,--hostname改为由短哈希派生(qwen-sandbox-<12hex>,25 字符)。短名字(包括集成测试的容器名)行为完全不变。为什么需要
这是 #10605 验证报告中 F1 的 follow-up:#10605 之后镜像 ID 成为
QWEN_SANDBOX_IMAGE的合法形态,但常规(非集成测试)沙箱在镜像 ID 形态下必然启动失败;CI 之所以没发现,是因为集成测试用的是较短的生成名。审阅者验证方式
如何验证
运行新增单测:
cd packages/cli && npx vitest run src/serve/sandbox.test.ts。新用例以sha256:<64hex>作为镜像,断言--name超过 64 字符而--hostname不超过 64。完整输出:Test Files 1 passed (1)、Tests 30 passed (30)。前后对比
不适用(非用户可见,已由单测覆盖)。修复前:
docker run --hostname <80字符名>以 125 退出并报sethostname: invalid argument;修复后:--hostname为qwen-sandbox-<12hex>,容器正常启动。测试平台
仅 🐧 Linux(✅);macOS / Windows 未测。
环境
仅单元测试(vitest),未在本机实际运行 Docker。
风险与范围
scripts/tests/e2e-workflow.test.js一行契约测试)将单独跟进。关联 Issue
#10605 验证报告 F1 的 follow-up(不自动关闭)。