Fixes from the Qodo review of the whole codebase (#34); v1.12

[compot-dev]

Fixes for the findings of the first full Qodo review (#35, review-only PR).

#	Finding	Result
1	Renewed leases keep obsolete routes	fixed: routes of each DHCPACK applied while bound
2	Idle clients can exhaust proxy slots	fixed: SOCKS5 handshake deadline
3	UDP socket failure closes stdin	not a bug: r->fd = socket() is -1 on failure before goto fail
4	DHCP can restart after the client exits	fixed: stop hook waits for earlier hooks
5	On-link DHCP routes fail to install	fixed: no via 0.0.0.0
6	Malformed fetch ports	fixed: checked 1..65535
7	Invalid timer settings can spin the client	fixed: negatives rejected, backoff overflow
8	Partial frames evade peer timeouts	fixed: only complete frames refresh last_rx
9	QEMU test failures pass CI	fixed: exit status kept
10	Toolchains downloaded unverified	fixed: pinned SHA256 (Bootlin, OpenWrt), GitHub digest (rolling OpenIPC asset)
11	First-frame self-test can pass incorrectly	fixed
12	Init stop can kill an unrelated process	fixed: -x "$DAEMON"
13	Mistyped run time runs forever	fixed: --seconds checked

Tested: unit tests (native and ASan/UBSan, 111 + 63 checks), two new tests fail on the old client and pass now; ci/build-musl.sh for arm32, mipsel and arm64 from an empty cache (downloads verified). The bench acceptance runs in CI.

Refs #34

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix DHCP renewals, tunnel timeouts, and build verification

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Replace stale DHCP routes on renewal and install on-link routes correctly.
• Harden tunnel, proxy, CLI, and shutdown behavior against invalid input and stalled peers.
• Verify toolchain downloads and QEMU results; strengthen timeout and first-frame tests.

Diagram

graph TD
  CI["CI checks"] --> Client["Tunnel client"] --> Stack["lwIP stack"] --> Routes["DHCP routes"]
  CI --> CLI["CLI lifecycle"] --> Hooks["TAP hooks"] --> OSRoutes["OS routes"]
  Stack --> Proxy["SOCKS proxy"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Handle renewals directly in the DHCP ACK path

• ➕ Applies routes at the exact lease transition without checking after input and timer processing.
• ➖ Requires a deeper integration with lwIP's DHCP internals and may increase maintenance across lwIP updates.

2. Pin an immutable OpenIPC toolchain release

• ➕ Makes the ARM toolchain reproducible and independently reviewable by digest.
• ➖ Depends on an immutable release being available and requires explicit updates when the toolchain changes.

Recommendation: Keep the localized renewal check rather than modifying lwIP internals. Pinned Bootlin and OpenWrt hashes are appropriate; the rolling OpenIPC asset's GitHub-reported digest verifies the download against GitHub metadata but does not provide an independently pinned toolchain identity.

Files changed (11) +200 / -31

Bug fix (6) +86 / -22

vtesse_cli.cValidate CLI values and serialize shutdown hooks +24/-4

Validate CLI values and serialize shutdown hooks

• Rejects malformed or out-of-range fetch ports and nonpositive or malformed --seconds values. Waits for earlier hooks before running stop, then waits for stop to finish.

cli/vtesse_cli.c

proxy.cTime out unfinished SOCKS5 handshakes +7/-1

Time out unfinished SOCKS5 handshakes

• Applies the connect timeout when a SOCKS5 client is accepted and includes handshake connections in timer scheduling and expiration, freeing slots held by idle clients.

netcore/proxy.c

stack.cReplace DHCP routes after lease renewals +28/-10

Replace DHCP routes after lease renewals

• Tracks the DHCP transaction whose routes are active and applies each newly bound transaction's classless routes, including removal when the ACK has none. Checks after received packets and lwIP timer processing because an unchanged address does not trigger the status callback.

netcore/stack.c

S99vtesseMatch the daemon executable during stop and status +3/-2

Match the daemon executable during stop and status

• Requires the PID-file process to match the configured daemon binary before stop or status treats it as vtesse.

scripts/S99vtesse

vtesse-udhcpc.scriptHandle on-link classless routes +10/-2

Handle on-link classless routes

• Adds and removes routes with gateway 0.0.0.0 as device routes, without an invalid via clause.

scripts/vtesse-udhcpc.script

vtun_client.cHarden peer liveness and reconnect arithmetic +14/-3

Harden peer liveness and reconnect arithmetic

• Refreshes peer activity only after a complete frame, rejects negative tuning values, and caps reconnect backoff before doubling can overflow.

vtun/vtun_client.c

Tests (2) +62 / -1

selftest.pyCheck the actual first session frame +19/-1

Check the actual first session frame

• Adds a helper that reads the first frame header and verifies it is ECHO_REQ, rather than accepting an ECHO_REQ that appears later.

bench/selftest.py

test_client.cCover partial-frame timeouts and negative settings +43/-0

Cover partial-frame timeouts and negative settings

• Adds a peer that trickles an unfinished frame and checks that the client still reports a keepalive timeout. Also checks rejection of negative keepalive and reconnect settings.

tests/test_client.c

Documentation (1) +21 / -0

CHANGELOG.mdDocument the 1.12 fixes +21/-0

Document the 1.12 fixes

• Records the DHCP, timeout, validation, lifecycle, route, and CI fixes in the 1.12 release notes.

CHANGELOG.md

Other (2) +31 / -8

build-musl.shVerify toolchains and preserve QEMU failures +30/-7

Verify toolchains and preserve QEMU failures

• Checks downloaded archives against pinned SHA256 values or the OpenIPC GitHub asset digest before extraction. Captures each QEMU test's exit status so failures stop the build.

ci/build-musl.sh

version.hBump the release version to 1.12 +1/-1

Bump the release version to 1.12

• Changes the version reported by binaries from 1.11 to 1.12.

include/vtesse/version.h

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. DHCP renewals retain obsolete routes ✓ Resolved

Description

check_renewal() applies routes only when d->xid differs from st.applied_xid, but lwIP reuses
the existing transaction ID for renewal requests. When a renewal ACK changes or removes classless
routes without changing the lease address, the new routes are recorded but never applied.

Code

netcore/stack.c[R230-231]

+    if (st.bound && !st.cfg.static_ip && d && d->state == DHCP_STATE_BOUND && d->xid != st.applied_xid)
+        apply_dhcp_routes(d);

Evidence

The option hook records ACK routes, while the new guard requires a different XID before applying
them. lwIP's DHCP message construction explicitly preserves the XID for DHCPREQUEST outside the
rebooting state, including normal renewals.

netcore/stack.c[121-137]
netcore/stack.c[211-232]
third_party/lwip/src/core/ipv4/dhcp.c[1918-1934]
third_party/lwip/src/core/ipv4/dhcp.c[1170-1177]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Ordinary DHCP renewals reuse the existing transaction ID, so comparing XIDs cannot detect that a new ACK has made different routes current.
## Fix Focus Areas
- netcore/stack.c[211-232]
- netcore/stack.c[121-137]
## Recommended Fix
Track each accepted DHCPACK or completed bind using a signal that changes on every accepted renewal, including ACKs without classless routes. Apply or clear routes after that ACK becomes the current lease, without relying on a changed XID.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Renewals can restore obsolete routes ✓ Resolved

Description

check_renewal() leaves st.pending intact when an ACK arrives while DHCP remains BOUND, even
though lwIP ignores that ACK after parsing its route options. If a duplicate ACK contains classless
routes and a later accepted renewal with the same transaction ID contains none,
apply_dhcp_routes() installs the duplicate ACK's routes instead of removing them.

Code

netcore/stack.c[R241-242]

+    if (prev != DHCP_STATE_BOUND || d->lease_used < st.lease_used)
+        apply_dhcp_routes(d);

Evidence

The option hook records routes before lwIP's ACK state check; an ACK in BOUND is ignored, but the
new renewal check does not clear its pending routes. A later accepted renewal can reuse the
transaction ID, which is the only association checked before pending routes are applied.

netcore/stack.c[103-129]
netcore/stack.c[212-243]
third_party/lwip/src/core/ipv4/dhcp.c[1819-1854]
third_party/lwip/src/core/ipv4/dhcp.c[1093-1095]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An ACK parsed but ignored while DHCP is already bound can leave pending classless routes for a later renewal that contains no routes.
## Fix Focus Areas
- netcore/stack.c[103-129]
- netcore/stack.c[212-244]
## Recommended Fix
Associate pending routes with an ACK that lwIP actually accepts, and clear pending options from ignored ACKs before a subsequent renewal can use them. Add a regression test with a duplicate ACK carrying routes followed by a renewal ACK without routes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. arm32 builds fail in the cross build container ✓ Resolved

Description

The OpenIPC digest lookup in ci/build-musl.sh pipes GitHub release JSON through jq, but
ci/Dockerfile.cross, the documented build environment, does not install it. On an arm32 build from
an empty toolchain cache, including VTUND=1 … arm32, the missing command fails the SHA
assignment under set -e, stopping the build before extraction, cleanup, or the TOOLCHAIN_SHA256
hint.

Code

ci/build-musl.sh[79]

+        SHA=$(printf '%s' "$REL" | jq -r --arg n "${URL##*/}" '.assets[] | select(.name == $n) | .digest // "" | sub("^sha256:"; "")')

Evidence

The arm32 target selects the GitHub digest path, where ci/build-musl.sh invokes jq
unconditionally when SHA=github. ci/Dockerfile.cross supplies the environment for that script
but installs only make, curl, ca-certificates, xz-utils, file, and qemu-user, so the digest lookup
cannot run in the image.

ci/Dockerfile.cross[1-6]
ci/build-musl.sh[36-37]
ci/build-musl.sh[73-83]
ci/Dockerfile.cross[1-5]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ci/build-musl.sh` requires `jq` for the arm32 toolchain digest lookup, but the cross-build image does not install it. With an empty toolchain cache, `set -e` stops the build before it can provide the cleanup and `TOOLCHAIN_SHA256` guidance.
## Fix Focus Areas
- ci/Dockerfile.cross[1-5]
- ci/build-musl.sh[73-80]
## Recommended Fix
Add `jq` to the `apt-get install` list in `ci/Dockerfile.cross`. Before the API call in `ci/build-musl.sh`, check for it with `command -v jq >/dev/null || { echo "jq is required to verify $URL (or set TOOLCHAIN_SHA256)" >&2; rm -f "$CACHE/toolchain.tar"; exit 1; }`. Verify an arm32 build from an empty toolchain cache inside the image.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. A DHCP client can outlive shutdown after a slow up hook ✓ Resolved

Description

When a hook outlives HOOK_WAIT_MS, wait_hooks() sends kill(hook_pids[i], SIGTERM) only to the
/bin/sh PID that run_hook() forked; the hook runs in vtesse-cli's process group, and nothing
creates a group for it. If shutdown happens while the up hook is still in the foreground of
udhcpc -b (it waits for a first lease before going to the background), only the shell dies, so the
DHCP client can go to the background and keep running after the stop hook and the tap teardown,
which is the outcome this change was meant to prevent.

Code

cli/vtesse_cli.c[R553-557]

+                printf("[%8.3f] hook: pid %d still running, terminated\n", elapsed(t_start),
+                       (int)hook_pids[i]);
+                kill(hook_pids[i], SIGTERM);
+                while (waitpid(hook_pids[i], NULL, 0) < 0 && errno == EINTR)
+                    ;

Evidence

run_hook forks and execs sh with no setpgid/setsid, and wait_hooks signals only that single PID. The
up hook runs udhcpc -b as a child of that shell, so SIGTERM does not reach it.

cli/vtesse_cli.c[543-565]
cli/vtesse_cli.c[585-598]
scripts/vtesse-hook.sh[26-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
On timeout, wait_hooks() sends SIGTERM only to the hook's shell, not to the commands it started, such as udhcpc running in the foreground.
## Fix Focus Areas
- cli/vtesse_cli.c[553-557]
- cli/vtesse_cli.c[586-588]
## Recommended Fix
In the child branch of run_hook(), call `setpgid(0, 0)` before execl (and `setpgid(pid, pid)` in the parent to avoid a race). In wait_hooks(), use `kill(-hook_pids[i], SIGTERM)` so that the shell and its children are terminated, then reap the shell. Also consider bounding the blocking waitpid after SIGTERM (e.g. send SIGKILL after a short grace period).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View review recommended (4)

5. Interrupted waits leave hooks running ✓ Resolved

Description

wait_hooks() treats every nonzero waitpid(..., WNOHANG) result as completion, and run_hook()
likewise treats it as a free slot, without distinguishing -1 with errno == EINTR. If a signal
interrupts either check while a hook is active, its PID can be cleared or overwritten, allowing the
stop hook and interface cleanup to proceed without waiting for that earlier hook.

Code

cli/vtesse_cli.c[R539-542]

+            pid_t r = waitpid(hook_pids[i], NULL, WNOHANG);
+            if (r != 0) {
+                hook_pids[i] = 0;               /* finished, or already reaped */
+            } else if (nc_now_ms() >= deadline) {

Evidence

The installed signal handler can run during shutdown; both new PID checks classify any nonzero wait
result as finished, and shutdown subsequently runs the stop hook and destroys the interface.

cli/vtesse_cli.c[103-109]
cli/vtesse_cli.c[537-547]
cli/vtesse_cli.c[565-568]
cli/vtesse_cli.c[950-954]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An interrupted `waitpid()` can be mistaken for a completed hook, causing shutdown to lose track of a still-running child.
## Fix Focus Areas
- cli/vtesse_cli.c[537-547]
- cli/vtesse_cli.c[565-568]
## Recommended Fix
Handle `waitpid()` results separately in both locations: retry when the result is `-1` with `errno == EINTR`, treat `0` as still running, and clear or reuse a PID only after a confirmed exit or `ECHILD`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. A ninth hook can outlive shutdown ✓ Resolved

Description

run_hook() records a child only if one of the eight hook_pids entries is free, and silently
discards its PID when they are all occupied. Repeated reconnect and down events can start
overlapping hooks; if a ninth is still running at shutdown, wait_hooks() cannot wait for or
terminate it before the stop hook and interface cleanup.

Code

cli/vtesse_cli.c[R578-582]

+        for (i = 0; i < sizeof(hook_pids) / sizeof(hook_pids[0]); i++)
+            if (hook_pids[i] <= 0 || waitpid(hook_pids[i], NULL, WNOHANG) != 0) {
+                hook_pids[i] = pid;
+                break;
+            }

Evidence

Reconnect and down callbacks can repeatedly start asynchronous hooks. The new insertion loop has no
full-table branch, and both shutdown waits inspect only the eight stored PIDs.

cli/vtesse_cli.c[523-553]
cli/vtesse_cli.c[557-612]
cli/vtesse_cli.c[944-950]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The fixed-size hook PID table silently loses a child when more than eight hooks overlap, leaving shutdown unable to wait for that hook.
## Fix Focus Areas
- cli/vtesse_cli.c[523-553]
- cli/vtesse_cli.c[573-583]
## Recommended Fix
Use a growable collection of hook PIDs, or explicitly handle a full table without discarding a live child's PID. Ensure the shutdown wait accounts for every hook successfully started.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Routes stay stale after a quick tunnel reconnect ✓ Resolved

Description

check_renewal() detects a new DHCPACK only when lease_used decreases, but dhcp_bind() resets
that counter to zero and the 60-second coarse timer is the only thing that increments it. If the
tunnel reconnects before the first tick, a reboot ACK can keep the same address while changing or
removing option 121/249 routes; the counter remains zero, status_cb() does not apply the routes,
and the old routes can remain active until a later renewal.

Code

netcore/stack.c[R231-235]

+    if (!st.bound || st.cfg.static_ip || !d || d->state != DHCP_STATE_BOUND)
+        return;
+    if (d->lease_used < st.lease_used)
+        apply_dhcp_routes(d);
+    st.lease_used = d->lease_used;

Evidence

The stack preserves the bound lease across link loss and restarts DHCP on link-up. After the first
bind, apply_dhcp_routes() records st.lease_used as zero; on a quick reconnect, check_renewal()
returns while DHCP is REBOOTING, then dhcp_bind() resets d->lease_used to zero again when the
ACK is accepted. The strict d->lease_used < st.lease_used test is therefore false, and the
bound-address branch of status_cb() does not apply routes because the address has not changed.

third_party/lwip/src/core/ipv4/dhcp.c[1092-1094]
third_party/lwip/src/core/ipv4/dhcp.c[515-521]
netcore/stack.c[287-299]
netcore/stack.c[227-236]
netcore/stack.c[227-247]
netcore/stack.c[285-315]
third_party/lwip/src/core/ipv4/dhcp.c[939-950]
third_party/lwip/src/core/ipv4/dhcp.c[1089-1094]
third_party/lwip/src/core/ipv4/dhcp.c[1833-1855]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`check_renewal()` relies on `lease_used` decreasing to detect a new DHCPACK. A reconnect ACK before the first 60-second coarse timer tick leaves that counter at zero, so changed or removed option 121/249 routes are not applied even though the lease is bound again.
## Fix Focus Areas
- netcore/stack.c[38-38]
- netcore/stack.c[224-236]
- netcore/stack.c[278-281]
## Recommended Fix
Record the DHCP state seen on the previous check, for example in `st.dhcp_state`, and update it on every call, including the early-return path for non-BOUND states. While `st.bound && !static_ip`, call `apply_dhcp_routes(d)` when `d->state == DHCP_STATE_BOUND` and the previously recorded state was not BOUND, including after RENEWING, REBINDING, or REBOOTING; retain the `lease_used` decrease check as a fallback. Ensure the accepted ACK's option 121/249 routes are applied, or cleared when absent. Add a regression test in which the tunnel reconnects before the first coarse timer tick and the server sends different classless routes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Client exit can hang on a slow hook ✓ Resolved

Description

wait_children() calls blocking waitpid(-1, NULL, 0) without a deadline both before and after the
stop hook, after the event loop has returned. An earlier asynchronous hook such as up,
reconnect, or down can remain stuck—including udhcpc -b retrying DHCP while the tap is no
longer serviced—preventing the DHCP cleanup hook from running; restarting signal handlers keep
SIGINT/SIGTERM from interrupting the wait, and S99vtesse restart can start another instance on the
same interface while the first still waits.

Code

cli/vtesse_cli.c[R524-528]

+static void wait_children(void)
+{
+    while (waitpid(-1, NULL, 0) > 0 || errno == EINTR)
+        ;
+}

Evidence

run_hook launches hook shell processes asynchronously, while wait_children loops on blocking
waitpid without a timeout and is called twice after nc_loop_run returns, including before the
shipped stop hook that terminates the DHCP client. The bundled up hook runs udhcpc -b, which
can remain in foreground DISCOVER retries while the tap is no longer pumped; the signal() handlers
restart the wait, and S99vtesse restart removes the PID file and sleeps only one second between
stop and start.

cli/vtesse_cli.c[522-549]
cli/vtesse_cli.c[905-916]
scripts/vtesse-hook.sh[26-35]
scripts/S99vtesse[25-35]
cli/vtesse_cli.c[522-550]
cli/vtesse_cli.c[552-578]
cli/vtesse_cli.c[908-915]
scripts/vtesse-hook.sh[63-75]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`wait_children()` can wait indefinitely for an earlier asynchronous hook after the event loop stops, preventing the `stop` hook from cleaning up DHCP and delaying teardown. The wait also cannot be interrupted by SIGTERM, allowing a restart to begin while the first instance still waits.
## Fix Focus Areas
- cli/vtesse_cli.c[522-528]
- cli/vtesse_cli.c[911-913]
## Recommended Fix
Replace the blocking wait with polling via `waitpid(-1, NULL, WNOHANG)`, a short sleep, and an overall shutdown deadline (for example, five seconds). Give up early if a stop signal arrives; if an earlier hook remains stuck, log which hooks are still running and terminate or otherwise handle them before continuing to the `stop` hook and teardown. Bound the wait for the `stop` hook as well.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. arm32 builds fail when GitHub rate-limits ✓ Resolved

Description

For the rolling OpenIPC asset, the checksum comes from an unauthenticated call to
api.github.com/repos/OpenIPC/firmware/releases/tags/toolchain. If that call is refused (for
example by the 60-per-hour anonymous limit, which shared CI runner IPs hit), jq gets empty input
and SHA ends up empty. The build then stops with "no SHA256 … set TOOLCHAIN_SHA256" and the
toolchain it just downloaded is deleted, so every arm32 build from an empty cache fails until the
limit resets.

Code

ci/build-musl.sh[R73-77]

+    if [ "$SHA" = github ]; then
+        SHA=$(curl -fsSL --retry 3 "https://api.github.com/repos/OpenIPC/firmware/releases/tags/toolchain" |
+              jq -r --arg n "${URL##*/}" '.assets[] | select(.name == $n) | .digest // "" | sub("^sha256:"; "")')
+    fi
+    [ -n "$SHA" ] || { echo "no SHA256 for $URL: set TOOLCHAIN_SHA256" >&2; rm -f "$CACHE/toolchain.tar"; exit 1; }

Evidence

The digest lookup sends no Authorization header, and curl -f fails on a 403 or 429 response. jq
reading empty input prints nothing and still exits 0, so the [ -n "$SHA" ] guard is what stops the
build and removes the archive. This happens on a cold cache, which is the case the PR says it tested
("from an empty cache").

ci/build-musl.sh[69-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The OpenIPC toolchain digest is fetched from the GitHub API without authentication. When the anonymous rate limit is hit, the SHA comes back empty and the build fails.
## Fix Focus Areas
- ci/build-musl.sh[73-76]
## Recommended Fix
When `GITHUB_TOKEN` (or `GH_TOKEN`) is set, add `-H "Authorization: Bearer $GITHUB_TOKEN"` to the curl call, and pass the token in from the workflow. Also print a clear message when the API request itself fails, so it is not confused with a missing digest.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Run-time error message says 0 is allowed ✓ Resolved

Description

The --seconds check rejects anything not > 0, but its error message says "expected a number of
seconds from 0 to 1e9". A user who passes --seconds 0 gets a message that says the value they
typed is in range. Before this change, --seconds 0 meant "no limit".

Code

cli/vtesse_cli.c[R653-654]

+            if (end == arg || *end || !(seconds > 0) || !(seconds <= 1e9)) {
+                fprintf(stderr, "--seconds: expected a number of seconds from 0 to 1e9, got '%s'\n", arg);

Evidence

The condition !(seconds > 0) rejects 0, yet the message describes the range as starting at 0. With
the old atof, 0 gave seconds > 0 ? ... : 0, which means no deadline.

cli/vtesse_cli.c[648-657]
cli/vtesse_cli.c[917-917]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `--seconds` validation rejects 0, but its error text says values from 0 are accepted. Before this change, 0 meant "run until a signal".
## Fix Focus Areas
- cli/vtesse_cli.c[648-657]
## Recommended Fix
Either change the message to "a positive number of seconds up to 1e9", or keep 0 as "no limit" by changing the check to `!(seconds >= 0)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Typing 'inf' for run time is undefined ✓ Resolved

Description

--seconds validation accepts any strtod value greater than 0, including inf and huge values
like 1e30. Later, (uint64_t)(seconds * 1000) converts that out-of-range double to an integer,
which is undefined behaviour, so the run deadline depends on the platform (wraps on x86, saturates
on ARM) even though this change was meant to make the run time strictly checked.

Code

cli/vtesse_cli.c[R616-617]

+            seconds = strtod(arg, &end);
+            if (end == arg || *end || !(seconds > 0)) {   /* "1m" or "-5" must not mean "forever" */

Evidence

Line 617 only rejects NaN, values that are 0 or less, and trailing garbage. Line 881 casts
seconds*1000 to uint64_t without checking the range.

cli/vtesse_cli.c[613-621]
cli/vtesse_cli.c[881-881]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--seconds inf` or `1e30` passes validation, and casting it to uint64_t later is undefined behaviour.
## Fix Focus Areas
- cli/vtesse_cli.c[613-621]
## Recommended Fix
Also reject values that are `!isfinite(seconds)` or larger than a sane limit (for example 1e9 seconds), and print the same error message for them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

2. Client exit can hang on a slow hook 🐞 Bug ☼ Reliability

wait_children() calls blocking waitpid(-1, NULL, 0) without a deadline both before and after the
stop hook, after the event loop has returned. An earlier asynchronous hook such as up,
reconnect, or down can remain stuck—including udhcpc -b retrying DHCP while the tap is no
longer serviced—preventing the DHCP cleanup hook from running; restarting signal handlers keep
SIGINT/SIGTERM from interrupting the wait, and S99vtesse restart can start another instance on the
same interface while the first still waits.

Agent Prompt

## Issue description
`wait_children()` can wait indefinitely for an earlier asynchronous hook after the event loop stops, preventing the `stop` hook from cleaning up DHCP and delaying teardown. The wait also cannot be interrupted by SIGTERM, allowing a restart to begin while the first instance still waits.

## Fix Focus Areas
- cli/vtesse_cli.c[522-528]
- cli/vtesse_cli.c[911-913]

## Recommended Fix
Replace the blocking wait with polling via `waitpid(-1, NULL, WNOHANG)`, a short sleep, and an overall shutdown deadline (for example, five seconds). Give up early if a stop signal arrives; if an earlier hook remains stuck, log which hooks are still running and terminate or otherwise handle them before continuing to the `stop` hook and teardown. Bound the wait for the `stop` hook as well.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[compot-dev]

Qodo round 2 → all three fixed in 177b1ea:

1. Renewal routes: real — lwIP reuses the xid in the renewal DHCPREQUEST. A new ACK is now detected by dhcp->lease_used going back (dhcp_bind() resets it to 0 on every ACK).
2. Hook wait: bounded to 5 s, a hook still running then is terminated and teardown continues. (The wait also tracks only our hook PIDs since 8a97271: as PID 1 in a container the daemonized DHCP client becomes our child.)
3. --seconds inf: rejected, as is anything above 1e9.

/review

[qodo-free-for-open-source-projects]

Qodo round 2 → all three fixed in 177b1ea:

1. Renewal routes: real — lwIP reuses the xid in the renewal DHCPREQUEST. A new ACK is now detected by dhcp->lease_used going back (dhcp_bind() resets it to 0 on every ACK).
2. Hook wait: bounded to 5 s, a hook still running then is terminated and teardown continues. (The wait also tracks only our hook PIDs since 8a97271: as PID 1 in a container the daemonized DHCP client becomes our child.)
3. --seconds inf: rejected, as is anything above 1e9.

/review

I can’t rerun the Qodo review from this thread because the review action isn’t available here. The reported fixes cover finding 1, finding 2, and finding 3; you can trigger /review in a context with the review action enabled, or share the new report for verification.

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 177b1ea

[compot-dev]

Qodo round 3 → fixed in 3b6b6c6:
2. Ninth hook: the free slot is found before fork() (table of 16); with none free the hook is not started and this is logged, so every started hook is waited for (bounded, 5 s).
3. Quick reconnect: a return of dhcp->state to BOUND also applies the ACK's routes; lease_used going back stays as the fallback. Covered by the bench (fault-test: link loss, lease renewal), no unit harness for lwIP DHCP yet.
4. Slow hook: already bounded since 177b1ea (5 s, then SIGTERM).
5. Message: now "a positive number of seconds up to 1e9".

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Qodo round 3 → fixed in 3b6b6c6:
2. Ninth hook: the free slot is found before fork() (table of 16); with none free the hook is not started and this is logged, so every started hook is waited for (bounded, 5 s).
3. Quick reconnect: a return of dhcp->state to BOUND also applies the ACK's routes; lease_used going back stays as the fallback. Covered by the bench (fault-test: link loss, lease renewal), no unit harness for lwIP DHCP yet.
4. Slow hook: already bounded since 177b1ea (5 s, then SIGTERM).
5. Message: now "a positive number of seconds up to 1e9".

Thanks — the update addresses the remaining open findings:

• finding 2: hooks are capped before fork(), so every started hook is tracked and waited for.
• finding 3: DHCP state transitions now detect quick reconnects, with lease_used retained as a fallback.
• finding 4: shutdown waiting is already bounded and terminates lingering hooks.
• finding 5: the error message now correctly requires a positive duration.

The published review still shows these findings as open; they should be marked resolved after a review of commit 3b6b6c6.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 3b6b6c6

[compot-dev]

Qodo round 4 → both fixed in 4536fa1:
2. Interrupted waits: hook_done() retries on EINTR; only an exit (or ECHILD) frees a slot or ends the wait.
6. Rate limit: the digest lookup is authenticated with GITHUB_TOKEN/GH_TOKEN when set (the workflow passes github.token), and a failed API request is reported as such.

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Qodo round 4 → both fixed in 4536fa1:
2. Interrupted waits: hook_done() retries on EINTR; only an exit (or ECHILD) frees a slot or ends the wait.
6. Rate limit: the digest lookup is authenticated with GITHUB_TOKEN/GH_TOKEN when set (the workflow passes github.token), and a failed API request is reported as such.

Thanks—acknowledged. The fixes for finding 2 (interrupted waits) and finding 6 (GitHub API rate-limit handling) are reported in commit 4536fa1. I haven’t independently rerun the review here.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 4536fa1

[compot-dev]

Qodo round 5 → all three fixed in 06bc832:
2. Ignored ACKs: the option hook records routes only in the states where lwIP accepts an ACK (REQUESTING, REBOOTING, RENEWING, REBINDING), so a duplicate ACK while BOUND leaves nothing pending.
3. jq: added to ci/Dockerfile.cross; build-musl.sh checks for it. arm32 built from an empty cache inside the image.
4. Hook children: hooks run in their own process group (setpgid in child and parent); a stuck hook is ended with kill(-pgid, SIGTERM), then SIGKILL after 1 s.

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Qodo round 5 → all three fixed in 06bc832:
2. Ignored ACKs: the option hook records routes only in the states where lwIP accepts an ACK (REQUESTING, REBOOTING, RENEWING, REBINDING), so a duplicate ACK while BOUND leaves nothing pending.
3. jq: added to ci/Dockerfile.cross; build-musl.sh checks for it. arm32 built from an empty cache inside the image.
4. Hook children: hooks run in their own process group (setpgid in child and parent); a stuck hook is ended with kill(-pgid, SIGTERM), then SIGKILL after 1 s.

Thanks—I've noted commit 06bc832 as addressing findings 2, 3, and 4: ignored DHCP ACKs, missing jq, and hook process-group cleanup.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 06bc832