fix(remote): never fail a build on remote config, restore legacy prefixes#600
Merged
Conversation
…ixes Follow-up hardening for the OpenDAL remote (#593). Blocking issues: - Remote config errors no longer fail the build. `load_remote_config` became fallible in #593 and `Config::load` propagated it with `?`, which `run_wrapper_mode` also propagates — so a bad remote killed every compiler invocation instead of costing cache hits. The reason is now recorded in `Config::remote_error`, the build continues local-only, and commands that exist to use the remote surface it via `require_remote()`. `kache status` reports MISCONFIGURED rather than "not configured". - Legacy prefixes work again. `prefix = "team/"`, `/team`, `a//b` and an empty `KACHE_S3_PREFIX` were all accepted before #593 and became hard errors. They now normalize (with a warning that objects move once); only `..`, `.` and backslashes stay errors. An empty prefix addresses the bucket root, which needs `join_remote_key` so keys do not gain a leading slash. - A present-but-empty `KACHE_S3_BUCKET` disables the remote instead of silently falling back to the file-configured bucket, which pointed jobs at a different remote than they asked for. - The S3 HTTP client sets connect and read timeouts again. Only `pool_idle_timeout` was configured, so a silent endpoint hung head/get/put forever with RetryLayer unable to fire. 3.1s matches the AWS SDK default this replaced. Transport and filesystem: - `credential_process` is re-lexed with shlex-style rules and executed directly instead of being rejoined and handed to `sh -c` / `cmd.exe /C`. reqsign splits on whitespace without stripping quotes, so the quoting must be reapplied, but a shell also expands globs, `$(...)`, `%VAR%` and treats `&`/`|`/`^` as operators — and under cmd.exe single quotes do not group at all. - `get` rejects a body shorter than its advertised length. - `list` gains a total deadline and an entry cap; the per-entry timeout only caught a stalled lister. - Filesystem writes verify the target resolves inside the configured root. OpenDAL canonicalizes only the root, so a symlink planted below it redirected writes out of the cache. Best-effort: TOCTOU remains. - Keys reject control characters, and trailing dots/spaces on filesystem remotes, which Windows strips (two keys would collide). - `atomic_write_dir` is rejected when on another filesystem (rename would EXDEV on every write) or inside the object prefix (staging files would list as cached objects). Smaller items: - `[cache.remote]` rejects unknown keys, so `bukcet = "..."` is reported. - `default-features = false` on the remaining OpenDAL crates (resolved graph unchanged; the manifest now matches the stated policy). - The JSON report carries `network.backend` so consumers can tell whether the HTTP-named metrics describe a network or a local mount. - `sync --workspace` validates the workspace before resolving credentials.
Follow-up from a cross-model review of the previous commit. - The staging-directory check compared against the whole object root, so with an empty prefix the documented default `<path>/.kache-tmp` sits under it and every empty-prefix filesystem remote was declared unusable. Compare against the object tree kache actually lists (`<prefix>/v3`) instead. - Move the same-filesystem (EXDEV) check out of `Config::load` and into `create_filesystem_operator`. It needs real syscalls, and `Config::load` runs on the rustc-wrapper hot path where stat-ing an unavailable network mount could stall the compiler — the exact failure this series exists to prevent. - `shlex_split` treated every backslash as a POSIX escape, so a perfectly ordinary `credential_process = C:\tools\aws-creds.exe` became `C:toolsaws-creds.exe`. Backslash is a path separator on Windows. - `verify_write_containment` started at the parent, so an existing symlink AT the destination was never inspected. Start at the leaf via `symlink_metadata`. - The LIST total deadline was only checked before waiting, so a stall could overshoot it by a whole progress timeout. Wait for whichever comes first. Also bound retained key bytes: entry count alone does not bound memory. - An empty `KACHE_S3_BUCKET` now disables the remote consistently, instead of erroring only when `type = "s3"` was explicit while the file bucket was fine. - Revert `deny_unknown_fields` on `[cache.remote]`. It fails the whole `FileConfig` parse, which silently reverts every other setting (`key_salt`, `cache_dir`, ...) to its default — a cache-key shift is far worse than an ignored typo. - Extract the body-length guard into `verify_complete_body` so it is directly testable; over HTTP the transport rejects a short body first, so the previous test could not exercise it. The report field is documented as the backend configured at report time; it is not per-transfer, so a window spanning a backend switch carries one label.
The S3 page still stated that a non-canonical prefix is rejected; it is now normalized with a warning, and an empty prefix stores at the bucket root. Document that objects move once, and that a rejected prefix costs cache hits rather than the build. The filesystem page gains the staging-directory containment rule and the shared-directory trust boundary the write-containment check defends.
`ProfileSelectingEnv` only redirects reqsign's own in-process environment reads. A `credential_process` helper is a separate process that inherits kache's environment, so `reqsign-command-execute-tokio` spawned it with the ambient `AWS_PROFILE` still set. A helper that shells out to AWS tooling could therefore return credentials for a different account than `profile` selected, with no indication anything was wrong. Spawn the child directly and set `AWS_PROFILE` on it when a profile is configured. The process environment is still never mutated, and with no profile configured the child keeps inheriting the ambient value. This drops the last use of `reqsign-command-execute-tokio`, so the direct dependency goes too. Also record the measured blast radius of `deny_unknown_fields` in the comment explaining why it is not used: one typo'd key resolves `key_salt` to None, which shifts the cache key and rebuilds the workspace, and leaves no reason to report.
CI runs under kache-action, which exports `KACHE_S3_PREFIX=artifacts` and `KACHE_S3_REGION`. Environment overrides win over file config, so the new prefix-normalization test read `artifacts` instead of the prefix it had just written and failed on Linux, taking twelve more config tests with it through the poisoned `config_path_lock`. Not a race: the tests validated clean locally because no `KACHE_S3_*` is set here, and `--test-threads=1` also hid the resulting cascade. Reproduced with `KACHE_S3_PREFIX=artifacts cargo test`, which fails without this change and passes with it. Add `remove_env_var_for_test` and `isolate_s3_env` (restoring on drop, unlike the existing tests that remove permanently) and use them in every new test that asserts on file config. Also stop the credential-process test from mutating `AWS_PROFILE` on the whole process. It now asserts the configured profile reaches the child and that the ambient value passes through when none is configured, without writing to the environment at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up hardening for the OpenDAL remote (#593), from a review of that change after it merged.
Why this is worth landing soon
Three of these break setups that worked before #593, and the first one breaks builds rather than just cache hits.
A misconfigured remote failed every compiler invocation
load_remote_configbecame fallible in #593,Config::loadpropagates it with?, and so doesrun_wrapper_mode. A remote config error therefore meant kache exited non-zero and rustc never ran.Two configs that the pre-OpenDAL loader accepted started hard-failing on that path:
prefix = "team/"(or any leading/trailing/duplicated slash)KACHE_S3_PREFIX="", which used to mean "store at the bucket root"The reason is now recorded in
Config::remote_error, the build continues local-only, and commands that exist to talk to the remote surface it throughrequire_remote().kache statusreportsMISCONFIGUREDwith the reason instead of claiming no remote was configured.Legacy prefixes are normalized instead of rejected
Non-canonical prefixes normalize and log a warning. Only backslashes and
./..segments stay errors, because there is no safe normalization for those.Normalization moves objects:
prefix = "team/"previously wroteteam//v3/...and now writesteam/v3/..., so the remote repopulates once. That is a deliberate trade against the alternatives of failing the build or disabling the remote entirely. Cache keys are content hashes and the layout layer verifies blake3 per file, so sharing a namespace across two spellings of the same prefix cannot serve a wrong artifact.An empty prefix addresses the bucket root, which needed
join_remote_keyso keys do not gain a leading/. Without that, the config parsed but every operation failed key validation.An empty
KACHE_S3_BUCKETselected a different bucket.filter(|v| !v.is_empty())made a present-but-empty override fall through to the file-configured bucket, so a job that neutralized its remote silently got the one from the config file. It now disables the remote.The S3 client had no connect or request timeout
Only
pool_idle_timeoutwas set, which just reaps idle pooled connections. An endpoint that accepted a connection and then went silent hunghead/get/putforever, andRetryLayercould not fire because no error was ever produced. On the wrapper path that looks like a hung build.Restored a 3.1s connect timeout, matching
SDK_DEFAULT_CONNECT_TIMEOUTfrom theaws-configthis replaced, plus a 30s read-inactivity deadline. Not a total-request timeout, which would cap large transfers on slow links.Transport and filesystem
credential_processis re-lexed with shlex-style rules and executed directly, replacingsh -c/cmd.exe /C. reqsign parses the configured command withcommand.split_whitespace()and does not strip quotes, so quote characters arrive inside the tokens and the quoting does have to be reapplied. Doing it with a shell also expands globs,$(...)and%VAR%, treats&|^as operators, and undercmd.exedoes not regroup single quotes at all. Known limitation: reqsign's split has already collapsed runs of whitespace inside quoted arguments, and no amount of re-lexing here can recover that.credential_processchild now receives the selectedAWS_PROFILE.ProfileSelectingEnvonly redirects reqsign's own in-process reads, but the helper is a separate process inheriting kache's environment, so it saw the ambient profile. A helper that shells out to AWS tooling could return credentials for a different account thanprofileasked for, silently. The child is spawned directly withAWS_PROFILEset on it alone; the process environment is still never mutated. This removed the last use ofreqsign-command-execute-tokio, so that direct dependency is gone.openat2(RESOLVE_BENEATH)to close.getrejects a body shorter than its advertised length. Over HTTP the transport catches this first; the check covers a backend that ends the stream cleanly, such as the filesystem stat-then-read race.atomic_write_diris rejected when inside the object tree (staging files would list as cached objects) or on another filesystem (rename fails with EXDEV on every write). The EXDEV check runs at backend creation, not inConfig::load, so it never stats an unavailable mount on the wrapper path.Smaller items
network.configured_backend, so a consumer can tell whether the HTTP-named metrics describe a network or a local mount. It reflects the backend configured when the report is generated, not per transfer, and says so.default-features = falseon the remaining OpenDAL crates. The resolved graph is unchanged; the manifest now matches the stated policy.sync --workspacevalidates the workspace before resolving credentials, so it no longer prompts for SSO and then reports there are no members.Deliberately not done
deny_unknown_fieldson[cache.remote]looked attractive but is a trap. Serde fails the wholeFileConfigparse on an unknown key, so a single typo takes every other setting with it. Measured withkey_salt = "from-file"in[cache]and a typo'dbukcetunder[cache.remote]:Silently rebuilding the world because of one typo is a worse outcome than ignoring the typo, and the config does not even end up with something to show in
kache status. Reporting unknown keys properly needs remote parsing isolated from the rest of the config.Validation
cargo test -q -p kache --locked(1333 unit tests plus all integration suites)cargo clippy -q -p kache --all-targets --locked -- -D warningscargo fmt --all -- --checkcargo deny check bans licenses sourcesEvery regression fix has a test that fails without it. The symlink containment test was checked red by disabling the guard, so it is not a vacuous gate.