From 2454cee4c53dc6b4357230677ff6c494be8230e2 Mon Sep 17 00:00:00 2001 From: Shekhar Mudarapu Date: Fri, 31 Jul 2026 11:23:01 -0400 Subject: [PATCH 1/4] fix(microvm): reap the manager's whole process group on teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed integration-test run left a live runtime-manager holding :50054. Every LATER run then could not bind and reported "Firecracker backend did not report available" — a symptom that points at the backend, or at whatever was last edited, rather than at the leaked process. It cost three runs during the harness startup work and briefly looked like a code regression; the A/B that cleared it only worked because the host was cleaned by hand in between. `kill "${MANAGER_PID}"` was already there. It was not enough, in two ways that compound: the manager does not reliably exit on SIGTERM, and its firecracker children are reparented rather than killed when it does. So the manager now starts via `setsid`, and cleanup signals the whole process group — TERM, a bounded wait for a graceful exit, then KILL. Deliberately not a host-wide `pkill -f firecracker`: that would kill unrelated VMs, and on the usual dev host (a Lima VM named `firecracker-dev`) the pattern also matches the caller's own shell command line and kills the session running the script. The group id is read back with `ps -o pgid=` rather than assumed equal to the captured pid: `setsid` execs in place when the caller is not already a group leader but forks when it is, and only one of those keeps that pid. Teardown then waits for the PORT, not the pid — the port is what the next run needs back — and warns if it is still held, so the leak is reported by the run that caused it. A matching pre-flight fails fast when the port is already taken, naming the likely cause and the command to find it, so the misleading message above cannot recur. Verified on real KVM, all three paths: - forced failure (corrupt rootfs, panics after the manager is up) -> manager 0, firecracker 0, port free [previously: manager alive, port held] - normal run -> 5/5 assertions, host clean afterwards; the TAP orphaned by the failed run above went 1 -> 0, confirming the manager's startup reconciliation handles the one thing group-kill cannot - port pre-occupied -> fails immediately with the real cause, not a backend error --- infra/firecracker/integration-test.sh | 69 +++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/infra/firecracker/integration-test.sh b/infra/firecracker/integration-test.sh index ad70ab63..594695dc 100755 --- a/infra/firecracker/integration-test.sh +++ b/infra/firecracker/integration-test.sh @@ -94,13 +94,63 @@ SECRET_VALUE="itest-secret-value-do-not-log" # manager rejects an untenanted spec outright. ITEST_TENANT_ID="00000000-0000-0000-0000-000000000001" +# Is something listening on the manager's gRPC port? +# +# Defined up here because cleanup uses it too: the port — not the pid — is what +# the NEXT run actually needs back. No `nc` dependency; bash /dev/tcp is enough. +port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; } + +# Reap the manager AND everything it spawned. +# +# `kill "${MANAGER_PID}"` was not enough, in two ways that compound: the manager +# does not reliably exit on SIGTERM, and its firecracker children are reparented +# rather than killed when it does. A failed run therefore left a live manager +# holding :50054, and every LATER run then failed to bind and reported +# "Firecracker backend did not report available" — a symptom that points at the +# backend, or at whatever was last edited, rather than at the leaked process. +# That cost three runs and briefly looked like a code regression. +# +# The manager is started in its own process group (see `setsid` below) so the +# whole tree can be signalled by group id. Deliberately NOT a host-wide +# `pkill -f firecracker`: that would kill unrelated VMs, and on the usual dev +# host — a Lima VM named `firecracker-dev` — the pattern also matches the +# caller's own shell command line and kills the session running this script. cleanup() { - [ -n "${MANAGER_PID:-}" ] && kill "${MANAGER_PID}" 2>/dev/null || true - # Best-effort teardown of any leaked TAPs / sockets from a failed boot. + if [ -n "${MANAGER_PGID:-}" ]; then + kill -TERM "-${MANAGER_PGID}" 2>/dev/null || true + # Give a graceful shutdown a bounded chance (~6s), then stop asking. + for _ in $(seq 1 30); do + port_open || break + sleep 0.2 + done + kill -KILL "-${MANAGER_PGID}" 2>/dev/null || true + fi + + # Confirm the port came back. If it did not, say so HERE — the next run would + # otherwise fail with the misleading message described above. + for _ in $(seq 1 25); do + port_open || break + sleep 0.2 + done + if port_open; then + printf '\033[1;33m[integration] WARN:\033[0m %s\n' \ + "port 50054 still held after teardown; the next run will fail to bind. \ +Find it with: sudo ss -lntp | grep 50054" >&2 + fi + rm -rf "${WORK}" 2>/dev/null || true } trap cleanup EXIT +# Fail fast, and for the RIGHT reason, if the port is already taken — by a +# manager leaked from an earlier run, or by anything else. Without this the +# script starts a manager that cannot bind and reports a backend problem. +if port_open; then + fail "something is already listening on 127.0.0.1:50054 (likely a runtime-manager + leaked by an earlier run). This script cannot start its own manager until that + is gone. Find and stop it with: sudo ss -lntp | grep 50054" +fi + # --------------------------------------------------------------------------- # 1. Mint a manager mTLS CA + server cert. The Firecracker backend issues a # per-VM client cert (CN=vm_id) signed by this CA at spawn; the harness @@ -160,7 +210,10 @@ fi [ -x "${MANAGER_BIN}" ] || fail "manager binary not found at ${MANAGER_BIN}" log "Starting runtime-manager (RUNTIME_BACKEND=firecracker) on ${MANAGER_ADDR}" -env \ +# `setsid` puts the manager in its own process group so cleanup can reap it and +# every firecracker VM it spawns as one unit, without signalling anything else +# on the host. +setsid env \ RUNTIME_BACKEND=firecracker \ LANTERN_RUNTIME_BACKEND=firecracker \ LISTEN_ADDR="${MANAGER_LISTEN_ADDR}" \ @@ -176,10 +229,16 @@ env \ "${SECRET_ENV_KEY}=${SECRET_VALUE}" \ "${MANAGER_BIN}" >"${MANAGER_LOG}" 2>&1 & MANAGER_PID=$! +# Read the group id back rather than assuming it equals MANAGER_PID: `setsid` +# execs in place when the caller is not already a group leader, but forks when +# it is, and only one of those keeps the pid we captured. The group id is what +# cleanup signals, so derive it instead of inferring it. +MANAGER_PGID="$(ps -o pgid= -p "${MANAGER_PID}" 2>/dev/null | tr -d ' ')" +: "${MANAGER_PGID:=${MANAGER_PID}}" # Wait for the gRPC port to accept connections. The manager has no gRPC -# reflection, so probe the raw TCP port via bash /dev/tcp (no nc dependency). -port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; } +# reflection, so probe the raw TCP port (port_open is defined near cleanup, +# which needs it too). for _ in $(seq 1 100); do port_open && break kill -0 "${MANAGER_PID}" 2>/dev/null || fail "manager exited early; log:\n$(cat "${MANAGER_LOG}")" From 4ba48a355651d9b31844f352c91d790da62c4e73 Mon Sep 17 00:00:00 2001 From: Shekhar Mudarapu Date: Fri, 31 Jul 2026 16:21:43 -0400 Subject: [PATCH 2/4] fix(microvm): resolve the manager's process group by handshake, not inference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both PR auditors flagged the first cut, and they were right. `setsid` behaves two different ways depending on whether its caller is already a process group leader, and the patch guessed at both: - Caller NOT a leader (a script under CI, job control off): setsid execs in place, so the leader keeps the pid `$!` returned — but only after setsid() has actually run. Reading the pgid immediately after `&` is a race, and losing it yields the SCRIPT'S OWN pgid. Cleanup would then have killed this script's process group, taking the runner step with it. That is the very self-kill the comment two lines above warns about for `pkill -f firecracker`, reached from the other direction. My verification passed only because the race happened to resolve favourably. - Caller IS a leader (sourced from an interactive shell): setsid forks, `$!` is the short-lived wrapper. The liveness check would have called it dead at once ("manager exited early"), and the `ps` fallback would have signalled a group the manager was never in — leaking exactly what this PR set out to fix. So stop inferring it. The manager now launches behind a one-line wrapper that publishes its own `$$` and then execs the manager. Inside that wrapper `$$` is the session leader in BOTH cases, and the exec keeps the pid, so the published value is the manager's pid and its process group id at once. The script waits for that file — a handshake, with no race left to lose — and uses it for liveness too. Cleanup additionally refuses to signal a group equal to this script's own. The handshake should make that unreachable; it is there because the cost of being wrong is the whole run plus its diagnostics. Verified on real KVM across both setsid paths and the failure path: - job control OFF, normal run -> 5/5, host clean - job control ON (fork path), normal run -> 5/5, host clean [previously: aborted immediately with "manager exited early"] - job control ON, forced failure -> correct FAIL; manager 0, firecracker 0, port free [previously: wrong group signalled, manager leaked] The refusal guard never fired in any run, which is the expected result. --- infra/firecracker/integration-test.sh | 53 +++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/infra/firecracker/integration-test.sh b/infra/firecracker/integration-test.sh index 594695dc..2ed0ea91 100755 --- a/infra/firecracker/integration-test.sh +++ b/infra/firecracker/integration-test.sh @@ -82,6 +82,10 @@ command -v firecracker >/dev/null 2>&1 || [ -n "${FC_BINARY_PATH:-}" ] \ [ -s "${FC_ROOTFS_PATH:-}" ] || fail "FC_ROOTFS_PATH unset/empty — run build-image.sh first" WORK="$(mktemp -d)" +# This script's own process group — cleanup refuses to signal it (see cleanup). +SELF_PGID="$(ps -o pgid= -p $$ 2>/dev/null | tr -dc '0-9')" +# Where the manager wrapper publishes its process group id (see the launch). +MANAGER_PGID_FILE="${WORK}/manager.pgid" MANAGER_ADDR="127.0.0.1:50054" # The guest reaches the manager over its point-to-point TAP link (172.16.x.1), # never loopback, so the manager has to bind all interfaces. grpcurl still @@ -116,6 +120,15 @@ port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; # host — a Lima VM named `firecracker-dev` — the pattern also matches the # caller's own shell command line and kills the session running this script. cleanup() { + # Never signal our own process group, whatever went wrong upstream: that would + # kill this script (and, under CI, the runner step) instead of the manager. + # The handshake below should make this unreachable; it is here because the + # cost of being wrong is losing the run and its diagnostics. + if [ -n "${MANAGER_PGID:-}" ] && [ "${MANAGER_PGID}" = "${SELF_PGID:-}" ]; then + printf '\033[1;33m[integration] WARN:\033[0m %s\n' \ + "refusing to signal process group ${MANAGER_PGID} — it is this script's own" >&2 + MANAGER_PGID="" + fi if [ -n "${MANAGER_PGID:-}" ]; then kill -TERM "-${MANAGER_PGID}" 2>/dev/null || true # Give a graceful shutdown a bounded chance (~6s), then stop asking. @@ -213,7 +226,26 @@ log "Starting runtime-manager (RUNTIME_BACKEND=firecracker) on ${MANAGER_ADDR}" # `setsid` puts the manager in its own process group so cleanup can reap it and # every firecracker VM it spawns as one unit, without signalling anything else # on the host. -setsid env \ +# +# The wrapper publishes the group id ITSELF rather than letting us infer it, +# because `setsid` behaves two different ways and neither is safe to guess at: +# +# - caller is NOT already a group leader (a script under CI, job control off): +# setsid execs in place, so the leader keeps the pid `$!` gave us — but only +# AFTER setsid() has run. Reading the pgid right after `&` is a race, and +# losing it returns the SCRIPT'S OWN pgid. Cleanup would then `kill` the +# test script's process group: the same self-kill this file already warns +# about for `pkill -f firecracker`, arrived at from the other direction. +# - caller IS a group leader (sourced from an interactive shell): setsid +# forks, `$!` is the short-lived wrapper, and the pid we captured exits at +# once — so the health check below would declare the manager dead, and a +# pgid derived from it would be wrong. +# +# Inside the wrapper, `$$` is the session leader in BOTH cases, so it is the one +# value that is always right. It writes that, then execs the manager, which +# inherits the pid and stays the leader. +setsid bash -c 'echo $$ >"$1"; shift; exec "$@"' _ "${MANAGER_PGID_FILE}" \ + env \ RUNTIME_BACKEND=firecracker \ LANTERN_RUNTIME_BACKEND=firecracker \ LISTEN_ADDR="${MANAGER_LISTEN_ADDR}" \ @@ -228,13 +260,18 @@ setsid env \ LANTERN_VM_SIGNING_CA_KEY="${WORK}/ca.key" \ "${SECRET_ENV_KEY}=${SECRET_VALUE}" \ "${MANAGER_BIN}" >"${MANAGER_LOG}" 2>&1 & -MANAGER_PID=$! -# Read the group id back rather than assuming it equals MANAGER_PID: `setsid` -# execs in place when the caller is not already a group leader, but forks when -# it is, and only one of those keeps the pid we captured. The group id is what -# cleanup signals, so derive it instead of inferring it. -MANAGER_PGID="$(ps -o pgid= -p "${MANAGER_PID}" 2>/dev/null | tr -d ' ')" -: "${MANAGER_PGID:=${MANAGER_PID}}" +# Wait for the wrapper to publish its pid — this is a handshake, not a guess, so +# there is no race left to lose. +for _ in $(seq 1 50); do + [ -s "${MANAGER_PGID_FILE}" ] && break + sleep 0.1 +done +MANAGER_PGID="$(tr -dc '0-9' <"${MANAGER_PGID_FILE}" 2>/dev/null)" +[ -n "${MANAGER_PGID}" ] || fail "manager wrapper never published its process group id" +# The wrapper exec'd into the manager, so the leader's pid IS the manager's pid. +# Use it for liveness rather than `$!`, which is the setsid wrapper when setsid +# forks and would look dead immediately. +MANAGER_PID="${MANAGER_PGID}" # Wait for the gRPC port to accept connections. The manager has no gRPC # reflection, so probe the raw TCP port (port_open is defined near cleanup, From 59fcdbe541d30ffac3c77caef4e752084bfffbce Mon Sep 17 00:00:00 2001 From: Shekhar Mudarapu Date: Fri, 31 Jul 2026 16:41:01 -0400 Subject: [PATCH 3/4] fix(microvm): stop the VM through the manager before killing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second round of auditor findings on this PR, both correct. 1. Killing the manager's process group never reaped its VMs. The backend launches each firecracker with `pre_exec(libc::setsid())` (backends/firecracker.rs), so every VM leaves the manager's session before exec and survives a group kill. A run failing after Spawn but before the explicit Stop left a live VM behind — and because the manager itself DID die, the port check came back clean and the run reported a tidy teardown while the VM leaked. That is the same shape of false green this file keeps producing: the check passed because it was measuring the wrong thing. My own verification missed it for the same reason. The forced-failure case I tested panics the guest, and a panicking VM exits on its own, so firecracker was 0 for a reason that had nothing to do with the cleanup being correct. Cleanup now issues Stop through the manager FIRST, while it is still alive. That is also the only path that releases the VM's TAP, so it fixes the tap orphan the previous round left behind rather than deferring it to the next run's reconciliation. A uuid-scoped backstop (matching this run's vm_id in the api socket path) covers a VM that outlives the manager anyway — deliberately not `pkill -f firecracker`, which matches the usual dev host's Lima VM name and this script's own command line. 2. `MANAGER_PGID` was only assigned after the handshake, so a handshake that timed out while the manager was running leaked it — a regression against the old `MANAGER_PID=$!`, which covered exactly that window. The launch pid is now recorded before the handshake and used as the fallback, and cleanup re-reads the pgid file in case it was published late. Verified on real KVM by fault injection — a synthetic failure after a HEALTHY boot, which is the case that leaks (the panicking guest hid it): previous commit -> firecracker 1 LEAKED, tap 1, port free (false green) this commit -> firecracker 0, tap 0, port free Plus the three paths from the previous round still clean: job control on and off, and the forced-failure case. --- infra/firecracker/integration-test.sh | 54 +++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/infra/firecracker/integration-test.sh b/infra/firecracker/integration-test.sh index 2ed0ea91..b3e5de03 100755 --- a/infra/firecracker/integration-test.sh +++ b/infra/firecracker/integration-test.sh @@ -120,6 +120,34 @@ port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; # host — a Lima VM named `firecracker-dev` — the pattern also matches the # caller's own shell command line and kills the session running this script. cleanup() { + # 1. Stop the VM through the manager FIRST, while the manager is still alive. + # + # Killing the manager's process group does NOT reap its VMs: the backend + # launches each firecracker with `pre_exec(libc::setsid())` (see + # services/runtime-manager/src/backends/firecracker.rs), so every VM leaves + # the manager's session before exec and survives a group kill. A run that + # failed after Spawn but before the explicit Stop would leave a live VM — and + # since the manager itself does die, the port check below would come back + # clean and report success while the VM leaked. + # + # Stop is also the only path that releases the VM's TAP, so this is a clean + # shutdown rather than a kill. Best-effort: before the certs exist, or with + # the manager already gone, grpcurl simply fails and the kill below covers it. + if [ -n "${VM_ID:-}" ] && [ -s "${WORK}/client.crt" ] && port_open; then + grpcurl -cacert "${WORK}/ca.crt" -cert "${WORK}/client.crt" -key "${WORK}/client.key" \ + -servername localhost -import-path "${PROTO_DIR}" -proto "${PROTO_FILE}" \ + -d "$(jq -n --arg id "${VM_ID}" '{vm_id:$id, reason:"integration-test cleanup"}')" \ + "${MANAGER_ADDR}" lantern.v1.RuntimeManager/Stop >/dev/null 2>&1 || true + fi + + # The pgid may have been published after the handshake loop gave up, so read + # the file again rather than trusting only the variable. Without this, a + # handshake that times out while the manager IS running leaks it — the very + # thing `MANAGER_PID=$!` used to cover. + if [ -z "${MANAGER_PGID:-}" ] && [ -s "${MANAGER_PGID_FILE:-/nonexistent}" ]; then + MANAGER_PGID="$(tr -dc '0-9' <"${MANAGER_PGID_FILE}" 2>/dev/null)" + fi + # Never signal our own process group, whatever went wrong upstream: that would # kill this script (and, under CI, the runner step) instead of the manager. # The handshake below should make this unreachable; it is here because the @@ -137,6 +165,27 @@ cleanup() { sleep 0.2 done kill -KILL "-${MANAGER_PGID}" 2>/dev/null || true + elif [ -n "${MANAGER_LAUNCH_PID:-}" ]; then + # No usable group id — the wrapper never published one. Fall back to the pid + # `&` gave us, which is what the pre-handshake code always used. It is the + # manager itself when setsid exec'd, and a already-exited wrapper when setsid + # forked, so this is safe either way and strictly better than leaking. + kill -TERM "${MANAGER_LAUNCH_PID}" 2>/dev/null || true + sleep 1 + kill -KILL "${MANAGER_LAUNCH_PID}" 2>/dev/null || true + fi + + # Backstop for any VM that outlived the manager (each firecracker setsid()s + # into its own session, so nothing above reaches it). Matched on THIS run's + # vm_id in the api socket path — a uuid, so it cannot match another run's VM, + # an unrelated hypervisor, or this script's own command line. That last part + # matters: a broad `pkill -f firecracker` also matches the usual dev host's + # Lima VM name and the shell running this script. + if [ -n "${VM_ID:-}" ]; then + for pid in $(pgrep -f "/run/firecracker/${VM_ID}.sock" 2>/dev/null); do + [ "${pid}" = "$$" ] && continue + kill -KILL "${pid}" 2>/dev/null || true + done fi # Confirm the port came back. If it did not, say so HERE — the next run would @@ -260,6 +309,11 @@ setsid bash -c 'echo $$ >"$1"; shift; exec "$@"' _ "${MANAGER_PGID_FILE}" \ LANTERN_VM_SIGNING_CA_KEY="${WORK}/ca.key" \ "${SECRET_ENV_KEY}=${SECRET_VALUE}" \ "${MANAGER_BIN}" >"${MANAGER_LOG}" 2>&1 & +# Recorded BEFORE the handshake so an early failure still has something to kill: +# every `fail` from here on runs cleanup, and until the pgid is published this is +# the only handle on the manager. +MANAGER_LAUNCH_PID=$! + # Wait for the wrapper to publish its pid — this is a handshake, not a guess, so # there is no race left to lose. for _ in $(seq 1 50); do From 528df0a476c34ef32b1f66cea1ce37ce269ba050 Mon Sep 17 00:00:00 2001 From: Shekhar Mudarapu Date: Fri, 31 Jul 2026 16:47:21 -0400 Subject: [PATCH 4/4] fix(microvm): make the VM backstop jailer-aware; kill with -- MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both non-blocking findings from the third audit round. The backstop matched `/run/firecracker/.sock`, which is the socket path only in the default configuration. Under jailer mode (FIRECRACKER_JAILER_PATH) the socket lives inside the jail root instead, so the match would miss and the VM would still leak — the exact case the backstop exists for. It now matches the vm_id anywhere in the argv of a process actually named `firecracker`, which covers both layouts. Scope is unchanged: the id is a uuid minted for this run, and restricting to `pgrep -x firecracker` means it cannot match another run, an unrelated hypervisor, or this script. Group kills pass `--` so a negative pgid can never be parsed as an option by a kill implementation that reads it that way. Verified on real KVM: fault injection after a healthy boot still leaves manager 0, firecracker 0, taps 0, port free; and a normal run is 5/5 with a clean host. --- infra/firecracker/integration-test.sh | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/infra/firecracker/integration-test.sh b/infra/firecracker/integration-test.sh index b3e5de03..df950758 100755 --- a/infra/firecracker/integration-test.sh +++ b/infra/firecracker/integration-test.sh @@ -158,13 +158,13 @@ cleanup() { MANAGER_PGID="" fi if [ -n "${MANAGER_PGID:-}" ]; then - kill -TERM "-${MANAGER_PGID}" 2>/dev/null || true + kill -TERM -- "-${MANAGER_PGID}" 2>/dev/null || true # Give a graceful shutdown a bounded chance (~6s), then stop asking. for _ in $(seq 1 30); do port_open || break sleep 0.2 done - kill -KILL "-${MANAGER_PGID}" 2>/dev/null || true + kill -KILL -- "-${MANAGER_PGID}" 2>/dev/null || true elif [ -n "${MANAGER_LAUNCH_PID:-}" ]; then # No usable group id — the wrapper never published one. Fall back to the pid # `&` gave us, which is what the pre-handshake code always used. It is the @@ -181,10 +181,18 @@ cleanup() { # an unrelated hypervisor, or this script's own command line. That last part # matters: a broad `pkill -f firecracker` also matches the usual dev host's # Lima VM name and the shell running this script. + # Matched on the vm_id ANYWHERE in a firecracker process's argv, not on the + # bare `/run/firecracker/.sock` path: under jailer mode the socket lives + # inside the jail root instead, so a path match would miss it and leak the VM. + # The id is a uuid minted for this run, and only processes actually named + # `firecracker` are considered, so this cannot match another run, an unrelated + # hypervisor, or this script. if [ -n "${VM_ID:-}" ]; then - for pid in $(pgrep -f "/run/firecracker/${VM_ID}.sock" 2>/dev/null); do + for pid in $(pgrep -x firecracker 2>/dev/null); do [ "${pid}" = "$$" ] && continue - kill -KILL "${pid}" 2>/dev/null || true + if tr '\0' ' ' <"/proc/${pid}/cmdline" 2>/dev/null | grep -qF "${VM_ID}"; then + kill -KILL "${pid}" 2>/dev/null || true + fi done fi