diff --git a/main.go b/main.go index 28102bb..22351df 100644 --- a/main.go +++ b/main.go @@ -423,21 +423,29 @@ func sshExec(st *state, command string) (int, error) { return runSession(session, command) } -// uploadFile copies a local file to the remote box by piping its bytes to -// `cat > remotePath` over a plain (non-PTY) exec session -- no SFTP -// subsystem required, just enough to get companion scripts (e.g. -// stall-watch.py) onto a box whose only other content is whatever gets -// piped to `bash -s`. -// -// Uses Start()+StdinPipe()+explicit Close() rather than Run()'s automatic -// Stdin-reader-copying, so the exact moment EOF is signaled to the remote -// `cat` is under our control instead of implicit in the library's internal -// copy goroutine. +// uploadFile copies a local file to the remote box. See uploadBytes for +// the transfer mechanism. func uploadFile(st *state, localPath, remotePath string) error { content, err := os.ReadFile(localPath) if err != nil { return err } + return uploadBytes(st, content, remotePath) +} + +// uploadBytes writes content to remotePath by piping it to `cat > +// remotePath` over a plain (non-PTY) exec session -- no SFTP subsystem +// required. Deliberately non-PTY: a plain pipe has no fixed-size line +// buffer the way a PTY's canonical-mode input queue does (historically +// 4KB on Linux) and won't silently truncate a multi-KB write -- see +// sshExecScript's doc comment for why that distinction turned out to +// matter a great deal. +// +// Uses Start()+StdinPipe()+explicit Close() rather than Run()'s automatic +// Stdin-reader-copying, so the exact moment EOF is signaled to the remote +// `cat` is under our control instead of implicit in the library's internal +// copy goroutine. +func uploadBytes(st *state, content []byte, remotePath string) error { session, client, err := sshSession(st, false) if err != nil { return err @@ -472,66 +480,57 @@ func uploadFile(st *state, localPath, remotePath string) error { return nil } -// sshExecScript pipes script to `bash -s` on the remote box, prefixed with -// `export` lines for env. Values are forwarded only over the SSH channel's -// stdin — never as a command-line argument on either end — so they don't -// show up in `ps` output on the box or in this process's argv. -// sshExecScript pipes script to `bash -s` over a PTY (needed so the remote -// side line-buffers instead of block-buffering -- see sshSession). +// sshExecScript runs script on the remote box by uploading it as a file +// and executing it, rather than piping it to `bash -s` over stdin. // -// The script ends with an explicit `exit $?` rather than relying on EOF to -// end bash's read loop. Confirmed in production (ci runs #56 and #60, -// plus a direct reproduction: a script that finishes and prints its own -// "end" timestamp, after which the local process sits for 10+ more -// minutes) that PTY stdin EOF is not a reliable "hang up" signal the way -// it is on a plain pipe -- a PTY is fundamentally for interactive -// sessions, where "stopped sending input" doesn't inherently mean -// "terminate". This held true even after switching to -// Start()+StdinPipe()+explicit Close() (the fix that *did* work for -// uploadFile, which uses a plain, non-PTY session) -- ruling out -// Run()'s implicit copying as the cause and pointing at the PTY itself. -// An explicit `exit` sidesteps the ambiguity entirely: bash terminates on -// its own the instant the script's last command finishes, independent of -// whatever the PTY does or doesn't do with EOF. +// This replaced two earlier, progressively-less-wrong attempts, both +// disproven in production: +// 1. `session.Run("bash -s")` with Stdin set to a Reader -- hung +// indefinitely after the script's real work finished (ci runs #56, +// #60): PTY stdin EOF is not a reliable "hang up" signal, since a PTY +// is fundamentally for interactive sessions where a pause in input +// isn't supposed to mean "terminate". Confirmed independently outside +// this codebase: golang/go#6088 (identical x/crypto/ssh symptom) and +// net-ssh/net-ssh#460 (different language, same symptom, confirms +// removing request_pty fixes it). +// 2. Switching to Start()+StdinPipe()+explicit Close(), appending +// `exit $?` so bash would terminate on its own instead of depending on +// EOF at all -- this fixed short scripts but a real sync.sh run (ci +// run #63) still hung, this time *mid-script*: the process was found +// blocked in do_select() on its stdin fd, and directories the script +// creates early on (depot_tools clone) didn't exist yet even though +// later output (an apt-get install several lines further down) had +// already reached the log. That means the script was truncated +// partway through delivery, not just missing its termination signal -- +// consistent with a PTY's small, fixed-size canonical-mode input queue +// (historically 4KB on Linux) silently dropping the tail of a +// multi-KB single write() that arrives faster than bash drains it. A +// plain pipe (as uploadBytes uses) doesn't have this limitation. +// +// Uploading the script and executing it as a file sidesteps this whole +// class of problem: no script content ever goes through a PTY's stdin at +// all, only the plain, already-proven-reliable uploadBytes path. The PTY +// is still used for the *execution* (via sshExec, for live output +// streaming), but that session has no stdin data of its own -- a plain +// command with nothing written to its stdin has no EOF ambiguity to begin +// with. func sshExecScript(st *state, script string, env map[string]string) (int, error) { - session, client, err := sshSession(st, true) - if err != nil { - return -1, err - } - defer client.Close() - defer session.Close() - var buf strings.Builder + buf.WriteString("#!/usr/bin/env bash\n") for k, v := range env { fmt.Fprintf(&buf, "export %s=%s\n", k, shellQuote(v)) } buf.WriteString(script) - buf.WriteString("\nexit $?\n") - stdin, err := session.StdinPipe() - if err != nil { - return -1, err + remotePath := fmt.Sprintf("/root/.hetzner-runner-script-%d.sh", time.Now().UnixNano()) + if err := uploadBytes(st, []byte(buf.String()), remotePath); err != nil { + return -1, fmt.Errorf("uploading script: %w", err) } - session.Stdout = os.Stdout - session.Stderr = os.Stderr - - if err := session.Start("bash -s"); err != nil { - return -1, err + code, err := sshExec(st, fmt.Sprintf("bash %s", shellQuote(remotePath))) + if rmCode, rmErr := sshExec(st, fmt.Sprintf("rm -f %s", shellQuote(remotePath))); rmErr == nil && rmCode != 0 { + fmt.Fprintf(os.Stderr, "warning: failed to remove remote script %s\n", remotePath) } - if _, err := stdin.Write([]byte(buf.String())); err != nil { - return -1, err - } - if err := stdin.Close(); err != nil { - return -1, err - } - - if err := session.Wait(); err != nil { - if exitErr, ok := err.(*ssh.ExitError); ok { - return exitErr.ExitStatus(), nil - } - return -1, err - } - return 0, nil + return code, err } func shellQuote(v string) string {