feat(run): give the command an empty stdin, so no call needs < /dev/null (LAB-7678)

README.md

@@ -12,7 +12,7 @@ is not repeated. A typical use is a secret read in a shell startup file, where
 every new shell would otherwise call the secret manager again:
 
 ```sh
-API_TOKEN="$(ck run --ttl 12h --stale 7d -- op read op://Private/api/token < /dev/null)"
+API_TOKEN="$(ck run --ttl 12h --stale 7d -- op read op://Private/api/token)"
 export API_TOKEN
 ```
 
@@ -55,7 +55,7 @@ prints nothing and exits with the failed run's code. `--refresh` runs the
 command whatever the backoff says:
 
 ```sh
-ck run --refresh -- date -u < /dev/null
+ck run --refresh -- date -u
 ```
 
 Concurrent calls for the same command on one machine run it once: the first
@@ -67,32 +67,32 @@ output passes the limit. With `--backend saas` the backoff is shared by every
 host, but the lock is not, so hosts that miss at the same moment each run the
 command.
 
-### stdin must be `/dev/null`
+### The command does not get your stdin
 
-The cache key covers the command line and `--scope`, not what arrives on
-stdin. So ck caches a call only when stdin is `/dev/null` (or closed). With a
-terminal, a pipe, a file or a socket on stdin, ck runs the command uncached and
-prints one warning. That is why every example here ends in `< /dev/null`.
-Programs that prompt by opening the terminal themselves, as `op`, `ssh` and
-`sudo` do, still reach you with stdin redirected.
+ck runs the command with an empty stdin (`/dev/null`), whatever ck's own stdin
+is, and never reads its own. The cache key does not cover stdin, so this is
+what lets a call cache the same way from a terminal, a pipe, CI or an agent's
+shell. Pass input in arguments, files or `--scope` instead. Programs that
+prompt by opening the terminal themselves, as `op`, `ssh` and `sudo` do, still
+reach you.
 
 ### What the key does not cover
 
 The key is the command and its arguments, plus `--scope`. It does not cover
 the environment, the working directory, the contents of files the command
