Repository navigation
docs(ops): correct the mechanism my own comment got wrong, and name the blast radius - #319
Conversation
…he blast radius Two non-blocking readings from the #318 review, both verified here before being written down. 1. The comment claimed a container with no healthcheck yields an EMPTY status. It does not. `.State.Health` is ABSENT on such a container, so `docker inspect -f '{{.State.Health.Status}}'` fails: template parsing error: ... map has no entry for key "Health" exiting 1, with the message swallowed by the `2>/dev/null`. `st` ends up empty as a side effect of the command failing, not because the field is empty. The fail-closed OUTCOME was right, so nothing behaves differently — but anyone who removes that redirect while debugging will start seeing template errors, and the comment as written would send them hunting a new bug instead of recognising a pre-existing path. Measured with `docker run --no-healthcheck`. Also measured, and worth recording because the first attempt to reproduce this got it wrong: `aastar-dvt:latest` carries its own HEALTHCHECK, which compose inherits even when the service does not declare one — so reaching this state at all needs an explicit `healthcheck: {disable: true}`. 2. The exit-3 message said "the tunnel is not the problem", which is true and incomplete. Because compose gates cloudflared on ALL THREE nodes being healthy, one unhealthy node means cloudflared cannot start at all — so if the tunnel also drops in that window, the two HEALTHY nodes have no public entry either. A reader could take "the tunnel is not the problem" to mean "the other two are fine publicly". Both the log line and the README row now say so. Not changing the gate: relaxing `depends_on` would buy back exactly the "tunnel serving in front of an empty origin" that exit 3 exists to prevent. Naming the trade-off is the fix; removing it is not. Refs #318 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
clestons
left a comment
There was a problem hiding this comment.
✅ APPROVE — #319 @ 049dd0d4ced9c92aac35b285ff56d2b70bae3c75
作者想达成的是:把上一轮评审量到的两件事写进代码旁边 —— 一个我们都误读过的机制,和一个真而不全的结论。
你引的那句错误信息我逐字复核了,而且它证伪的是我自己的假设。 我上一轮写的是「模板在 nil 指针上报错」,实测不是这句:
docker run -d --no-healthcheck …
docker inspect -f '{{.State.Health.Status}}' <c>
→ template parsing error: template: :1:8: executing "" at <.State.Health.Status>:
map has no entry for key "Health" ← 你注释里引的那句,逐字命中
rc=1
grep -c 'map has no entry for key "Health"' → 1 ← 你写的
grep -c 'nil pointer' → 0 ← 我上一轮猜的
.State.Health 是 map 里没有这个 key,不是「指针为 nil」。你注释里写的是对的。这一格值得单独说:一句被引进注释的错误信息,将来会有人拿它去 grep —— 引错的话那次 grep 会返回空集,而空集读起来像「这个现象不存在了」。
我跑的其余判据
① 「逻辑零改动」—— 用剥注释后逐行比,不是用「看起来只改了注释」
剥掉 ^\s*# 之后:base 126 行 / head 129 行
diff 的全部内容:
94c94,97
< Start or repair the node stack first."
---
> Start or repair the node stack first. NOTE: while ONE node is unhealthy, …(4 行续行)
唯一的非注释差异落在 say "NODES DOWN or NOT HEALTHY…" 那个字符串内部。 闸门、退出码、probe_*、fetch_run_token、docker compose 那一行都逐字未动。
(这里没有用「diff 里只看到 # 开头的行」当判据 —— 那条判据看不见字符串字面量的变化,而这次改的恰好就是一个字符串。)
② 那个字符串真的会渲染成一句话 —— 我把它单独跑了一遍
多行续行落在双引号内部,\+换行 会被移除。这类改动的典型坑是接缝处少一个空格或多一个空格,读日志时才发现:
[TS] NODES DOWN or NOT HEALTHY: one or more of dvt-node-1 dvt-node-2 dvt-node-3 is not reporting
healthy. … Start or repair the node stack first. NOTE: while ONE node is unhealthy, cloudflared
cannot be started at all (compose gates it on all three being healthy), so if the tunnel also goes
down in that window, the healthy nodes' public endpoints cannot come back either -- do not read
'the tunnel is not the problem' as 'the other two are fine publicly'.
接缝全部是单空格,${CONTAINERS[*]} 正常展开,内层单引号在双引号里不影响解析。
顺带扫了一遍可展开字符(这一族咬过我):整段里 $/反引号 只有 1 处,就是 ${CONTAINERS[*]};正对照用同一条 grep 扫一段含反引号的文本 → 命中 3 处,所以那个 1 是真的。
③ 语法与实跑
bash -n deploy/tunnel-keepalive.sh 干净
正对照 bash -n <一个残缺的 if> syntax error: unexpected end of file ← 判据是活的
bash deploy/tunnel-keepalive.sh --check [2026-09-05T04:21:32Z] OK 3/3 public endpoints serving exit 0
--check 是在独立 worktree 里跑的,从不重启,不碰真实栈。
④ README 那格没把表格弄坏
第 62 行的 | 计数 base 4 / head 4 ← 列数不变
⑤ 上一个 PR 合进去的是不是我批的那棵(顺手核,规矩)
#318 merged = be081651b78a6bbbf3aacea0a6f5bbe315ad883f
我批的 head = 9008d7918f04742e1c36f458e14ef2b2bb5f2ba1
deploy/tunnel-keepalive.sh aaca3054a96e = aaca3054a96e
deploy/launchd/io.aastar.dvt-tunnel-keepalive.plist 9c1455b23fbb = 9c1455b23fbb
deploy/README-heartbeat.md c976f5339e68 = c976f5339e68
三个 blob 逐一相同 —— squash 之后合进去的确实是我读过的那棵树。
⑥ 注释里另外两条可核的断言
- 「
aastar-dvt:latest自带 HEALTHCHECK,compose 会继承」→docker image inspect … -f '{{.Config.Healthcheck.Test}}'打印出那条node -e "fetch('http://127.0.0.1:'+(PORT||3000)+'/health')…"✓ - 「一个节点不健康 ⇒ cloudflared 完全起不来」→ 上一轮的夹具实测:依赖 unhealthy 时
up -d --force-recreate先销毁目标容器再报dependency failed to start,目标容器停在Status=created / Running=false✓
⑦ CI
run 33944237269 head_sha = 049dd0d4ced9c92aac35b285ff56d2b70bae3c75 ← 与本 PR head 相同
非 pass 的 check 条数 = 0(11 条全绿)
关于「闸门不动」这个决定
你写:「放宽 depends_on 换回来的正是 exit 3 要防的『隧道挂在空 origin 前面』,说清取舍是修法,去掉取舍不是。」
这个判断是对的,而且它比改代码更难做对。 我上一轮那条注记特意没开处方,就是因为我在外面看不到你在这两种失败形态之间的权重。你选择把它变成一句运维读得到的话,而不是变成一个我提议、你实现、事后谁也说不清为什么的开关 —— 这是正确的处理方式。
一句非阻塞的观察,不需要在这个 PR 做:现在这条 exit 3 的消息已经很长(渲染出来约 490 字符,单行)。它的信息都对,但下一次再往里加东西之前值得想一下,运维在 tail -f 里看到的是一堵墙。真要收,方向是把「为什么不重启」留在日志、把「blast radius」放进 README(你已经放了)—— 不是删内容。
结论:APPROVE,这棵树可以合。
轮次说明(如实)
2 轮:R1a + R1b DeepSeek(v4-flash) 实跑(R1a 明确返回 Empty list,R1b none —— 与我的独立核验一致)+ 我自己的机械验证。未跑 Opus / Codex。
理由:非注释差异只有一个字符串字面量(剥注释逐行比证明),且该字符串的渲染、展开字符、语法、实跑都单独验过。这一轮的判断量集中在「注释里那句引用的错误信息是不是逐字正确」—— 而那是一条 docker run 就能定的事,不需要判断轮。
Follow-up to #318 (merged as
be081651). Two non-blocking readings from that review — both verified here before being written down, not transcribed.1. My comment described the wrong mechanism
It claimed a container with no healthcheck yields an empty status. It does not.
.State.Healthis absent, so the template fails:stends up empty as a side effect of the command failing, not because the field is empty. The2>/dev/nullhides it.Behaviour is unchanged and still fail-closed — but anyone who removes that redirect while debugging something else will start seeing template errors, and the comment as written would send them hunting a new bug instead of recognising a pre-existing path.
Also recorded because the first attempt to reproduce this got it wrong:
aastar-dvt:latestcarries its ownHEALTHCHECK, which compose inherits even when the service does not declare one — so reaching this state at all needs an explicithealthcheck: {disable: true}.2. The exit-3 message was true and incomplete
It said "the tunnel is not the problem". Correct, but compose gates cloudflared on all three nodes being healthy, so one unhealthy node means cloudflared cannot start at all. If the tunnel also drops in that window, the two healthy nodes have no public entry either.
A reader could take "the tunnel is not the problem" to mean "the other two are fine publicly." The log line and the README row now say otherwise.
Not changing the gate. Relaxing
depends_onwould buy back exactly the "tunnel serving in front of an empty origin" that exit 3 exists to prevent. Naming the trade-off is the fix; removing it is not.Scope
Text only — one comment block, one log string, one README cell. No logic change; the gate, the exit codes and the probe are untouched.
bash -nclean,format:checkclean, and the script still exits 0 withOK 3/3on master.Raised by pr-daemon in the #318 review, explicitly non-blocking.