sshExecScript: upload script as a file and exec it, instead of piping via stdin
Root cause of ci run #63's stall (found via live inspection this time -- mainframe now hosts the runner, so its ephemeral SSH key is reachable): bash was blocked in do_select() on stdin, and depot_tools hadn't been cloned yet even though later script output (an apt-get install) had already reached the log. The script was truncated mid-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 sync.sh's ~7.4KB single write() when it arrives faster than bash drains it. This is a different, worse failure mode than the EOF-hang bug fixed earlier today (data loss vs. a missing signal), and the previous fix (explicit `exit $?`) didn't address it since the trailing exit was itself part of what got dropped. Fix: never send script content through a PTY's stdin at all. Upload it via the already-proven-reliable, non-PTY uploadBytes path (a plain pipe has no such queue-size limitation), then exec it as a file over a PTY session with no stdin of its own -- no EOF ambiguity when there was never any stdin data to signal the end of in the first place. Verified against a real box with the actual sync.sh (7.4KB): full depot_tools clone, full chromium-fork checkout (500k+ files), gclient sync invoked cleanly through all 5 retry attempts, clean "torn down" -- no truncation, no hang. (The run itself failed on an unrelated, expected test artifact: stall-watch.py wasn't uploaded in this manual invocation.)
This commit is contained in:
parent
5f57db6c82
commit
cec64a1711
1 changed files with 59 additions and 60 deletions
119
main.go
119
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 {
|
||||
|
|
|
|||
Loading…
Reference in a new issue