Deep Review: 20260512-213934-pr-4967

	}
	// RFC4253 §4.2: an SSH server sends an identification string ending in CR
	// LF (or just LF historically) before the version exchange. If the proxied
	// connection inside the guest has no live peer, the host side will close
	// and ReadString returns EOF.
	banner, err := bufio.NewReader(conn).ReadString('\n')
	if err != nil {
		return fmt.Errorf("read SSH banner from %s: %w", addr, err)
	}
	if !strings.HasPrefix(banner, "SSH-2.0-") && !strings.HasPrefix(banner, "SSH-1.99-") {
		return fmt.Errorf("unexpected banner from %s: %q", addr, strings.TrimRight(banner, "\r\n"))
	}
	return nil
}

			ConfigFile:     sshConfig.ConfigFile,
			Persist:        false,
			AdditionalArgs: sshutil.DisableControlMasterOptsFromSSHArgs(sshConfig.AdditionalArgs),
		}
	}
	stdout, stderr, err := ssh.ExecuteScript(a.instSSHAddress, a.sshLocalPort, sshConfig, script, r.description)
	logrus.Debugf("stdout=%q, stderr=%q, err=%v", stdout, stderr, err)
	if err != nil {
		return fmt.Errorf("stdout=%q, stderr=%q: %w", stdout, stderr, err)
	}
	return nil

