Repository navigation
feat(run): ck run on the file backend (LAB-5118) - #2
Conversation
ck run wraps a command and caches its stdout when it exits 0, on the cachekit-rs file backend with client-side encryption. - Reader-side freshness from a stored_at timestamp, so each call applies its own --ttl and --stale. - A keyed BLAKE2b-256 cache key over the length-prefixed scope and argv. - A failure marker with escalating backoff and a 15-minute active window. - A per-key flock fill lock, so concurrent cold calls run the command once. - An auto-created file key, published with link() so racing first runs agree. - Only /dev/null stdin is cached; CACHEKIT_* is stripped from the child. - SIGTERM and SIGHUP are forwarded; output past 20 MiB streams uncached. The binary is named ck. CI gains an MSRV 1.85 build and the hit-path budget benchmark, cargo-deny bans a direct HTTP client, and the licence allow list carries exactly the permissive licences in the graph.
- Record a received signal inside its handler, so a child killed by a process-group SIGINT is never read as an ordinary failure. A child that dies of SIGINT, SIGQUIT, SIGTERM or SIGHUP also counts as interrupted. - After the child exits, a signal ends ck at once instead of being dropped. - Leave signals inherited as ignored alone, so `nohup ck run` keeps its SIGHUP immunity and the command inherits it. Reading the disposition is the crate's one unsafe block; unsafe_code is deny instead of forbid. - Delete the marker on every exit-0 run, even one too large to store. - Stop streaming and drop the pipe once stdout fails, so the command gets SIGPIPE. - Treat stdin as uncacheable when /dev/null cannot be inspected. - Read file.key through one O_NOFOLLOW descriptor that is also fstat'ed. - Reject a non-ASCII duration instead of panicking; never panic on a closed stderr.
With SIGCHLD ignored, the kernel reaps the child itself and its exit status is lost, so a successful run read as exit 126 and set a failure marker. - When SIGCHLD is inherited as ignored, ck handles it so the status can be collected, and sets it back to ignored in the child before exec. - An exit status that cannot be collected is no evidence the origin failed: no marker, stale output if servable, otherwise exit 126 with a warning. - A waitid error other than EINTR is no longer swallowed.
- A run is interrupted only when ck itself received the signal. A command that dies of a signal on its own (`kill -INT $$`, `pkill op`) is an ordinary failure, so a stale value is still served. - ck's helper threads (the signal thread and tokio's blocking pool) block the handled signals, so their handlers always run on the main thread and the flag is set before the main thread reads the child's status. - When ck's reader goes away mid-stream, the command's SIGPIPE death sets no failure marker. - A signal that arrives while only a background job holds the command's stdout ends ck at once. - file.key is opened O_NONBLOCK | O_NOCTTY, so a FIFO there exits 125 instead of hanging.
- On a handled signal while the command runs, the signal thread forwards it (TERM and HUP), waits for the command to exit, and exits 128 + n. A background job left holding the command's stdout no longer keeps ck alive. - An interrupted run prints nothing, as well as storing nothing. - When the command dies of a handled signal before ck's own handler has run, wait up to 50 ms for it: kernels that signal a process group one member at a time give no order against the child's exit. - Rename Stdout::ReaderGone to OutputLost, since any stdout write failure sets it. - README: the Signals section now matches the behaviour. - Tests poll a readiness file instead of sleeping, and a Linux test checks that every helper thread blocks the handled signals.
- The signal thread forwards every SIGTERM and SIGHUP while the command runs, so a command that traps the first one can still be stopped. On the first signal it records it and starts a waiter that ends ck once the command itself has exited. - Every path that ends ck on a signal exits 128 + the first signal, so two different signals give one deterministic exit code. - README and docs: a signal discards the output ck was holding back; output already written stays written. - Tests: INT then TERM and TERM then TERM against commands that trap the first signal, the exited-command case again, and readiness polls in place of fixed sleeps.
- record_first reports whether it recorded the signal, and only that call starts the waiter, so repeated TERM or HUP signals add no threads. - A signal that arrived while the spawn path held the state lock ends the run before the command starts. - README: signals that arrive together count in signal-number order; a streamed run can lose its tail on a signal; a signal inside the spawn itself is honoured when the command exits. - Tests: a Linux thread-count check, readiness polls in place of the last fixed pauses, and one wait_exit helper.
…-5118) - Signals::new now runs before the flag handlers, so a signal landing between the two still reaches the signal thread, which exits 128 + n. - A signal found before spawn releases the lock and lets the signal thread decide the first signal, so signals keep signal-number order there too. - README: only SIGINT and SIGQUIT can miss a command that is starting, and only an interrupted streamed run can lose its tail. - Tests: ck runs as a process-group leader and wait_exit kills the whole group on timeout; the thread-count test waits on a baseline.
signal-hook-registry installs a signal's OS handler before it publishes the action list, so a signal in that window reached neither the iterator nor the flag. install now blocks the handled signals on the main thread for the whole of installation and the signal-thread spawn, then restores the caller's mask with SIG_SETMASK. A signal sent meanwhile is delivered after every action is published, to both handlers. The mask code is one sigmask helper, still the crate's single unsafe site for it. settle_first waits up to 5 s for the signal thread's record, now that every flagged signal is guaranteed to reach that thread. Redundant per-test process_group calls are gone.
…LAB-5118) Every return in cached() that runs no command (stale on a contended lock, a fresh value read under the lock, stale or suppression under an active marker) and the spawn-failure path now checks Supervisor::interrupted() first. A signal ck has already received ends the call with 128 + n and no output, instead of being ignored or answered alongside served output. A signal that arrives after the check is still answered by the signal thread, so ck stays killable while it blocks writing a served value. README and docs: ck exits 128 plus the number of a signal it received; which of several close together is unspecified.
- Supervisor::run's spawn-failure arm now releases the state lock and returns Interrupted when ck has already received a signal, so the uncached path (stdin not /dev/null, lock or backend failure) exits 128 + n instead of racing to 127 or 126. The separate check in record() is gone: one site covers both callers. - README: a signal while ck prints stored output can cut it off, and ck still exits 128 + n, so $(...) callers must check the exit code. - Comments no longer claim every no-command return checks, or that the signal thread sees signals in order.
cachekit-rs 0.9.0 sweeps orphaned temp files on a file backend's first set instead of in FileBackend::build. A hit no longer reads the whole cache directory, so the 10,000-entry hit-path bench drops from 3.8 ms to 0.5 ms added on Linux. 0.9.0 also makes EncryptionLayer::new reject any master key that is not exactly 32 bytes. ck accepted 32 or more, so a longer CACHEKIT_MASTER_KEY would now fail inside the library with advice meant for Rust callers. ck checks for exactly 32 bytes itself and names `openssl rand -hex 32`, and the README says the key is exactly 64 hex characters.
|
@kody review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
Kody Code Review — 2 suggested fixes. 🛠️ Open Agent Prompt |
| 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)" |
There was a problem hiding this comment.
Violates team rule 'No hardcoded API key/token literal in credential-named variable': WHAT: Flag a string literal assigned to a variable, constant, struct field, or default value whose name indicates a credential (api_key, token, secret, password, access_key, client_secret) in non-test source code.
HOW: In ADDED lines, look for let/const/static <credential-ish name> = "..." or credential_field: "..." where the right-hand side is a literal instead of an env/secret lookup.
Kody rule violation: Treat suspicious hardcoded tokens/keys as secrets and move them to a secret store
Prompt for LLM
File README.md:
Line 15:
Violates team rule 'No hardcoded API key/token literal in credential-named variable': WHAT: Flag a string literal assigned to a variable, constant, struct field, or default value whose name indicates a credential (api_key, token, secret, password, access_key, client_secret) in non-test source code.
HOW: In ADDED lines, look for `let/const/static <credential-ish name> = "..."` or `credential_field: "..."` where the right-hand side is a literal instead of an env/secret lookup.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Not a hardcoded credential. API_TOKEN="$(ck run … -- op read op://Private/api/token < /dev/null)" assigns the output of a command substitution: the token is read from 1Password at run time, and no literal secret appears in the README.
A decrypt or AAD failure stays a miss that is never served, but it now prints one stderr line per call. A new master key also changes the keyed file name, so key rotation never reaches this path: a failure here means a corrupted or tampered entry, which should not pass silently. The message carries the library's error text and no key material.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Adds
ck runon the file backend. It wraps a command and caches its stdout when the command exits 0.ck requires cachekit-rs 0.9. In 0.8,
FileBackend::buildread every entry in the cache directory on each start, looking for orphaned temp files, and against 10,000 entries that put ck over its 5 ms hit-path budget. 0.9.0 moves that sweep to a backend's firstset, so a hit no longer pays it: on the same Linux host, the added time drops from 3.8 ms to 0.5 ms. 0.9.0 also requires a raw master key of exactly 32 bytes, so ck now checks for exactly 32 itself.What
ck rundoes{stdout, stored_at}. Each call judges age on its own clock against its own--ttland--stale, so lowering--staletakes effect on the next call. The backend TTL is the writer'sttl + staleand only collects garbage.ns:ck-run:v1:<64 hex>is a keyed BLAKE2b-256 over the length-prefixed--scope, then each argv element. The hash key comes from cachekit-core'sderive_domain_key(master, "cache_keys", tenant), which is the protocol'scache_key_salt. The failure marker sits at the value key plus:neg. Both use the sameEncryptionLayer, so the AAD binds each one to its own key.{exit, failures, retry_at}. Backoff ismin(30 s × 2^(n−1), 15 min), and the marker's backend TTL is(retry_at − now) + 15 min. A marker is active only whilenow < retry_at ≤ now + 15 min. A fresh value always wins, and the marker is read only when no fresh value exists. Only ck's own spawn failure (exit 127 or 126) leaves the marker untouched.flockon~/.cache/ck/locks/<hash>, openedO_CLOEXECand never unlinked. A contended call serves stale output when it has some, and otherwise waits. Entries live in~/.cache/ck/data.CACHEKIT_MASTER_KEY, ck creates~/.config/ck/file.keyon first use. It writes the key under a unique temp name withO_EXCL, mode 0600, andfsyncs it. Then itlink()s the temp tofile.key,fsyncs the directory, and removes the temp name. A key file that is not 64 hex characters, not owned by the user, or not mode 0600 exits 125 and is left alone./dev/null(or a closed fd 0) is cached. A terminal, pipe, file or socket runs the command uncached with one warning that names< /dev/null.CACHEKIT_*variable is removed before the command starts.nohup, stays ignored and is never forwarded. An inherited ignored SIGCHLD is the exception: ck handles it so the child's exit status survives, and sets it back to ignored in the child before exec. A run whose exit status cannot be collected, or whose output could not be written, sets no marker.ttl + stale > 30d, aCACHEKIT_MASTER_KEYthat is not exactly 32 bytes of hex, an unusable file key, or a cache directoryFileBackend::buildrefuses (that message nameschmod 700). A backend read error runs the command uncached with one warning. A file-backendTimeoutand a decrypt failure are both misses. Any write error after the run is a warning, and the exit code stays the command's.ck drives cachekit-rs at the
Backendtrait withdefault-features = falseandencryption, file. It parses arguments by hand, because one subcommand does not justify a parser framework on a path with a 5 ms budget.Repo changes
ck([[bin]] name = "ck"), and the CI binary grep scanstarget/release/ck.rust-version = "1.85"withresolver = "3", so the lockfile resolves to 1.85-compatible versions. A newmsrvjob runscargo +1.85 check --locked --all-targets.deny.tomlbans a direct HTTP client:deny = [{ crate = "reqwest", wrappers = ["cachekit-rs"] }]. Addingreqwestdirectly failscargo deny check bans. With cachekit-rs'scachekitiofeature on, it passes, so the wrapper list is exact.url, plus unicode-ident).cbindgen(MPL-2.0). It is an unconditional build dependency of cachekit-core, but its build script only runs it under theffifeature, which ck does not enable. None of its code is linked into ck. No copyleft licence goes on the allow list. Making cbindgen optional in cachekit-core would remove the exception.cachekit-rs, pluscachekit-core,blake2,hex,getrandom,libc,rustixandtokio(rtonly), which cachekit-rs already pulls in.signal-hookandsignal-hook-registry(MIT OR Apache-2.0) are the only crates new to the graph. Dev-only:tempfile.unsafe_codemoves fromforbidtodeny, with three audited exceptions insrc/child.rs.inherited_ignoredis asigactioncall with a null new action, which only reads a disposition.ignore_sigchld_after_forkis apre_exechook that calls only async-signal-safesignal.block_handled_signalsis apthread_sigmaskcall that blocks the four handled signals on the calling thread only. No dependency offers any of these safely.Tests
cargo test --lockedruns 12 unit tests, 40 separate-process tests and a README test. The separate-process tests drive the real binary with its ownHOMEand a PATH shim that counts its calls. They cover:--stalestops an old entry being served;/dev/nullcaches;CACHEKIT_*variable;--refreshnever serves stale;nohup) stays ignored;file.keyexits 125 without hanging;tests/readme.rsruns everyshblock in the README with a stand-inoponPATH. It also fails if an example lacks< /dev/null.tests/bench.rsis the hit-path gate. It timesck run -- trueagainsttrueover 300 interleaved runs, after filling the cache directory with 10,000 entries, and fails if the median adds more than 5 ms. CI runs it in release mode on Linux and macOS.Locally on Linux, fmt, clippy
-D warnings,cargo test --locked,cargo deny checkand the 1.85 check all pass. The suite also passes pinned to one CPU (taskset -c 0), which is how the Ctrl-C race reproduces on the earlier code. With cachekit-rs 0.9.0 the benchmark adds 0.5 ms locally on Linux (ext4), against 3.8 ms on 0.8.0 on the same host.