Copilot commented on code in PR #6336:
URL: https://github.com/apache/texera/pull/6336#discussion_r3564912856
##########
.github/scripts/smoke-boot.sh:
##########
@@ -67,48 +72,49 @@ port_open() {
fi
}
+# Wait for the service to reach one of three terminal states: it opens its port
+# (booted), it exits on its own (crashed), or neither happens in time (hung).
outcome="timeout"
for ((i = 0; i < timeout; i++)); do
if port_open; then outcome="listen"; break; fi
if ! kill -0 "$pid" 2>/dev/null; then outcome="exited"; break; fi
sleep 1
done
-# Stop the service (it may already be gone). SIGTERM, then a bounded grace
-# period, then SIGKILL -- so a service that ignores SIGTERM or hangs in
shutdown
-# can't leave the CI step running indefinitely.
-kill "$pid" 2>/dev/null || true
-for _ in $(seq 1 10); do
- if ! kill -0 "$pid" 2>/dev/null; then
- break
- fi
- sleep 1
-done
-if kill -0 "$pid" 2>/dev/null; then
- kill -9 "$pid" 2>/dev/null || true
-fi
-wait "$pid" 2>/dev/null || true
-
fail() {
echo "::error::smoke-boot: $*"
echo "----- last 80 log lines from '$launcher' -----"
tail -n 80 "$log" || true
exit 1
}
-# A linkage/module error on boot fails regardless of whether the port came up.
-if grep -qE "$crash_re" "$log"; then
- fail "'$launcher' hit a runtime classpath/linkage error on boot"
-fi
+# Stop a still-running service. SIGTERM, then a bounded grace period, then
+# SIGKILL -- so a service that ignores SIGTERM or hangs in shutdown can't leave
+# the CI step running indefinitely.
+stop_service() {
+ kill "$pid" 2>/dev/null || true
+ for _ in $(seq 1 10); do
+ kill -0 "$pid" 2>/dev/null || return 0
+ sleep 1
+ done
+ kill -9 "$pid" 2>/dev/null || true
+ wait "$pid" 2>/dev/null || true
+}
case "$outcome" in
listen)
+ stop_service
echo "smoke-boot: OK -- '$launcher' reached LISTEN on :$port"
;;
exited)
- fail "'$launcher' exited before listening on :$port"
+ # The service died before it ever listened -- a boot crash. Its own exit
+ # code is the signal; reap it (the process has already exited) and report
it.
+ code=0
+ wait "$pid" 2>/dev/null || code=$?
+ fail "'$launcher' exited on boot (exit code $code) before listening on
:$port"
Review Comment:
In the "exited" outcome, the script doesn’t actually capture the child
process’s real exit code. `wait` returns the child’s exit status on success;
with the current `code=0` + `wait ... || code=$?` pattern, a failing process
will typically be reported as exit code 0, defeating the purpose of reporting
the crash code.
##########
.github/scripts/smoke-boot.sh:
##########
@@ -67,48 +72,49 @@ port_open() {
fi
}
+# Wait for the service to reach one of three terminal states: it opens its port
+# (booted), it exits on its own (crashed), or neither happens in time (hung).
outcome="timeout"
for ((i = 0; i < timeout; i++)); do
if port_open; then outcome="listen"; break; fi
if ! kill -0 "$pid" 2>/dev/null; then outcome="exited"; break; fi
sleep 1
done
-# Stop the service (it may already be gone). SIGTERM, then a bounded grace
-# period, then SIGKILL -- so a service that ignores SIGTERM or hangs in
shutdown
-# can't leave the CI step running indefinitely.
-kill "$pid" 2>/dev/null || true
-for _ in $(seq 1 10); do
- if ! kill -0 "$pid" 2>/dev/null; then
- break
- fi
- sleep 1
-done
-if kill -0 "$pid" 2>/dev/null; then
- kill -9 "$pid" 2>/dev/null || true
-fi
-wait "$pid" 2>/dev/null || true
-
fail() {
echo "::error::smoke-boot: $*"
echo "----- last 80 log lines from '$launcher' -----"
tail -n 80 "$log" || true
exit 1
}
-# A linkage/module error on boot fails regardless of whether the port came up.
-if grep -qE "$crash_re" "$log"; then
- fail "'$launcher' hit a runtime classpath/linkage error on boot"
-fi
+# Stop a still-running service. SIGTERM, then a bounded grace period, then
+# SIGKILL -- so a service that ignores SIGTERM or hangs in shutdown can't leave
+# the CI step running indefinitely.
+stop_service() {
+ kill "$pid" 2>/dev/null || true
+ for _ in $(seq 1 10); do
+ kill -0 "$pid" 2>/dev/null || return 0
+ sleep 1
+ done
+ kill -9 "$pid" 2>/dev/null || true
+ wait "$pid" 2>/dev/null || true
+}
Review Comment:
`stop_service` can return without calling `wait` when the process has
already exited (`kill -0` fails). That leaves the child unreaped (zombie) until
the shell exits, and it can accumulate across repeated smoke-boot invocations
in the same CI job.
##########
.github/scripts/test_smoke_boot.sh:
##########
@@ -0,0 +1,94 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# End-to-end regression tests for smoke-boot.sh. Each case launches a fake
+# "service" through smoke-boot.sh and checks its verdict from the exit code --
+# smoke-boot no longer scans logs, so its decision is driven purely by whether
+# the process reaches LISTEN, exits, or hangs.
+#
+# Guards issue #6332: a service that boots fine but prints exception-name prose
+# in its log (e.g. jOOQ's random "tip of the day") must PASS; and a service
that
+# crashes on boot must still FAIL.
+
+set -uo pipefail
+
+command -v python3 >/dev/null || { echo "python3 is required to run these
tests" >&2; exit 1; }
+
+script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+smoke="$script_dir/smoke-boot.sh"
+work="$(mktemp -d)"
Review Comment:
`mktemp -d` without a BSD-compatible fallback will fail on macOS (BSD
`mktemp` requires `-t` or a template). Since the `infra` job runs on
`macos-latest`, this test script will error out there.
##########
.github/scripts/smoke-boot.sh:
##########
@@ -67,48 +72,49 @@ port_open() {
fi
}
+# Wait for the service to reach one of three terminal states: it opens its port
+# (booted), it exits on its own (crashed), or neither happens in time (hung).
outcome="timeout"
Review Comment:
The `infra` job runs this script on `macos-latest`, but `log="$(mktemp)"`
(line 62) is not portable to BSD/macOS `mktemp` (it requires `-t` or a
template). This will cause smoke-boot to fail before launching the service on
macOS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