-	banner, err := bufio.NewReader(conn).ReadString('\n')
-	if err != nil {
-		return fmt.Errorf("read SSH banner from %s: %w", addr, err)
-	}
-	if !strings.HasPrefix(banner, "SSH-2.0-") && !strings.HasPrefix(banner, "SSH-1.99-") {
-		return fmt.Errorf("unexpected banner from %s: %q", addr, strings.TrimRight(banner, "\r\n"))
-	}
-	return nil
+	reader := bufio.NewReader(conn)
+	for range 50 {
+		line, err := reader.ReadString('\n')
+		if err != nil {
+			return fmt.Errorf("read SSH banner from %s: %w", addr, err)
+		}
+		line = strings.TrimRight(line, "\r\n")
+		if strings.HasPrefix(line, "SSH-2.0-") || strings.HasPrefix(line, "SSH-1.99-") {
+			return nil
+		}
+		if strings.HasPrefix(line, "SSH-") {
+			return fmt.Errorf("unexpected banner from %s: %q", addr, line)
+		}
+	}
+	return fmt.Errorf("too many pre-identification lines from %s", addr)

	// Lima's own SSH client, which has internal retry behavior that can mask a
	// race where the hostagent accepts a TCP connection on 127.0.0.1:sshLocalPort
	// before guest sshd has bound :22. External clients (ssh-keyscan, sftp,
	// rsync, ...) that connect during that window get EPIPE on the first write.
	// This native probe proves the public-facing path is end-to-end usable.
	port := a.sshLocalPort
	req = append(req,
		requirement{
			description: "sshLocalPort serves an SSH banner",
			check: func(ctx context.Context) error {
				return probeSSHBannerOnLocalPort(ctx, port)
			},
			debugHint: `The host agent is listening on 127.0.0.1:<sshLocalPort> but the
proxied connection into the guest is failing. Most commonly this means
guest sshd has not yet bound :22. Check the serial log for sshd startup.
`,

func probeSSHBannerOnLocalPort(ctx context.Context, port int) error {
    addr := fmt.Sprintf("127.0.0.1:%d", port)

The existing script-based ssh requirement at requirements.go:154 dials (a.instSSHAddress, a.sshLocalPort). The new probe ignores a.instSSHAddress and hardcodes 127.0.0.1. Every in-tree driver — qemu, vz, krunkit, wsl2 — returns "127.0.0.1" from SSHAddress(ctx) today, so the two checks target the same address in practice. The divergence matters only for an external driver plugin (pkg/driver/external/client/methods.go:323) that returns a non-loopback host: the existing ssh requirement would honor the plugin's address while the new probe would still dial loopback and either fail forever or — worse — match an unrelated SSH server on the host.

			ConfigFile:     sshConfig.ConfigFile,
			Persist:        false,
			AdditionalArgs: sshutil.DisableControlMasterOptsFromSSHArgs(sshConfig.AdditionalArgs),
		}
	}
	stdout, stderr, err := ssh.ExecuteScript(a.instSSHAddress, a.sshLocalPort, sshConfig, script, r.description)
	logrus.Debugf("stdout=%q, stderr=%q, err=%v", stdout, stderr, err)
	if err != nil {
		return fmt.Errorf("stdout=%q, stderr=%q: %w", stdout, stderr, err)
	}
	return nil

func (d *DriverClient) InspectStatus(_ context.Context, _ *limatype.Instance) string {
	return ""
}

func (d *DriverClient) SSHAddress(ctx context.Context) (string, error) {
	d.logger.Debug("Getting SSH address for the driver instance")

	resp, err := d.DriverSvc.SSHAddress(ctx, &emptypb.Empty{})
	if err != nil {
		d.logger.WithError(err).Error("Failed to get SSH address")

Fix: thread a.instSSHAddress through alongside the port, mirroring the existing requirement.

-	port := a.sshLocalPort
 	req = append(req,
 		requirement{
 			description: "sshLocalPort serves an SSH banner",
 			check: func(ctx context.Context) error {
-				return probeSSHBannerOnLocalPort(ctx, port)
+				return probeSSHBanner(ctx, a.instSSHAddress, a.sshLocalPort)
 			},

-func probeSSHBannerOnLocalPort(ctx context.Context, port int) error {
-	addr := fmt.Sprintf("127.0.0.1:%d", port)
+func probeSSHBanner(ctx context.Context, host string, port int) error {
+	addr := net.JoinHostPort(host, strconv.Itoa(port))

Also rename the function (probeSSHBannerOnLocalPort encodes the loopback assumption) and update the doc comment at requirements.go:338-344, which currently describes "the host agent's TCP forwarder on 127.0.0.1:port."

	conn, err := d.DialContext(ctx, "tcp", addr)
	if err != nil {
		return fmt.Errorf("dial %s: %w", addr, err)
	}
	defer conn.Close()
	if err := conn.SetReadDeadline(time.Now().Add(probeSSHBannerTimeout)); err != nil {
		return fmt.Errorf("set read deadline on %s: %w", addr, err)
	}
	// RFC4253 §4.2: an SSH server sends an identification string ending in CR
	// LF (or just LF historically) before the version exchange. If the proxied
	// connection inside the guest has no live peer, the host side will close
	// and ReadString returns EOF.
	banner, err := bufio.NewReader(conn).ReadString('\n')
	if err != nil {
		return fmt.Errorf("read SSH banner from %s: %w", addr, err)
	}
	if !strings.HasPrefix(banner, "SSH-2.0-") && !strings.HasPrefix(banner, "SSH-1.99-") {
		return fmt.Errorf("unexpected banner from %s: %q", addr, strings.TrimRight(banner, "\r\n"))
	}
	return nil

DialContext honors ctx, but the subsequent ReadString only respects the 5-second read deadline. If the user runs limactl stop while a probe attempt is mid-read, the goroutine still sits in the read for up to 5 seconds. waitForRequirements does check ctx.Done() between attempts (requirements.go:48-53), so this is bounded; it just means the shutdown deadline is "next attempt boundary plus 5 seconds" rather than "immediate."

			}
			if j == retries-1 {
				errs = append(errs, fmt.Errorf("failed to satisfy the %s requirement %d of %d %q: %s: %w", label, i+1, len(requirements), req.description, req.debugHint, err))
				break retryLoop
			}
			select {
			case <-ctx.Done():
				errs = append(errs, fmt.Errorf("failed to satisfy the %s requirement %d of %d %q: %s: %w", label, i+1, len(requirements), req.description, req.debugHint, ctx.Err()))
				return errors.Join(errs...)
			case <-time.After(sleepDuration):
			}
		}
	}
	return errors.Join(errs...)
}

Fix: wire ctx into the read with context.AfterFunc(ctx, func() { conn.Close() }).

 	defer conn.Close()
+	stop := context.AfterFunc(ctx, func() { _ = conn.Close() })
+	defer stop()
 	if err := conn.SetReadDeadline(time.Now().Add(probeSSHBannerTimeout)); err != nil {

	// Server accepts but neither writes a banner nor closes, so the probe must
	// time out on the read deadline rather than hang forever.
	var lc net.ListenConfig
	ln, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0")
	assert.NilError(t, err)
	t.Cleanup(func() { _ = ln.Close() })
	accepted := make(chan net.Conn, 1)
	go func() {
		conn, err := ln.Accept()
		if err != nil {
			return
		}
		accepted <- conn
	}()
	_, portStr, err := net.SplitHostPort(ln.Addr().String())
	assert.NilError(t, err)
	port, err := strconv.Atoi(portStr)
	assert.NilError(t, err)

	ctx, cancel := context.WithTimeout(t.Context(), 2*probeSSHBannerTimeout)
	defer cancel()
	start := time.Now()
	err = probeSSHBannerOnLocalPort(ctx, port)
	elapsed := time.Since(start)
	if conn := <-accepted; conn != nil {
		_ = conn.Close()
	}
	assert.Assert(t, err != nil)
	assert.Assert(t, elapsed < 2*probeSSHBannerTimeout, "probe did not honor read deadline: elapsed=%s", elapsed)
}

If DialContext fails (rare on loopback but not impossible — local port exhaustion, sandboxed CI), the probe returns immediately. The Accept goroutine never unblocks because nothing dialed it. The main goroutine then blocks on <-accepted forever, t.Cleanup never fires (the test function has not returned), and the test stalls until the Go test framework's outer timeout. The realistic failure mode is "test hang, hard to root-cause" rather than a silent miss.

Fix: drop the channel and tie the accepted connection to the test context.

-	accepted := make(chan net.Conn, 1)
 	go func() {
-		conn, err := ln.Accept()
+		conn, err := ln.Accept()
 		if err != nil {
 			return
 		}
-		accepted <- conn
+		defer conn.Close()
+		<-t.Context().Done()
 	}()
 	// ...
 	err = probeSSHBannerOnLocalPort(ctx, port)
 	elapsed := time.Since(start)
-	if conn := <-accepted; conn != nil {
-		_ = conn.Close()
-	}

		requirement{
			description: "sshLocalPort serves an SSH banner",
			check: func(ctx context.Context) error {
				return probeSSHBannerOnLocalPort(ctx, port)
			},
			debugHint: `The host agent is listening on 127.0.0.1:<sshLocalPort> but the
proxied connection into the guest is failing. Most commonly this means
guest sshd has not yet bound :22. Check the serial log for sshd startup.
`,
		})
	req = append(req,
		requirement{
			description: "ssh",
			script: `#!/bin/sh

The existing finalRequirements block at requirements.go:307-311 already branches on *a.instConfig.OS == limatype.DARWIN to pick the right log file — cloud-init-output.log in the guest for Linux, serialv.log in the host for Darwin. The new hint hardcodes "serial log" and sends Linux-guest users — the overwhelming majority — looking for a file that does not exist on their setup.

	}
	return req
}

func (a *HostAgent) finalRequirements() []requirement {
	req := make([]requirement, 0)
	logLocation := "/var/log/cloud-init-output.log in the guest"
	if *a.instConfig.OS == limatype.DARWIN {
		logLocation = "serialv.log in the host"
	}
	req = append(req,
		requirement{
			description: "boot scripts must have finished",
			script: fmt.Sprintf(`#!/bin/sh
set -eux

Fix: mirror the existing pattern, building the hint based on *a.instConfig.OS.

func (a *HostAgent) bashAvailable() bool {
	return *a.instConfig.OS != limatype.FREEBSD
}

func (a *HostAgent) waitForRequirement(ctx context.Context, r requirement) error {
	if r.check != nil {
		logrus.Debugf("checking requirement %q", r.description)
		return r.check(ctx)
	}
	logrus.Debugf("executing script %q", r.description)
	script := r.script
	if a.bashAvailable() {
		var err error
		// FIXME: prefixExportParam depends on bash

func (a *HostAgent) waitForRequirement(ctx context.Context, r requirement) error {
    if r.check != nil {
        logrus.Debugf("checking requirement %q", r.description)
        return r.check(ctx)
    }
    logrus.Debugf("executing script %q", r.description)

The dispatcher silently prefers check over script if both are set; the "exactly one" claim from the doc-comment is documentation, not invariant. Either tighten the dispatcher with an explicit panic/error when both are set, or rephrase the comment as "if check is set, script is ignored."

The package-internal callers all set exactly one today, so this is a small future-proofing note, not a current bug.