build: tune the release profile, drop the no-op feature flag, and share test helpers - #8
Open
ctxswitch wants to merge 1 commit into
Open
Conversation
…re test helpers The release profile was never set, so a performance-sensitive server shipped untuned; it now uses thin LTO and a single codegen unit. CARGO_FEATURES was --all-features against a crate with no [features] table, a no-op on every target. AGENTS.md claimed Cargo auto-discovers the integration tests, but each is an explicit [[test]] target because they live in a subdirectory. Test scaffolding: the oneshot call wrapper duplicated across four integration suites moves to a shared common module, the bind-and-serve block repeated eight times in cacheprog_test.rs becomes one serve helper, and the SpacePolicy literal repeated four times in space_test.rs becomes ledger_from. Deletes foreground_get_waits_for_the_inflight_prefetch_download, which the queued variant that follows it strictly subsumes: begin_download runs before the semaphore acquire, so both tests observe the same entry through the same wait, and the queued one additionally asserts the get actually blocked. Also writes PrefetchObservation's completion flag through a method. Drop reads it, so the behaviour was already correct -- confirmed by driving a body to completion and asserting the recorder sees completed -- but a newer rustc reads the direct field assignment in the stream's final arm as a dead store, which would fail clippy -D warnings on the toolchain the container builds with.
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.
Summary
Final phase of six. Based on
cleanup/phase-5-config(#7).Build. Added
[profile.release]withlto = "thin"andcodegen-units = 1— aperformance-sensitive server was shipping untuned. Dropped
CARGO_FEATURES ?= --all-featuresand its six expansions from theMakefile; the crate has no[features]table, so it was a no-op on every target, and thebuildhelp text thatpromised "every feature enabled" went with it.
Docs.
AGENTS.mdclaimed Cargo auto-discovers the integration tests. It does not —they live in a subdirectory, so each is an explicit
[[test]]target inCargo.toml,and a new file needs a new entry. Corrected, with a note on how the new shared-helper
module is included without becoming a target itself.
Test scaffolding. The oneshot
call/requestwrapper duplicated acrosschannels.rs,build_cache.rs,local_artifacts.rs, andmaintenance.rsmoves totests/integration/common/mod.rs. The bind-and-serve block repeated eight times incacheprog_test.rsbecomes oneserve(app)helper. TheSpacePolicyliteral repeatedfour times in
space_test.rsbecomesledger_from(source).ring_test.rsloses adangling comment and gains an honest test name.
Also folds in one cross-phase fix, described under Review notes.
Verification
make ciclean, includingclippy --all-targets -- -D warnings.docker build .succeeds — this was the end-of-sweep check that the releaseprofile and the dependency removal from an earlier PR in this stack do not break the
release build.
Assertion accounting across the scaffolding changes: test and assertion counts are
byte-identical before and after in every touched file except
cacheprog_test.rs,which drops exactly one test and its three assertions — the deliberate deletion below.
make ciOperational impact
Release binaries are now built with thin LTO and a single codegen unit: slower builds,
faster binary. No runtime behaviour, configuration, storage format, route, or metric
changes.
Review notes
One deletion, verified before removing it.
foreground_get_waits_for_the_inflight_prefetch_downloadwas deleted as subsumed bythe queued variant that follows it. The subsumption is genuine, not assumed:
begin_downloadruns at enqueue time, beforelimit.acquire_owned(), so "activelydownloading" and "queued behind the semaphore" are the same entry in the same
inflightmap, and the foreground get performs the same singledownload_in_progresslookup,pending.changed().await, and disk recheck for both.The queued test exercises the strictly harder precondition and carries an assertion
the deleted one lacked — a
releasedflag proving the get actually blocked ratherthan racing through.
Two planned items were already resolved by earlier PRs in the stack, and correctly
changed nothing. The stale doc comment in
proxy/mod.rswent out with the functionit was glued to. And
docs/operations.mdneeded no metric updates: every stringliteral in
src/telemetry.rswas diffed between the base commit and HEAD, yieldingonly six new literals, all
tracingfield names and messages. The agent metrics lookchanged under a name grep only because they moved from
format!("flywheel_agent_ {suffix}")to full literals; extracting the (name, help) pairs at both revisionsgives 19 identical pairs in identical order.
One fix from outside this phase's scope is included, deliberately. The container
build surfaced
warning: value assigned to observation is never readinobserve_prefetch_body, which the earlier consolidation introduced. It looked like amiscounting bug — completed transfers recorded as cancelled — so it was tested rather
than assumed: driving a body to completion through
observe_prefetch_bodywith aprobe recorder shows the recorder receives
completed = true.Dropreads the field,so the metric was always correct and the warning is a false positive.
It is still worth fixing, because it is a latent CI break: local
rustcis 1.91.0 andthe container's is 1.97.1, and the expanded
unused_assignmentslint only fires on thenewer one — so
clippy -- -D warningswould fail on a current toolchain even thoughit passes here. The flag is now written through a
complete()method, which is whatthe original plan specified anyway. Folded into this PR rather than sent back through
the stack, since rebasing five stacked branches to place a three-line change would
cost more review than it saves; cherry-pick it onto #5 instead if you would rather it
sat with the code it belongs to.
Noted, not fixed:
tests/integration/package_proxies.rshas four morecall-familyhelpers and two inline
.oneshot(...)sites that could sit oncommon::call. It wasnot on this phase's list; the module is in place for a follow-up.