diff --git a/README.md b/README.md index a01f300..1f3a4d3 100644 --- a/README.md +++ b/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,24 +67,24 @@ 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 @@ -92,7 +92,7 @@ ck run --ttl 1h --scope "$PWD" -- ls < /dev/null 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 diff --git a/src/child.rs b/src/child.rs index 2857e75..70b70d6 100644 --- a/src/child.rs +++ b/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) -> 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()); } diff --git a/src/cli.rs b/src/cli.rs index b6d3c32..30c6425 100644 --- a/src/cli.rs +++ b/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] -- [args...] -Runs 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 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 diff --git a/src/run.rs b/src/run.rs index 17360d9..a5e5713 100644 --- a/src/run.rs +++ b/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 { - // 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. diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 9527e6a..cf96361 100644 --- a/tests/common/mod.rs +++ b/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" ); diff --git a/tests/readme.rs b/tests/readme.rs index 8d96642..06866e3 100644 --- a/tests/readme.rs +++ b/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}"); } } diff --git a/tests/run.rs b/tests/run.rs index bdc100e..8bf3f1f 100644 --- a/tests/run.rs +++ b/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) { 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] diff --git a/tests/saas.rs b/tests/saas.rs index d307fd2..b15799d 100644 --- a/tests/saas.rs +++ b/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() };