Skip to content

Commit 74802f5

Browse files
charliekclaude
andcommitted
fix(deb): three CodeRabbit findings on the extracted scripts
`resolve-one-deb.sh` accepted an empty `--arch`, which fell through to the unfiltered branch and matched any .deb — silently disabling the very guard the flag exists for. release.yml passes `"${ARCH}"` from the matrix, so an unset ARCH could have handed an arm64 package to the job that labels it Intel/AMD. `smoke-deb.sh`'s cleanup did an unbounded `wait` before its KILL sweep, so the sweep was unreachable in exactly the case it existed for: anything in the process group ignoring SIGTERM would block the EXIT trap until the job timeout. Same hang class this plan already fixed in the closure script, on the same release-gating path. Now a bounded poll, then KILL, then reap. The closure script's exit-code mapping conflated its two timeouts. Both `timeout` calls exit 124 and the inner one propagates out as the container status, so a launch timeout reported itself as the 900s docker budget being exceeded — the same misdiagnosis this script was rewritten to stop making, one layer down. The inner timeout is now remapped to 125 with its own message. All three verified in the shed: empty `--arch` rejected; smoke and closure still pass on a good .deb; and the new 125 path exercised end-to-end against the stripped-Depends .deb with a short launch budget, which correctly reports "did not answer roostctl identify within 5s ... the package DID install" instead of blaming the docker budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
1 parent b41dd3f commit 74802f5

3 files changed

Lines changed: 29 additions & 5 deletions

File tree

linux/scripts/resolve-one-deb.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ while [ "$#" -gt 0 ]; do
3030
;;
3131
--arch)
3232
[ "$#" -ge 2 ] || { usage >&2; die "--arch requires a value"; }
33+
# An empty value would fall through to the unfiltered branch and match
34+
# any .deb, silently disabling the guard this flag exists for.
35+
# release.yml passes "${ARCH}" from the matrix, so an unset ARCH could
36+
# otherwise hand an arm64 package to the job that labels it Intel/AMD.
37+
[ -n "$2" ] || { usage >&2; die "--arch requires a non-empty value"; }
3338
arch="$2"
3439
shift 2
3540
;;

linux/scripts/smoke-deb.sh

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,19 @@ cleanup() {
8484
if [ -n "${APP}" ]; then
8585
# TERM the whole process group, not just $APP: $APP is the xvfb-run
8686
# wrapper, and reaping it does not necessarily reap the Xvfb + roost
87-
# children it started. `wait` before the KILL sweep so the UI gets a
88-
# chance to shut down cleanly.
87+
# children it started.
8988
kill -TERM -- "-${APP}" 2>/dev/null || true
90-
wait "${APP}" 2>/dev/null || true
89+
# Poll for a clean exit on a deadline rather than a bare `wait`. An
90+
# unbounded wait here would block the EXIT trap forever if anything in
91+
# the group ignored SIGTERM — the same hang this script's sibling was
92+
# rewritten to prevent, and on the same release-gating path. The KILL
93+
# sweep has to be reachable, so it cannot sit behind the wait.
94+
for _ in $(seq 1 20); do
95+
kill -0 "${APP}" 2>/dev/null || break
96+
sleep 0.5
97+
done
9198
kill -KILL -- "-${APP}" 2>/dev/null || true
99+
wait "${APP}" 2>/dev/null || true
92100
fi
93101
# Only ever delete a directory this script created. A caller-supplied
94102
# --work-dir keeps its contents — after a failure the app log in there is

linux/scripts/verify-deb-closure.sh

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,15 @@ timeout "${DOCKER_TIMEOUT}" docker run --rm \
9393
# turns a finished check into a hung one.
9494
kill \"\$ui\" 2>/dev/null || true
9595
wait \"\$ui\" 2>/dev/null || true
96-
"
96+
" || {
97+
inner=$?
98+
# Re-map the inner timeout off 124. Both timeouts exit 124, and the inner
99+
# one propagates out as the container status, so leaving it would make the
100+
# launch timeout report itself as the outer docker budget — the same
101+
# misdiagnosis this script was rewritten to stop making, one layer down.
102+
[ "$inner" -eq 124 ] && { tail -40 /tmp/app.log || true; exit 125; }
103+
exit "$inner"
104+
}
97105
'
98106
rc=$?
99107
set -e
@@ -104,7 +112,10 @@ set -e
104112
case "${rc}" in
105113
0) ;;
106114
124)
107-
die "the closure check timed out after ${DOCKER_TIMEOUT}s (docker pull, apt, or the UI launch wedged). This is a harness/environment failure, NOT evidence about the Depends: list."
115+
die "the closure check exceeded its overall ${DOCKER_TIMEOUT}s budget before the UI launch was reached (docker pull or apt wedged). This is a harness/environment failure, NOT evidence about the Depends: list."
116+
;;
117+
125)
118+
die "the installed package did not answer roostctl identify within ${LAUNCH_TIMEOUT}s and the launch was killed. Distinct from the ${DOCKER_TIMEOUT}s budget above: the package DID install, so this points at the app or its runtime dependencies, not at docker or apt."
108119
;;
109120
*)
110121
die "the .deb installed but did not come up in a clean container (exit ${rc}). The most likely cause is an incomplete Depends: list — the container has nothing preinstalled — but check the log above before concluding that: a docker or apt-mirror failure lands here too."

0 commit comments

Comments
 (0)