Harness fixes: ready-file budget and run scoping - #1256
Merged
Merged
Conversation
The sftp, scp and get-put scripts wait for the echoserver to publish its port before connecting. Two seconds is not enough for a libtool re-exec, the dynamic linker and the sample-key parse with a dozen sibling test jobs on the machine, so a parallel make check failed them at their first scenario. Ten seconds matches what sshclient.test already allows. - raise the three wait loops from 20 to 100 iterations of 0.1 seconds - test -s, not -e, after the loop in scp.test and get-put.test: the ready file is created empty and the port written afterward, so -e can take a file caught mid-write and yield an empty port
Two runs of the suite on one machine no longer collide. Each run takes a block of ports, identifies the daemons it starts by the PID file in their generated config, and at exit stops only those. wolfSSHd writes that file after it has finished daemonizing, so the pid no longer has to be guessed from what appeared in the process table. - take a free block of ports per run, replacing the fixed 22222 and the constants the private daemons used - honour --port for a local run, and pass the port to create_sshd_config.sh rather than baking it into the four configs - read the daemon pid from a PidFile placed at the top of the generated config, ahead of any Match block, where it is applied - stop only the daemons recorded during this run, not every wolfsshd on the machine
The OpenSSH certificate test kills its daemon with a pkill pattern. That pattern now carries $WORK, the mktemp directory this invocation created, so it names one run's daemon. "sshd_config_ossh" appears on every concurrent run's command line, so the old pattern took their daemons down too.
sshd_term_size_test.sh drives the client through a tmux session named for the port it was given. The name used to be the constant "test", which is shared across everything the user runs, so two concurrent runs fought over one session and each EXIT trap killed the other's.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved port allocation, certificate isolation, and PID/cleanup safety issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves wolfsshd test reliability for slower startup and concurrent runs.
Changes:
- Extends readiness waits and validates non-empty ready files.
- Allocates per-run ports and supports local
--port. - Scopes daemon, tmux, and certificate cleanup.
File summaries
| File | Summary |
|---|---|
scripts/sftp.test |
Extends readiness polling. |
scripts/scp.test |
Extends and validates readiness. |
scripts/get-put.test |
Extends and validates readiness. |
apps/wolfsshd/test/start_sshd.sh |
Adds PID tracking and scoped teardown. |
apps/wolfsshd/test/sshd_term_size_test.sh |
Names tmux sessions per run. |
apps/wolfsshd/test/sshd_privdrop_fail_test.sh |
Derives a private daemon port. |
apps/wolfsshd/test/sshd_ossh_cert_test.sh |
Scopes certificate daemon cleanup. |
apps/wolfsshd/test/run_all_sshd_tests.sh |
Allocates port blocks and handles local ports. |
apps/wolfsshd/test/create_sshd_config.sh |
Parameterizes generated config ports. |
Review details
Suppressed comments (2)
apps/wolfsshd/test/run_all_sshd_tests.sh:127
- The requested local
--portis not excluded from the block selected above. For example,--port 22303can select base 22300, then the shared daemon uses +3 whilerun_hostkey_perm_checkalso starts on +3, causing a valid local run to fail with EADDRINUSE. Choose a block that excludes the requested port before assigning the private offsets.
LOCAL_PORT="${TEST_PORT:-$PORT_BASE}"
STRICTMODES_PORT=$((PORT_BASE + 1))
UPN_PORT=$((PORT_BASE + 2))
HOSTKEY_PERM_PORT=$((PORT_BASE + 3))
apps/wolfsshd/test/sshd_ossh_cert_test.sh:56
- The daemon kill is now scoped to
$WORK, but the certificates used by this test are still global: both runs invokekeys/renew-ossh-certs.shand overwrite the same$LOGINUSER-ossh-*-cert.pubfiles. In particular, the force-command certificate embeds each run'sMARKERDIR, so one run can authenticate with the other run's certificate and fail its marker assertion. Generate certificates under the per-run work directory (or serialize renewal) and use those paths for the clients.
trap 'pkill -f "wolfsshd .*$WORK" 2>/dev/null; rm -rf "$WORK"' EXIT
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The registry now holds only daemons that are still running, and it is cleaned up however the run ends. Both private-daemon ports come from the runner, so they stay inside the block it probed even when --port moves the shared daemon off it. - drop a pid from the registry once stop_wolfsshd has stopped it, so the end-of-run sweep cannot reach a pid since recycled by another run - run the sweep and the registry cleanup from an EXIT trap: every early exit used to skip them and leave the file in /tmp - export WOLFSSHD_PRIVDROP_PORT rather than re-deriving the offset in sshd_privdrop_fail_test.sh, and correct that script's usage message - quote the arguments to create_sshd_config.sh: an empty USER shifted the port into $1, silently leaving the daemon on 22222 - name the three tests that still read the whole process table, which are what keeps two concurrent runs from being fully independent
start_wolfsshd returns once the daemon accepts connections, not once it has written its pid: wolfSSHd saves the PID file just before tcp_listen(), so a caller connecting straight away could be refused while the daemon's log showed no connection at all. A run also claims its port block rather than only probing it. - wait for the daemon's own "Listening on port" line, matched on its pid so a previous daemon's line in the appended log cannot satisfy it - take a block by creating its lock directory, which mkdir makes atomic, and release it in the teardown; probing alone let two runners pick the same block, and a claim whose owner is gone is treated as stale - check that a pid from the PID file is still a wolfsshd, since kill -0 answers only whether some process owns the number - fail the OpenSSH cert test when mktemp gives it no work dir: an empty one reduced its teardown pattern to every wolfsshd present
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1256
No scan targets match the changed files in this PR. Review skipped.
philljj
self-requested a review
September 16, 2026 17:18
philljj
requested changes
Sep 16, 2026
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
Looks good, just a few suggestions / questions on some loops
A run's port block is leased through a directory named for the block and for the pid holding it, in a host-wide pool under /tmp. A run creates and removes only its own lease, and ps decides whether an existing one is still held. The allocator moves to port_lease.sh. - a pool under $TMPDIR was not host wide: TMPDIR is per user on macOS and sudo's env_reset drops it, so two runs kept private pools for one set of ports - a stale lease cannot be reclaimed in place: whatever does the removing is authorized by an earlier read of the owner, so a second runner displaces the live claim the first just made - ps -p rather than kill -0, which reports failure both for a pid that is gone and for one the caller may not signal -- opposite answers when a non-root run reads a lease held by a live root run, as in CI - the range starts at 28000, clear of the 22000-27999 that scripts/fwd-bulk.test picks from and fails outright when taken - a block holding the port passed to --port is skipped, so the shared daemon and a private one cannot be assigned the same port
sshd_port_lease_test.sh forks processes that contend for real leases and checks that a block is never held by two at once. It is the first entry in test_cases, so a broken allocator is named there rather than surfacing as a bind collision in an unrelated test much later. - covers a contended stale block, parallel allocation, a live lease, a live owner this run cannot signal, release and reuse, and --port - a winner holds its lease until the parent releases it, so a straggler cannot take a block already counted and read as a second winner - the cross-user case drops to $SUDO_USER, the one arrangement that tells "owner gone" from "owner not mine" - contends over 29000-29511, below the ephemeral range and away from the suite's daemon, with the scan pinned so --port has a block to skip
start_wolfsshd now records whether the daemon's listening line appeared in the ten seconds it waits for it. Without one it kills the daemon and clears PID, which puts the start through the empty-PID check every caller already has, so the run stops and names the daemon. - the pid reaches the run registry before that check, so a daemon that came up but never bound is still reaped by the runner's teardown
stop_wolfsshd escalates to SIGKILL when SIGTERM has left the daemon running after five seconds, and drops the pid from the run registry only once it is gone. wolfsshd_alive answers whether the pid is still a wolfsshd, which is what each of those steps turns on. - the entry was dropped unconditionally, so a stuck daemon lost its last handle and held its port for the rest of the run - kill -0 says only that some process holds the pid, and this sends SIGKILL as root, so a recycled pid was killed outright - a recycled pid now counts as stopped and leaves the registry
Dropping a stopped daemon from the run registry now installs the rewritten file only when grep either kept lines or matched none. Any other status is grep failing, and the empty file it leaves behind would become the registry, losing every other daemon's pid. - that registry is what the end-of-run sweep works from, so blanking it strands whatever else the run started
The exit teardown stops the shared daemon before sweeping the registry. stop_wolfsshd is what removes the per-daemon temp directory, which holds the rewritten config and root-owned copies of the trust anchors, and it is idempotent, so a run that stopped already is unaffected. - a run that exits early, on a daemon that will not start or on a failed test, reaches no stop of its own and left the directory in /tmp with a copy of the host key in it
sshd_ossh_cert_test.sh generates its OpenSSH certificates into its own work directory, through the OSSH_CERT_DIR that renew-ossh-certs.sh now reads. Only the keypairs stay shared, and those are committed. - the names carry just the login user, so two runs in one checkout reissued each other's certificates and the force-command case failed on a marker written to the other run's directory
sshd_large_sftp_test.sh puts its pid in both the local and the remote file name, and removes them from an EXIT trap. - the remote name was fixed in the daemon user's home, one directory for the whole host, so two runs uploaded to the same path and each compared its own file against the other's upload - 4.4G apiece, and the transfer runs under "set -e", so a failed run left both behind
philljj
approved these changes
Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two concurrent runs of the wolfsshd suite could not both finish: a fixed port, a host-wide teardown, and pid detection by process-table diff. Three test scripts also gave the echoserver only two seconds to publish its port.
sftp.test,scp.testandget-put.test, matchingsshclient.test, and test-safter the loop so a file caught between creation and the port write cannot pass as ready--portwork for a local runPidFilein its generated config, so a concurrent start is not mistaken for this onewolfsshdon the machine