fix(ci): verify APT packages are installed instead of trusting the cache - #7632
Merged
Conversation
`awalsh128/cache-apt-pkgs-action` reports which packages are present by
reading `manifest_main.log` out of the restored cache. The manifest is data
the cache carries, not an observation of the machine, so an entry saved
without its package payload restores as a "Cache hit" that installs nothing
and exits 0.
Measured on run 33648341770 (job 100316791350), key
cache-apt-pkgs_fe10c55f565538c3338c7fd7f037618c at 1229 B:
Cache hit for: cache-apt-pkgs_fe10c55f565538c3338c7fd7f037618c
Cache Size: ~0 MB (1229 B)
Found 3 files in the cache.
- cache_key.md5
- install.log
- manifest_main.log
Reading from main requested packages manifest...
- libacl1-dev=2.3.2-1build1.1
Three metadata files, no payload, every package announced as installed. The
failure surfaces later and elsewhere - upstream rsync stops linking:
/usr/bin/ld: cannot find -lacl: No such file or directory
/usr/bin/ld: cannot find -lxxhash: No such file or directory
Without the oracle binary the testsuite cells that need one report `got skip`
rather than their expected outcome, and a required context goes red for a
reason unrelated to the commit under test.
Deleting a poisoned entry does not fix it - the entry is re-saved by the next
run down the same path. Key fe10c55f... was deleted and re-created about ten
minutes later on two refs, at 1214 B and 1220 B; master and other PRs carry
their own sub-1.3 KB entries. A ~1.2 KB entry is the tell.
So the cache is treated as an optimisation and never as the thing that makes
the packages present. tools/ci/ensure_apt_packages.sh asks dpkg what is
actually installed and installs whatever is missing, emitting a ::warning so
a payload-less restore is visible rather than silent. It re-checks with dpkg
afterwards because `apt-get install` can exit 0 having skipped a package it
could not resolve.
The package list moves to a workflow-level `APT_PACKAGES` consumed by both
the cache step and the verify step, so there is exactly one definition per
workflow - a copied list is what drifts.
Verified on Linux, all three branches: all-installed is a no-op at exit 0 for
both `$APT_PACKAGES` and `"$APT_PACKAGES"`; with one absent package and a
stubbed sudo that exits 0, the script still reports the package missing and
exits 1, so the re-check does not inherit apt-get's verdict.
…ercise
The script's whole point is the repair path, and that is exactly the path a
workflow run cannot be relied on to reach: whether the restored cache is
poisoned on any given run is not something the run controls. GitHub scopes
caches to the ref that created them plus the default branch, so a PR whose key
exists only on a *different* PR's ref simply misses, installs for real, and
leaves the verify step a no-op. A green workflow proves the step is wired; it
does not prove it repairs.
Five cases, with `dpkg-query` and `sudo` stubbed on PATH so the tests install
nothing, need no root, and behave identically on a developer machine and a
runner:
- all installed -> no-op, exit 0, apt-get never invoked
- quoted vs unquoted -> `$APT_PACKAGES` and `"$APT_PACKAGES"` agree
- one missing -> installed, and only the missing one
- install exits 0 with
nothing installed -> still reported missing, exit 1
- no arguments -> usage error, exit 2
The fourth is the one that matters. `apt-get install` can exit 0 having
skipped a package it could not resolve, so a script inheriting its verdict
would report success there; re-asking dpkg is what makes the guard fail
closed, and this is what pins that it keeps doing so.
Non-vacuity is not assumed: the first draft of the dpkg stub read the package
name from the wrong argument index and three of the five failed, which is the
evidence that they exercise the script rather than passing regardless.
tools/tests/ runs blocking in CI via run_tools_tests.sh with pattern discovery
and a non-zero count assertion, so this needs no workflow change: 57 -> 62.
oferchen
added a commit
that referenced
this pull request
Sep 2, 2026
… merged PRs (#7634) * docs(rustdoc): reach the public items rustdoc could not see, and gate the class Two halves of the same question - is what a public item says actually reachable from `cargo doc`? Half 1: seven `//` comment blocks documented a `pub` item, so their text never reached the generated docs. Found positionally, not by content: a `//` run whose next non-attribute line declares a `pub` item. A census keyed on comment CONTENT is false-positive dominated; this one is mechanical. Nine sites matched; two are DELIBERATELY left as `//`, because they document the `#[cfg_attr]` / `#[allow]` attribute directly beneath them rather than the item - `local_copy/overrides.rs` and `local_copy/plan/report.rs`. Promoting those would attach a lint rationale to a public surface as if it described the surface. Text is preserved verbatim, upstream citations included (`generator.c:2149` on the obstacle module). The only rewrite is `socket_options/consts.rs`, where one group note covered two constants: split per item, matching the file's own house style where every other const already carries `///`. Half 2: five intra-doc links could not resolve in a DEFAULT build, and the existing gate is structurally unable to see them. crates/fast_io/src/lib.rs, ewma.rs (x4) -> `adaptive_dispatch`, gated by the off-by-default `adaptive-basis-dispatch` feature crates/rsync_io/.../sync_bridge.rs -> `crate::channel_adapter`, gated by the off-by-default `async-ssh` feature crates/fast_io/src/confinement.rs -> `PinnedRoot`, a PRIVATE struct plus one already on the branch in `module_list/types.rs`, where the link was `Transport::Quic` (the `quic` feature) and its surrounding prose still described the variant as unbuilt. Every crate denies `rustdoc::broken_intra_doc_links`, so each of these is a hard `cargo doc` failure, not a warning. MEASURED on the pristine base: cargo doc --no-deps --workspace -> 7 errors, `fast_io` and `rsync_io` could not be documented cargo doc --no-deps --workspace --all-features -> clean That gap is the whole point. Under `--all-features` the gated items EXIST, so the link resolves and the gate is green; in a default build the item is not compiled at all. `ci.yml`'s `rustdoc-links` job runs only `--all-features`, deliberately mirroring `pages.yml` - which is right for the question Pages asks and cannot ask this one. So the fix is paired with a SECOND step on default features, and neither subsumes the other: `crates/fast_io/src/io_uring` is `cfg(all(target_os = "linux", feature = "io_uring"))` and only the all-features run compiles it. The names stay in backticks with the reason stated at each site, rather than being deleted or the links made conditional - the prose still says what it said, and the next reader learns why it is not a link. After: both invocations document all 25 crates cleanly, 0 errors. fmt clean; clippy on the five touched crates reports no new diagnostics. * docs(changelog): record the ten pull requests merged since the last sweep #7624 brought the Unreleased section up to #7620 and then stopped being true: #7621-#7632 landed after it, of which #7624 is itself that changelog pass and #7628 was never merged. Ten entries were missing. Each is placed in the subsection its change belongs to rather than appended in number order, and #7625 is SPLIT across two, because it is two changes: wiring `--force` is a receiver fix, and the synthetic-transfer-root guard that wiring made necessary is a peer-supplied input bound. Filing it once under either heading would have hidden whichever half the reader was looking for. Security / peer-supplied input bounds #7625 (guard half), #7630 Fixed / transfer and receiver #7622, #7625 (--force half), #7627 Fixed / daemon #7629 Testing and CI #7621, #7623, #7632 Documentation #7626, #7631 Wording is taken from each merge commit's own body, not from its title - the titles compress away the part a reader needs. #7629's title says "keep a dead name converter apart from an unknown name"; what makes it worth an entry is that five outcomes collapsed into one `None` and the merge failed OPEN on exactly the mechanism an operator installs to take ownership decisions away from the peer.
oferchen
added a commit
that referenced
this pull request
Sep 4, 2026
… them (#7661) `README.md` ships the exact `awk` command that reads a leg's expected-outcome manifest and prints its pass/fail/skip counts. Running it disagrees with every row of the table directly beneath it. | row | documented | re-derived | |---|---|---| | non-root, pipe | 257 / 3 / 85 | **259 / 1 / 85** | | root, pipe | 286 / 3 / 56 | **288 / 1 / 56** | | non-root, tcp | 101 / 21 / 33 | **108 / 14 / 33** | | root, tcp | 113 / 27 / 15 | **120 / 20 / 15** | `SECURITY.md` carries the same four rows and the same two summary figures, so both files drifted together. Corrected throughout, along with: - **"3 of 345 tests currently diverge"** across the full-corpus legs. It is **one** - `filter-merge-content-echo` - and the README names it now rather than leaving the reader to count. - **"29 across all four"** distinct failures. It is **23**, and across **five** manifests, not four: the README's own glob `tools/ci/upstream-3.5.0-expect.*.txt` has always matched the macOS file its prose excluded. - **The macOS leg was missing from both tables.** It runs on every PR as `upstream-testsuite-macos` (`ci.yml`), on the full 345-cell corpus, with its own committed manifest. It is the only leg that can observe a platform-conditional divergence - one of its three remaining failures *skips* on Linux, so it had never executed in this repository's CI before the leg existed. Added as a fifth row, with its non-required status stated. - **"the required checks being the two stdio-pipe legs."** All four Linux legs have been required contexts since #7408 wired the TCP pair into PR CI; the ruleset returns ten contexts, not eight. - **`proxy protocol hosts` described as "not yet implemented"** and "still under audit" in two places. It shipped in #7648: parsed into a `ProxyProtocolPolicy` that mirrors upstream's `allow_proxy_protocol_peer()`, rejecting every peer when the trusted list is empty or unset, and warning at startup on the combination upstream warns about. - **The `MAX_PROXY_LINE_BYTES` citation** pointed at `connect/proxy.rs:344`. The constant moved and, more usefully, stopped being a typed literal: it is now `PROXY_BUF_SIZE - 1`, so the doc records the derivation rather than a line number that will drift again. The two macOS-leg rationale comments in the workflows carried the same pre-fix counts and a failure list six entries out of date. Both are recounted from the manifests. `CHANGELOG.md` stopped at #7632, leaving the 26 PRs merged since then unrecorded. Added under Security / Fixed / Testing and CI / Documentation. ## Why the numbers were wrong in a way reading could not catch Every figure here is the outcome column of a committed manifest. The previous values were transcribed once and then maintained by hand, so they decayed as fixes landed - and a reader checking the table against the prose beside it would find them perfectly consistent with each other. The check that finds this class is re-running the derivation, not re-reading the text, so that is what was done: a script recomputes all five legs and both summary figures from `tools/ci/upstream-3.5.0-expect*.txt` and asserts the two documents contain the results, with the superseded strings blacklisted so a partial edit cannot pass. It also caught a live error in this changeset - #7659 landed mid-review and flipped `operator-path-partial-dir-daemon`, taking macOS from 236/4 to 237/3 and the distinct-failure count from 24 to 23. The figures here are derived from master with that merge in place.
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
1 participant
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.
The defect
awalsh128/cache-apt-pkgs-actionreports which packages are present by readingmanifest_main.logout of the restored cache. The manifest is data the cache carries, not an observation of the machine. So a cache entry saved without its package payload restores like this — measured on run 33648341770, job 100316791350:Three metadata files, no payload. Every requested package is announced as installed, nothing is installed, and the step exits 0.
The failure then surfaces somewhere else entirely — upstream rsync stops linking:
With no oracle binary, the testsuite cells that need one report
got skipinstead of their expected outcome,overall result is 2, and a required context goes red for a reason unrelated to the commit under test.Why deleting the entry is not the fix
The entry is re-saved by the next run that takes the same path. Key
fe10c55f...was deleted and re-created about ten minutes later on two refs, at 1214 B and 1220 B;mastercarries1514e109...(1235 B) and another PR carries02d36ff9...(1173 B). A ~1.2 KB entry is the tell — a healthy one is ~226 KB.The fix
Treat the cache as an optimisation, never as the thing that makes the packages present.
tools/ci/ensure_apt_packages.shasks dpkg what is actually installed and installs whatever is missing, emitting a::warningso a payload-less restore is visible instead of silent. It re-checks with dpkg afterwards, becauseapt-get installcan exit 0 having skipped a package it could not resolve.The package list moves to a workflow-level
APT_PACKAGESconsumed by both the cache step and the verify step, so there is exactly one definition per workflow — a copied list is what drifts.11 call sites across 10 workflows, including both required contexts (
_upstream-testsuite.yml,_interop.yml).Verification
All three branches exercised on Linux:
$APT_PACKAGESunquotedAll 2 requested APT packages are installed.rc=0"$APT_PACKAGES"quotedsudostubbed to exit 0::error::APT packages still missing after installrc=1The third case is the non-vacuity proof: the stubbed
sudoexited 0, so a script that trustedapt-get's exit code would have reported success. The dpkg re-check caught it.Every workflow re-parsed with
yaml.safe_loadafter the edit.