diff --git a/.config/nextest.toml b/.config/nextest.toml index 8975b3c85..23277db44 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -67,13 +67,39 @@ success-output = "immediate" # default budget with room to spare: 4.2s and 7.6s on runs 34075197897 and # 34079222917, against a 244.0s median before. # -# Removing that second Cargo build also unblocked this one. The two used to run -# concurrently, each with four compile jobs on a four-vCPU runner, and each -# roughly halved the other; on those same two runs this test took 125.3s and -# 170.6s rather than the 274.7s median above. 420s is therefore sized against -# the older, contended distribution and is deliberately conservative while the -# new shape has two samples. It is a candidate for tightening, or for deletion, -# once ten runs have accumulated under it. +# Removing that second Cargo build also unblocked this one. Contention had been +# increasing each test's elapsed time: the two ran concurrently, each with four +# compile jobs on a four-vCPU runner, and each roughly halved the other. With +# the second build gone from the Windows lane, this test took 125.3s, 170.6s +# and 170.7s on runs 34075197897, 34079222917 and 34080385050, rather than the +# 274.7s median above. 420s is therefore sized against the older, contended +# distribution and is deliberately conservative while the new shape has three +# samples. +# +# Trimming the test is worth about 85s, not the 156s that figure implied +# before the second build left, on the uncontended reading those three runs +# support: what a trim returns there is the test's exclusive tail, the period +# after every other test has reported, measured at 62.3s, 87.2s and 85.5s, +# where 156s was the implied value of trimming both tests while they +# contended. The lane fell from a 1468s median to about 848s across the three +# changes, so 85s is roughly ten percent of what remains. +# +# That figure has since moved. This test is a member of `nested-cargo-builds` +# above, which serializes it with the other build-capable child Cargo tests, so +# it holds the group's single slot and every member behind it waits. Trimming +# it therefore returns its group occupancy rather than its exclusive tail, and +# the test's own duration caps that figure, because the duration is the +# occupancy it gives back. The trim returns exactly the duration whenever the +# shortened group chain still bounds the run. The Windows runs sampled on +# 2026-09-18 put the figure between 95s and 163s, against the 85s above, which +# was the exclusive tail under an uncontended lane that no longer exists. The +# repository has still decided not to spend the fidelity risk yet, and the trim +# remains a candidate for tightening or deletion at the ten-run revisit gate +# that ADR-028 defines; that record carries the serialized measurements, the +# alternatives already measured and rejected — `cargo check` for `cargo +# build`, warming the compiler cache, and sharing a target directory — and the +# ten-run gate. The developers' guide carries the constraints a fixture-crate +# replacement would have to preserve. # # Windows only. On Ubicloud both tests finish in a fraction of the budget, and # widening the timeout there would blunt the hang detection this file exists to diff --git a/docs/adr-028-defer-split-build-dir-harness-trim.md b/docs/adr-028-defer-split-build-dir-harness-trim.md new file mode 100644 index 000000000..7606507e7 --- /dev/null +++ b/docs/adr-028-defer-split-build-dir-harness-trim.md @@ -0,0 +1,257 @@ +# Architectural decision record (ADR) 028: Defer replacing the split-build-dir harness test + +## Status + +Accepted. + +## Date + +2026-09-19 + +## Context and problem statement + +The decision this record captures was taken on 2026-09-17, when the +`nested-cargo-builds` serialization group landed and changed what a trim could +return. The measurements it rests on were taken up to 2026-09-18, and the +sections below say where each figure came from rather than dating the record by +it. + +`harness_compiles_under_a_split_build_dir` is the regression test for the +Windows `CreateProcessW` command-line limit. It forces a split layout with its +own private `CARGO_TARGET_DIR` and `CARGO_BUILD_BUILD_DIR` roots, confirms the +collected dependency directories span the split, and compiles a fixture against +them. Every `-L dependency=` pair it produces is required to avoid `E0463`, so +the list cannot be shortened; it moves off the command line entirely into a +`rustc` response file. That is the behaviour under test. + +Because its roots are private — sharing the ambient target directory would race +the `#[once]` `test_support_rlib` fixture and fail with version-skew errors +(`E0460`) — the test compiles `test_support` and roughly 350 dependencies from +scratch. On the four-vCPU GitHub-hosted `windows-latest` gate, that build is +98.8% of the test's wall time, which makes it the most expensive test in the +lane and an obvious target for trimming: build a minimal fixture crate under +the split layout instead of the real `test_support`. + +This record exists because that obvious target was measured, and the +measurement did not support spending the fidelity risk when the ticket that +proposed it assumed. + +The figure has moved twice, in both cases because the system around the test +changed rather than the test itself. + +Before [#687](https://github.com/leynos/netsuke/pull/687), the two +isolated-Cargo tests ran concurrently, each with four compile jobs on a +four-vCPU runner, so each roughly halved the other. In that contended shape +trimming **both** tests was worth 156s. Once +`packaged_manifest_retains_build_script_sources` left the Windows lane, this +test got faster without being touched, and the implied value of trimming it +fell to about **85s** — measured as its exclusive tail, the period after every +other test had reported. + +A serialization group then changed the arithmetic a second time. A later change +to `.config/nextest.toml` added `nested-cargo-builds`, a `[test-groups]` entry +with `max-threads = 1`, which puts this test in a group with the other tests +that spawn a build-capable child Cargo command. It landed for the coverage +lane's benefit: four nextest workers each starting a four-job child Cargo build +on a four-vCPU runner is what the group exists to prevent. The Windows lane runs +`make test` with no `NEXTEST_PROFILE`, so it selects `[profile.default]` and +inherits the same group. + +Under that group the test is no longer merely a slow finisher. It is a **serial +link**: every other member waits while it holds the single slot, and the chain +cannot finish until it releases it. Removing it therefore returns its whole +group occupancy rather than its exclusive tail. + +The question this record answers is therefore not whether the test is expensive +— it is — but whether the fidelity risk of replacing it is currently worth +paying, given that the number has never been stable enough to plan against. + +## Decision drivers + +- The test guards a Windows-specific failure + (`Os { code: 206, kind: InvalidFilename }`) that cannot be reproduced on most + local hosts, so weakening it silently is the most expensive possible outcome. +- The figure the decision rests on has moved twice, both times because the + lane changed rather than the test. A decision taken against an unstable + number reopens on its own. +- The repository has already spent effort making this lane cheaper by other + means. The lane fell from a 1468s median to about 848s across + [#687](https://github.com/leynos/netsuke/pull/687), [#690](https://github.com/leynos/netsuke/pull/690) + and [#691](https://github.com/leynos/netsuke/issues/691), so the remaining + saving is roughly ten percent of what is left. +- Any replacement owes coverage that is not the group's rationale to supply. + The response-file pressure this build generates is a separate requirement + from the serialization the group provides. + +## Options considered + +### Option A: Replace the test with a minimal fixture crate + +Build a one-dependency crate under the split layout in place of `test_support`, +keeping the private roots and the split-directory assertion but dropping the +roughly 350-dependency compile. + +This returns most of the figure, because the test is the group's heaviest +member: removing it shortens the chain for every member behind it. It also +drops coverage in two ways, neither of which is optional. A one-dependency +fixture still exercises the split-directory derivation, but no longer covers it +at the real crate's scale, nor against the proc-macro and dynamic-library +artefacts that make the directory enumeration non-trivial. And it produces far +fewer `-L dependency=` entries, so it stops exercising the response-file path +that the test exists to protect. + +### Option B: `cargo check` instead of `cargo build` + +Timed cold at `-j 4` on a 32-core host: 114s against 102s, twelve percent. It +also writes nothing into the target directory, so the uplift the regression +exists to catch stops happening and the test passes vacuously. + +### Option C: Warm the lane's compiler cache + +The cache already reaches the spawned build, because `ci-windows.yml` sets +`RUSTC_WRAPPER` at job scope and the test adds to the child environment rather +than clearing it. Warming it is worth about three percent: 281.0s on the cold +run against a 271.9s warm median. There is no reuse left to claim. + +### Option D: Share a target directory + +The test needs private roots to avoid racing the `#[once]` fixture with +`E0460`, so its build cannot reuse the lane's artefacts or the other test's. + +### Option E: Keep the test and defer the decision to a gate + +Leave the test, its subject and its budget as they are, and record the +conditions under which the question is worth reopening. + +| Topic | A: fixture crate | B: `cargo check` | C: cache | D: shared target | E: defer | +| ------------------------- | ---------------- | ---------------- | -------- | ---------------- | -------------- | +| Returns the figure | Most of it | ~12%, then none | ~3% | None | No | +| Keeps split-dir coverage | At reduced scale | Vacuously | Yes | Yes | Yes | +| Keeps response-file path | No | No | Yes | Yes | Yes | +| Reproduction on this host | Yes | Yes | Yes | Fails `E0460` | Not applicable | + +_Table 1: Comparison of the measured options._ + +## Decision outcome + +**Option E.** The repository does not trim +`harness_compiles_under_a_split_build_dir` yet. The test keeps its real +`test_support` subject and its 420s budget, and the trimming question reopens +at the revisit gate below rather than on a schedule. + +The trim is deferred, not closed. The Windows lane keeps the real +`test_support` build, and the question reopens when the gate is met. + +## Rationale + +What a trim returns is the test's whole group **occupancy**, because the +duration _is_ the occupancy it gives back. That gives the figure both its +ceiling and its shape: **a trim can never return more than the test's own +duration**, and it returns exactly that whenever the shortened group chain is +still what bounds the run. It returns less only when unrelated non-group work +becomes the run's next binding constraint once the harness is gone. + +The harness is never the last test to finish. The group's cheap tail members +cannot start until it frees the slot, so they necessarily finish after it. The +mechanism is what generalizes past the sample; the sample is its evidence. +Measured from the same `build-test-windows` job logs, over the Windows runs +available on 2026-09-18: + +| Run | Test duration | Group chain end, trim applied | Trim returns | +| ----------- | ------------- | ----------------------------- | ------------ | +| 35266003414 | 152.0s | 162.4s | 152.0s | +| 35266979317 | 149.0s | 178.3s | 149.0s | +| 35272793454 | 124.7s | 134.9s | 113.4s | +| 35400200137 | 115.7s | 154.1s | 95.0s | +| 35403273264 | 141.6s | 154.4s | 136.3s | +| 35405043577 | 141.9s | 167.1s | 141.9s | +| 35407132087 | 162.9s | 174.6s | 162.9s | + +_Table 2: The harness test as a serialized group member, after the group +landed._ + +The rightmost column is the whole-run saving: the run's own end, less whichever +of the trimmed group chain and the last non-group test finishes later. Four of +the seven runs return the test's full duration; the other three return less, at +113s against 125s, 95s against 116s, and 136s against 142s. So the figure +tracks the test's own cost and moves with it, which is why the sample's +durations span 115.7s to 162.9s while its savings span 95s to 163s. + +The 85s reading is not a floor this settles back to. It was the exclusive tail +under an uncontended lane that no longer exists, and the group's arrival is +what retired it. + +Two cautions belong with that table, and they are why the decision is to defer +rather than to proceed on a larger number. The sample is small and it is not a +uniform one: it mixes trunk pushes with pull-request lanes, which start from +different tree states, so it sets an order of magnitude rather than a value. +And the group's own scheduling, not the test alone, produces the chain ends, so +those figures are readings of a serialized system rather than isolated +measurements of the test. + +The strongest argument for deferring is not the size of the number. It is that +the number has moved twice for reasons outside the test, so a decision taken +against it would be a decision taken against the lane's current shape. The +fidelity risk, by contrast, is real and one-directional: the coverage a fixture +crate would drop is exactly the coverage that fails only on Windows, where it +is least likely to be noticed. + +## Revisit gate + +Wait until **ten runs** of the split Windows lane exist under the serialization +group. That is enough for the harness test's share of the `build-test-windows` +job to be known under the shape that now exists, rather than estimated from the +sample above. + +The criterion has moved with the evidence. With the group in place the test +holds a serial slot, so the question is no longer whether its exclusive tail +has settled below 85s — it plainly has not. The question is whether the run +still ends when the test ends. If the group chain stops being what bounds the +run, or if the `Test` step stops being the lane's critical path, then the trim +is not worth the fidelity risk and the work closes without it. + +## Consequences + +- The Windows lane keeps its most expensive test, and the group keeps its + heaviest member. Every other member of `nested-cargo-builds` waits behind + that member, so the serialization the group provides costs the lane more than + the test's own duration. +- A future trim is not blocked, only deferred. Option A remains available and + its requirements are recorded in the developer's guide, which owns them: the + fidelity argument a replacement owes in a doc comment beside the test, and the + `rustc` response-file pressure it must either keep generating or move into a + dedicated test. +- The 420s budget stays sized against the older, contended distribution. It is + conservative by roughly a third against the measured 312.9s worst case, and + it remains a candidate for tightening or deletion once the gate is met. +- Any future change that removes the test from the lane also removes the + group's heaviest member, which changes every other member's scheduling. That + is a lane-wide effect, not a local one, and belongs in the decision that + takes it. +- Because the estimate is recorded as a mechanism with a dated sample rather + than a range, later runs do not falsify it. They belong to the revisit gate. + +## Related decisions + +- [ADR-011: Serial `deps` ordering via Ninja dyndep][adr-011] is the other + decision in this repository that trades throughput for ordering guarantees. +- [ADR-025: Persistent coverage data owned by `main`][adr-025] governs the + coverage lane whose contention the `nested-cargo-builds` group was added to + prevent. + +## References + +- [#673](https://github.com/leynos/netsuke/issues/673) measured the Windows + lane. +- [#687](https://github.com/leynos/netsuke/pull/687) relocated the packaging + verification build and so changed this test's cost. +- [#690](https://github.com/leynos/netsuke/pull/690) folded the native-recipe + smoke job into the gate job. +- [#691](https://github.com/leynos/netsuke/issues/691) split the Windows lints + from the tests. +- [Developer guide: Windows budget for the isolated-Cargo-build tests and what + a fixture-crate replacement would have to preserve][dev-guide]. + +[adr-011]: adr-011-use-ninja-dyndep-for-serial-dependency-ordering.md +[adr-025]: adr-025-main-owned-coverage-publication.md +[dev-guide]: developers-guide.md#what-a-fixture-crate-replacement-would-have-to-preserve diff --git a/docs/contents.md b/docs/contents.md index 7d78a7988..7bfc208f2 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -170,6 +170,9 @@ operator, user, and contributor references are easier to find. - [ADR-026](adr-026-manifest-environment-access-policy.md): Exact-name manifest environment policy evaluated before the reader, with project allow entries quarantined below the operator ceiling. +- [ADR-028](adr-028-defer-split-build-dir-harness-trim.md): Deferred trim of + the split-build-dir harness test, with the serialized-lane measurements that + made the figure unstable and the ten-run gate that reopens the question. ## Proposals diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 6b35e0fdf..e62cbd907 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2912,6 +2912,8 @@ Linux runner. Both platforms still assert the packaged file list. See [Windows budget for the isolated-Cargo-build tests][windows-test-budget]. [windows-test-budget]: #windows-budget-for-the-isolated-cargo-build-tests +[fixture-constraints]: #what-a-fixture-crate-replacement-would-have-to-preserve +[adr-028-trim]: adr-028-defer-split-build-dir-harness-trim.md `tests/workflow_contracts/test_execution_coverage_test.py` holds all of this: the coverage inputs, the denied warnings, the doctest pass and its position, @@ -2999,22 +3001,64 @@ budget, which clears the measured 312.9s worst case by 34%. Removing the second Cargo build sped this one up as well. The two used to run concurrently, each with four compile jobs on a four-vCPU runner, so each -roughly halved the other. Measured on runs 34075197897 and 34079222917, the -first two under the new shape: +roughly halved the other. Measured on runs 34075197897, 34079222917 and +34080385050, the first three under the new shape: Table: Windows durations before and after the verification build moved. -| Measure | Before (median) | Run 34075197897 | Run 34079222917 | -| ------------------------------------------------ | --------------- | --------------- | --------------- | -| `harness_compiles_under_a_split_build_dir` | 274.7s | 125.3s | 170.6s | -| `packaged_manifest_retains_build_script_sources` | 244.0s | 4.2s | 7.6s | -| nextest run phase | 365s | 185.5s | 258.0s | -| `Test` step | 471s | 260s | 368s | +| Measure | Before (median) | Run 34075197897 | Run 34079222917 | Run 34080385050 | +| ------------------------------------------------ | --------------- | --------------- | --------------- | --------------- | +| `harness_compiles_under_a_split_build_dir` | 274.7s | 125.3s | 170.6s | 170.7s | +| `packaged_manifest_retains_build_script_sources` | 244.0s | 4.2s | 7.6s | 6.8s | +| nextest run phase | 365s | 185.5s | 258.0s | 253.8s | +| `Test` step | 471s | 260s | 368s | 358s | The 420s budget is therefore sized against the older, contended distribution -and is deliberately conservative while the new shape has two samples. It is a -candidate for tightening, or for deletion, once ten runs have accumulated under -it. +and is deliberately conservative while the new shape has three samples. It is a +candidate for tightening, or for deletion, once the [ADR-028][adr-028-trim] +revisit gate is met. + +Those three samples predate the serialization group described below, which +lands the harness test in a group of one-at-a-time build-capable tests. Under +that group the test is a serial link rather than a slow finisher, and the trim +is worth more than the 85s this table supports. + +#### Deferring the split-build-dir harness trim + +The repository has decided **not** to trim +`harness_compiles_under_a_split_build_dir` yet. The test keeps its real +`test_support` subject and its 420s budget. + +A later change to `.config/nextest.toml` — `nested-cargo-builds`, a +`[test-groups]` entry with `max-threads = 1` — put this test in a group with +the other tests that spawn a build-capable child Cargo command, so only one of +them runs at a time. It landed for the coverage lane's benefit: four nextest +workers each starting a four-job child Cargo build on a four-vCPU runner is +what the group exists to prevent. The Windows lane runs `make test` with no +`NEXTEST_PROFILE`, so it selects `[profile.default]` and inherits the same +group. Under it the test is a serial link rather than a slow finisher, and a +trim returns its whole group occupancy rather than only the exclusive tail the +85s above measures. That raises the ceiling on the saving to the test's own +duration, and it makes the saving track the test's own cost rather than an +uncontended lane's tail. + +The decision does not change with the number, and it is not the number that +settles it. The figure has moved twice already, both times because the lane +changed rather than the test, so a decision taken against it would be a +decision taken against the lane's current shape. The fidelity risk, by +contrast, is one-directional: the coverage a fixture crate would drop is +exactly the coverage that fails only on Windows, where it is least likely to be +noticed. + +[ADR-028][adr-028-trim] holds the decision itself: the measurements, the rule +that a trim can never return more than the test's own duration, the +alternatives already measured and rejected (`cargo check` for `cargo build`, +warming the compiler cache, and sharing a target directory), and the ten-run +revisit gate. Read it before reopening the question or changing the Windows +shape of this lane. Any replacement built after that gate inherits the +constraints in +[what a fixture-crate replacement would have to preserve][fixture-constraints] +below. ### How this relates to the isolation utilities @@ -4551,7 +4595,51 @@ That private build is why this test is the most expensive one on the Windows gate: `test_support` depends on `netsuke-build`, so a private root means compiling that crate and roughly 350 dependencies from scratch. Its measured budget is recorded in -[Windows budget for the isolated-Cargo-build tests][windows-test-budget]. +[Windows budget for the isolated-Cargo-build tests][windows-test-budget]. The +[decision to defer a trim][adr-028-trim] and the gate at which it is revisited +are recorded in ADR-028. + +#### What a fixture-crate replacement would have to preserve + +This section is the constraint list for a future attempt, not a plan. The +decision to defer, and the gate that reopens it, are in [ADR-028][adr-028-trim] +; nothing here is built while the trim is deferred. + +If the trim is taken up after that gate, the obvious shape is a minimal fixture +crate built under the split layout in place of `test_support`. It needs at +least one dependency, so that dependency rlibs land in the split build +directory while the fixture's own uplifted rlib lands in the target directory. +That is precisely the arrangement the regression exists to catch: a single +derived `-L dependency=` directory that missed the dependencies entirely. This +section records what such a replacement must carry; nothing here is built while +the trim is deferred. + +**The fidelity argument.** The current test is a regression test for a defect +that was found once, and its subject is the real `test_support` build. Swapping +that subject for a stand-in weakens the test unless the argument for the swap +is explicit, in a doc comment beside the test, about exactly which regression +it still guards and what it no longer covers. A one-dependency fixture does +exercise the split-directory derivation — dependency artefacts in the build +directory, uplifted artefacts in the target directory — but it no longer covers +that derivation against the real crate's roughly 350-dependency scale, nor +against the proc-macro and dynamic-library artefacts described above. Those are +what makes the directory enumeration non-trivial, and the comment must say so +rather than let the coverage drop silently. + +**The Windows response-file pressure.** `TestSupportRlib::compile` passes its +arguments through a `rustc` response file, and the reason is a Windows command +line limit rather than a style choice. Cargo 1.99 gives every crate its own +artefact directory, so the `-L dependency=` set holds one entry per dependency; +this test adds long temporary roots on top of that. Passed directly, the result +exceeds the Windows `CreateProcess` command-line limit and the spawn fails with +`Os { code: 206 }` before `rustc` runs at all. A fixture crate with one +dependency produces far fewer directories and would stop exercising that +pressure, which is a measurable loss of coverage however cheap the fixture +becomes. So a replacement must either generate enough search paths to keep the +`@file` path genuinely exercised, or move the response-file contract into its +own dedicated test. Either way the doc comment above the replacement must say +which of the two it does, because the failure it guards is Windows-specific and +cannot be reproduced on most local hosts. ### Manifest `env()` reader diff --git a/tests/locale_stub_ui_tests.rs b/tests/locale_stub_ui_tests.rs index d933e2707..7cc377305 100644 --- a/tests/locale_stub_ui_tests.rs +++ b/tests/locale_stub_ui_tests.rs @@ -84,6 +84,32 @@ fn stub_env_builders_compile_under_the_same_harness( /// the collected `-L dependency=` set has to span the split for the control /// fixture to compile. This pins the regression where a single derived /// directory missed the dependencies entirely. +/// +/// The subject is the real `test_support` build, not a fixture crate, and that +/// is deliberate: it is what carries both the dependency artefacts and the +/// uplifted one that the split-directory derivation has to tell apart. The +/// cost of building it here is recorded in +/// docs/developers-guide.md, and the decision to defer trimming it, the gate +/// that reopens the question, and the fidelity argument any fixture-crate +/// replacement would owe are in ADR-028 +/// (docs/adr-028-defer-split-build-dir-harness-trim.md). +/// +/// This test is a member of the `nested-cargo-builds` nextest group, so on +/// Windows it holds that group's single slot: every other build-capable test +/// waits for it, so a trim returns its whole occupancy rather than only the +/// tail it finishes on, whenever the shortened group chain still bounds the +/// run. It returns less when unrelated work becomes the run's next binding +/// constraint once the slot frees. The group's measurements are in the same +/// developers' guide section. +/// +/// It is also what keeps the Windows response-file path exercised. The long +/// `-L dependency=` set this build produces, plus the long temporary roots the +/// test adds, is why `TestSupportRlib::compile` sends its arguments through a +/// `rustc` response file at all rather than a command line. A fixture crate +/// with one dependency would produce far fewer directories and stop +/// exercising that, so any replacement must either generate enough search +/// paths to keep the pressure or move the response-file contract into its own +/// dedicated test. #[rstest] fn harness_compiles_under_a_split_build_dir() -> io::Result<()> { let subscriber = tracing_subscriber::fmt().with_test_writer().finish();