-reads, or the network. Redirecting stdin to `/dev/null` is your assertion that
-none of those change the output. When one does, put it in `--scope`:
+reads, or the network. Wrapping a command in ck is your assertion that none of
+those change the output. When one does, put it in `--scope`:
 
 ```sh
-ck run --ttl 1h --scope "$PWD" -- ls < /dev/null
+ck run --ttl 1h --scope "$PWD" -- ls
 ```
 
 ### Wrap the command itself
 
 Run the command directly rather than through `sh -c`:
 
 ```sh
-ck run --ttl 5m -- uname -a < /dev/null
+ck run --ttl 5m -- uname -a
 ```
 
 When ck cannot start the command at all (it is not installed, or not
@@ -153,7 +153,7 @@ it. It needs two variables:
 Give every host that shares the cache the same two values:
 
 ```sh
-ck run --backend saas --ttl 12h --stale 1d -- date -u +%F < /dev/null
+ck run --backend saas --ttl 12h --stale 1d -- date -u +%F
 ```
 
 **The master key is the trust boundary.** Every host that holds it can store

src/child.rs

@@ -233,8 +233,10 @@ impl Supervisor {
     }
 
     /// Run `argv` with every `CACHEKIT_*` variable removed from its
-    /// environment. stdin and stderr pass straight through. stdout is
-    /// captured up to `capture` bytes when it is set, else inherited.
+    /// environment, and stdin on `/dev/null`. The key does not cover stdin, so
+    /// the command never gets ck's: that makes every call cacheable whatever
+    /// ck's stdin is. stderr passes straight through. stdout is captured up
+    /// to `capture` bytes when it is set, else inherited.
     pub fn run(&self, argv: &[OsString], capture: Option<usize>) -> Ran {
         let mut command = Command::new(&argv[0]);
         command.args(&argv[1..]);
@@ -245,6 +247,7 @@ impl Supervisor {
                 command.env_remove(name);
             }
         }
+        command.stdin(Stdio::null());
         if capture.is_some() {
             command.stdout(Stdio::piped());
         }

src/cli.rs

@@ -13,8 +13,8 @@ pub const MAX_RETENTION_SECS: u64 = 30 * 86_400;
 pub const USAGE: &str = "\
 usage: ck run [--backend file|saas] [--ttl D] [--stale D] [--scope S] [--refresh] -- <command> [args...]
 
-Runs <command> and caches its stdout when it exits 0. Only a call whose stdin
-is /dev/null is cached, so end every cached call with `< /dev/null`.
+Runs <command> and caches its stdout when it exits 0. The command gets an empty
+stdin, never ck's: pass input in arguments, files or --scope.
 
   --backend B     where entries live: file (default, in ~/.cache/ck) or saas
                   (CacheKit, shared across hosts; needs CACHEKIT_API_KEY and

src/run.rs

@@ -5,10 +5,9 @@
 //! is inert while the value is fresh.
 
 use std::fs;
-use std::io::IsTerminal;
 use std::ops::ControlFlow;
-use std::os::fd::{AsFd, OwnedFd};
-use std::os::unix::fs::{DirBuilderExt, FileTypeExt, MetadataExt};
+use std::os::fd::OwnedFd;
+use std::os::unix::fs::DirBuilderExt;
 use std::path::{Path, PathBuf};
 use std::time::Duration;
 
@@ -23,18 +22,11 @@ use crate::store::{ReadError, Store};
 use crate::{emit, warn, Fatal};
 
 pub fn run(args: &RunArgs, connect: &Connect) -> Result<i32, Fatal> {
-    // saas credentials are checked first, so a saas call without them exits
-    // 125 whatever its stdin, before anything runs.
+    // A saas call without its credentials exits 125 before anything runs.
     let credentials = match args.backend {
         BackendKind::File => None,
         BackendKind::Saas => Some(saas::credentials()?),
     };
-    if let Some(kind) = uncacheable_stdin() {
-        warn(&format!(
-            "stdin is {kind}, so this call is not cached. End the command with `< /dev/null` to cache it"
-        ));
-        return Ok(run_uncached(&Supervisor::install()?, &args.command));
-    }
 
     let home = std::env::var_os("HOME")
         .filter(|h| !h.is_empty())
@@ -334,49 +326,6 @@ fn run_uncached(supervisor: &Supervisor, argv: &[std::ffi::OsString]) -> i32 {
     }
 }
 
-/// The key covers argv and `--scope` only, so only a call whose caller has
-/// shut stdin off is cached: `/dev/null`, or a closed fd 0 (which Rust's
-/// runtime reopens on `/dev/null` before `main`). A TTY carries whatever the
-/// user types, so it does not qualify. Returns what stdin is otherwise.
-fn uncacheable_stdin() -> Option<&'static str> {
-    let stdin = std::io::stdin();
-    let meta = match stdin
-        .as_fd()
-        .try_clone_to_owned()
-        .map(fs::File::from)
-        .and_then(|f| f.metadata())
-    {
-        Ok(meta) => meta,
-        Err(e) if e.raw_os_error() == Some(rustix::io::Errno::BADF.raw_os_error()) => return None,
-        Err(_) => return Some("not inspectable"),
-    };
-    let kind = meta.file_type();
-    if kind.is_char_device() {
-        let Ok(null) = fs::metadata("/dev/null") else {
-            // Without /dev/null to compare against, nothing proves stdin
-            // is empty.
-            return Some("not inspectable");
-        };
-        if meta.rdev() == null.rdev() {
-            return None;
-        }
-        return Some(if stdin.is_terminal() {
-            "a terminal"
-        } else {
-            "a character device"
-        });
-    }
-    Some(if kind.is_fifo() {
-        "a pipe"
-    } else if kind.is_socket() {
-        "a socket"
-    } else if kind.is_file() {
-        "a file"
-    } else {
-        "not /dev/null"
-    })
-}
-
 /// Take the fill lock, an exclusive `flock` on the key's lockfile. When it is
 /// contended, wait for it only if `wait`; otherwise return `None`. flock is
 /// released when its holder exits or dies, so no lock goes stale.

tests/common/mod.rs

@@ -43,7 +43,8 @@ impl Sandbox {
 
     /// A command on PATH that records each call, then prints the contents of
     /// `out` (default `hello`) and exits with the code in `exit` (default 0).
-    /// `sleep` and `env` files make it sleep first or dump its environment;
+    /// `sleep`, `env` and `stdin` files make it sleep first, dump its
+    /// environment, or copy what it reads on stdin, then `EOF`, to `stdin.out`;
     /// `selfkill` (holding a signal name) makes it send that signal to itself.
     pub fn install_shim(&self, name: &str) {
         let r = self.root.path().display();
@@ -53,6 +54,7 @@ impl Sandbox {
              [ -f '{r}/sleep' ] && sleep \"$(cat '{r}/sleep')\"\n\
              [ -f '{r}/selfkill' ] && kill -\"$(cat '{r}/selfkill')\" $$\n\
              [ -f '{r}/env' ] && env > '{r}/env.out'\n\
+             [ -f '{r}/stdin' ] && {{ cat; echo EOF; }} > '{r}/stdin.out'\n\
              if [ -f '{r}/out' ]; then cat '{r}/out'; else echo hello; fi\n\
              exit \"$(cat '{r}/exit' 2>/dev/null || echo 0)\"\n"
         );

tests/readme.rs

@@ -6,7 +6,7 @@ mod common;
 use std::fs;
 use std::os::unix::fs::PermissionsExt;
 use std::path::Path;
-use std::process::Command;
+use std::process::{Command, Stdio};
 
 use common::{code, readme_sh_blocks, stderr, Sandbox, BIN};
 
@@ -27,13 +27,6 @@ fn readme_examples_run() {
     let ck_dir = Path::new(BIN).parent().unwrap();
 
     for example in examples {
-        for call in example.split("ck run").skip(1) {
-            let call = call.split([')', '\n']).next().unwrap_or_default();
-            assert!(
-                call.trim_end().ends_with("< /dev/null"),
-                "a ck run call without the redirect:\n{example}"
-            );
-        }
         // tests/saas.rs runs these against its fake CacheKit.
         if example.contains("--backend saas") {
             continue;
@@ -51,8 +44,10 @@ fn readme_examples_run() {
             .args(["-euc", &example])
             .env("HOME", s.home())
             .env("PATH", path)
+            // A pipe, as in CI, not /dev/null: the examples carry no redirect.
+            .stdin(Stdio::piped())
             .output()
             .unwrap();
-        assert_eq!(code(&out), 0, "{example}\n{}", stderr(&out));
+        assert_eq!((code(&out), stderr(&out)), (0, String::new()), "{example}");
     }
 }

tests/run.rs

@@ -223,9 +223,12 @@ fn lowering_stale_stops_an_old_entry_being_served() {
 }
 
 #[test]
-fn only_dev_null_stdin_is_cached() {
-    let file = tempfile::NamedTempFile::new().unwrap();
-    // Each stdin comes with whatever must stay open while ck runs.
+fn the_command_gets_an_empty_stdin_whatever_cks_is() {
+    use std::io::Write;
+    let mut file = tempfile::NamedTempFile::new().unwrap();
+    file.write_all(b"from a file\n").unwrap();
+    // Each stdin holds data, and comes with whatever must stay open while ck
+    // runs. A pipe's data is written once ck is running.
     let stdin_for = |kind: &str| -> (Stdio, Option<std::os::fd::OwnedFd>) {
         match kind {
             "a terminal" => {
@@ -239,46 +242,75 @@ fn only_dev_null_stdin_is_cached() {
                     .write(true)
                     .open(name.to_str().unwrap())
                     .unwrap();
+                // What the user typed, waiting to be read.
+                rustix::io::write(&master, b"typed\n").unwrap();
                 (Stdio::from(slave), Some(master))
             }
             "a pipe" => (Stdio::piped(), None),
             "a socket" => {
                 let (a, b) = UnixStream::pair().unwrap();
+                (&b).write_all(b"from a socket\n").unwrap();
                 (Stdio::from(std::os::fd::OwnedFd::from(a)), Some(b.into()))
             }
             _ => (Stdio::from(fs::File::open(file.path()).unwrap()), None),
         }
     };
-    let cases = ["a terminal", "a pipe", "a socket", "a file"];
-
-    for kind in cases {
+    for kind in ["a terminal", "a pipe", "a socket", "a file"] {
         let s = Sandbox::new();
+        s.set("stdin", "");
         for call in 1..=2 {
             let (stdin, _keep) = stdin_for(kind);
-            let out = s
-                .ck(&["run", "--", "origin"])
+            let mut child = s
+                .ck(&["run", "--ttl", "1h", "--", "origin"])
                 .stdin(stdin)
-                .output()
+                .spawn()
                 .unwrap();
-            assert_eq!(code(&out), 0, "{kind}");
-            let err = stderr(&out);
-            assert_eq!(err.lines().count(), 1, "{kind}: {err}");
-            assert!(
-                err.contains(kind) && err.contains("< /dev/null"),
-                "{kind}: {err}"
+            if let Some(mut pipe) = child.stdin.take() {
+                // ck may exit before the write lands, as nothing reads the
+                // pipe. Then the write fails with EPIPE, which is no failure.
+                if let Err(e) = pipe.write_all(b"from a pipe\n") {
+                    assert_eq!(e.kind(), std::io::ErrorKind::BrokenPipe, "{e}");
+                }
+            }
+            // A command handed ck's terminal or socket would wait on it forever.
+            let msg = format!("{kind}: the command waited on ck's stdin");
+            wait_exit(&mut child, Duration::from_secs(5), &msg);
+            let out = child.wait_with_output().unwrap();
+            assert_eq!(
+                (code(&out), stdout(&out), stderr(&out)),
+                (0, "hello\n".into(), String::new()),
+                "{kind}, call {call}"
             );
-            assert_eq!(s.runs(), call, "{kind}: the call was cached");
         }
-        assert!(
-            !s.home().join(".cache").exists(),
-            "{kind}: an uncached call touched the cache"
+        assert_eq!(s.runs(), 1, "{kind}: the second call did not hit");
+        assert_eq!(
+            s.read("stdin.out"),
+            "EOF\n",
+            "{kind}: the command read stdin"
         );
+        // Stdin is not in the key: a call with `< /dev/null` hits the same entry.
+        let out = s.run(&["--ttl", "1h"]);
+        assert_eq!((code(&out), s.runs()), (0, 1), "{kind}: /dev/null missed");
     }
+}
 
+#[test]
+fn a_pipe_that_never_closes_does_not_hang_ck() {
     let s = Sandbox::new();
-    s.run(&[]);
-    let out = s.run(&[]);
-    assert_eq!((stderr(&out), s.runs()), (String::new(), 1));
+    s.set("stdin", "");
+    for _ in 1..=2 {
+        let mut child = s
+            .ck(&["run", "--ttl", "1h", "--", "origin"])
+            .stdin(Stdio::piped())
+            .spawn()
+            .unwrap();
+        // The writer stays open and never writes.
+        let _writer = child.stdin.take();
+        let status = wait_exit(&mut child, Duration::from_secs(5), "ck hung on its stdin");
+        assert!(status.success());
+    }
+    assert_eq!(s.runs(), 1);
+    assert_eq!(s.read("stdin.out"), "EOF\n");
 }
 
 #[test]

tests/saas.rs

@@ -506,13 +506,17 @@ fn slow_but_healthy_fills_and_hits() {
 }
 
 fn a_401_on_the_value_read_runs_uncached() {
+    let mut file = tempfile::NamedTempFile::new().unwrap();
+    file.write_all(b"from a file\n").unwrap();
     for exit in ["0", "5"] {
         let service = Service::new();
         let host = Sandbox::new();
         host.set("exit", exit);
+        host.set("stdin", "");
         let out = service
             .ck(&host, &[])
             .env(FAULT, "get=401")
+            .stdin(fs::File::open(file.path()).unwrap())
             .output()
             .unwrap();
 
@@ -530,6 +534,8 @@ fn a_401_on_the_value_read_runs_uncached() {
             assert!(warnings[0].contains(fix), "no {fix:?} in {warnings:?}");
         }
         assert_eq!(host.runs(), 1);
+        // Running uncached, the command still gets an empty stdin.
+        assert_eq!(host.read("stdin.out"), "EOF\n");
         assert_eq!(service.calls(), ["get value"]);
     }
 }
@@ -605,8 +611,7 @@ fn unusable_api_keys_exit_125() {
             Some(k) => cmd.env("CACHEKIT_API_KEY", k),
             None => cmd.env_remove("CACHEKIT_API_KEY"),
         };
-        // Stdin that would otherwise run the command uncached.
-        let out = cmd.stdin(std::process::Stdio::piped()).output().unwrap();
+        let out = cmd.output().unwrap();
         assert_eq!(code(&out), 125, "{key:?}: {}", stderr(&out));
         assert!(stderr(&out).contains(says), "{key:?}: {}", stderr(&out));
     }
@@ -648,6 +653,8 @@ fn readme_saas_examples_run() {
                 .env(FAKE, service.0.path())
                 .env("CACHEKIT_API_KEY", "ck_sdk_test")
                 .env("CACHEKIT_MASTER_KEY", MASTER_KEY_HEX)
+                // A pipe, as in CI, not /dev/null: the examples carry no redirect.
+                .stdin(std::process::Stdio::piped())
                 .output()
                 .unwrap()
         };