From 8e95b52c43c1ebcdd89acfa42f343ae622b48066 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 13 Sep 2026 08:56:58 +0900 Subject: [PATCH 1/4] feat: state coverage as two targets and gate the three figures nothing read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/coverage.md published 431 registered at the top and, further down, called the target "205 services, not 431" while describing those 431 as not targeted. Both halves were written honestly at different times and the page could not be read as one claim. The summary table has been gated against the binary since Milestone 6; the target table under #the-target never was, so every number there could be wrong with CI green, and for a while every number there was. The fix is to stop pretending there is one target. Routing is a safety property: a registered service is answered at localhost:4747, an unregistered one leaves the machine and bills a real account, so the only number that satisfies it is all of them — 431 of 431, met. Depth is the separate and smaller promise, still 205, still where the 2026-09-05 demand study put it, because what that study refused was the cost of hand-building 283 services and the codegen scaffold removed that cost without producing any evidence of demand. A service count can no longer be read as a promise of fidelity because the page no longer states one number that could carry both meanings. TestPublishedTargetTableMatchesTheBinary now reads the two-axis table the way tierRow reads the tiers: the routing target is checked against plugin.DefaultRegistry.RegisteredServices(), and because the serving target is a decision rather than a measurement, it is read from the page and used as the arithmetic the third row must satisfy. The table cannot be internally inconsistent and cannot drift from the binary. CI now fails the build past 45 MiB. That is a regression ceiling and not the published figure — coverage.md measures 36.8 MiB on Apple Silicon and CI builds for linux, so a tight budget would fail on the platform difference rather than on a regression. The ceiling is named in coverage.md so the failure message points somewhere the number exists. cmd/devcloud/budget_test.go gates startup, and its first version did not. It timed Registry.Construct, which only calls the factory, and every factory in the tree is a struct literal — 84 microseconds against a 150 ms budget. The cost is in Init: S3Provider.Init does an os.MkdirAll and opens a SQLite database, and 431 of those is how a 42 ms startup becomes a second. Verified rather than assumed: injecting 8 ms per service into Registry.Init left the old gate at 79 microseconds and passing, and fails the new one at 4.0 s. It now brings every service up the way main.go does, measures 141 ms for all 431, and fails past 2 s. The ceiling is loose on purpose. The gate runs on a shared runner where a 2x reading is indistinguishable from a noisy neighbour, so it catches the regression that is real — a provider doing per-call work at startup, which arrives as 10x. It also fails if any registered service stops initializing, which main.go only warns about. The 194-model figures in sync_test.go and contributing.md predate vendoring the long tail and are now 420. The 2026-09-06 churn measurement is unchanged but says what it was taken at, since the diff a reviewer now faces is larger than anything measured there by roughly the same factor. RED/GREEN evidence: docs/testing/phase-4-review-followup.tdd.md --- .github/workflows/ci.yml | 23 +++ README.md | 2 +- changes/unreleased/Added-20260913-170500.yaml | 5 + .../Documentation-20260913-170000.yaml | 6 + cmd/devcloud/budget_test.go | 85 ++++++++++ cmd/devcloud/coverage_test.go | 66 ++++++++ cmd/devcloud/sync_test.go | 7 +- docs/README.md | 2 +- docs/contributing.md | 9 +- docs/coverage.md | 124 +++++++++----- docs/demand.md | 5 +- docs/testing/phase-4-review-followup.tdd.md | 158 ++++++++++++++++++ 12 files changed, 443 insertions(+), 49 deletions(-) create mode 100644 changes/unreleased/Added-20260913-170500.yaml create mode 100644 changes/unreleased/Documentation-20260913-170000.yaml create mode 100644 cmd/devcloud/budget_test.go create mode 100644 docs/testing/phase-4-review-followup.tdd.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b00b9f4b..38a974e5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,29 @@ jobs: CGO_ENABLED=0 go build -o dist/devcloud ./cmd/devcloud CGO_ENABLED=0 go build -o dist/codegen ./cmd/codegen + # docs/coverage.md publishes the binary size, and the single-binary, + # zero-config property is what that figure is evidence for. Nothing + # checked it, so a regression would have been found by a reader rather + # than by CI. The startup half of the same claim is gated in + # cmd/devcloud/budget_test.go. + # + # 45 MiB is a regression ceiling, not the published figure: coverage.md + # measures 36.8 MiB on Apple Silicon and this builds for linux, so the two + # are not the same number and a tight budget would fail on the difference. + # The ceiling is named in coverage.md's runtime section, so a reader who + # hits this failure finds it where the 36.8 MiB is. + # + # stat -c%s is GNU stat: this job runs only on the ubuntu runners above. + - name: Check the binary stays within its regression ceiling + run: | + size=$(stat -c%s dist/devcloud) + limit=$((45 * 1024 * 1024)) + echo "dist/devcloud: $((size / 1024 / 1024)) MiB (ceiling $((limit / 1024 / 1024)) MiB)" + if [ "$size" -gt "$limit" ]; then + echo "::error::dist/devcloud exceeds the 45 MiB ceiling docs/coverage.md names, against a published 36.8 MiB. Raising the ceiling is a decision; first find what grew." + exit 1 + fi + # internal/generated is committed but derived. The Go tests check the fidelity # manifest's shape — floors, registered services, the CRUD registry — none of # which notice an operation a provider gained and the manifest never did. diff --git a/README.md b/README.md index 39ea0692..c3f2f850 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ DevCloud is an **on-ramp to the cloud**, not a replacement for it. The goal is t ## Features -- **431 AWS services registered, 426 serving at least one operation** — the remaining 5 are routed and decline with a clean AWS error rather than letting the call bill a real account. See [coverage.md](docs/coverage.md) for what the numbers do and do not promise. +- **431 AWS services registered, 426 serving at least one operation** — every service AWS publishes is routed, so no SDK call escapes to a billable account; the remaining 5 decline with a clean AWS error. Depth is a separate, smaller promise — see [coverage.md](docs/coverage.md) for both targets. - **boto3-compatible** — a 1,530-test suite runs in CI (`make test-compat`) across every registered service. Unsupported operations return a clean AWS error, never a false success. - **Cross-service integration** — CloudFormation provisioning, DynamoDB Streams → Lambda, EventBridge targets, S3 → Lambda - **Smithy-driven codegen** — Go types, routers and error catalogues generated from AWS models, with a weekly sync workflow that keeps them current diff --git a/changes/unreleased/Added-20260913-170500.yaml b/changes/unreleased/Added-20260913-170500.yaml new file mode 100644 index 00000000..eac474fa --- /dev/null +++ b/changes/unreleased/Added-20260913-170500.yaml @@ -0,0 +1,5 @@ +kind: Added +body: 'Three published figures are now gated against the binary that were not: the + routing and depth targets on the coverage page, the size of the shipped binary + (45 MiB ceiling, checked in CI), and the cost of bringing all 431 providers up' +Issue: "163" diff --git a/changes/unreleased/Documentation-20260913-170000.yaml b/changes/unreleased/Documentation-20260913-170000.yaml new file mode 100644 index 00000000..e2672e36 --- /dev/null +++ b/changes/unreleased/Documentation-20260913-170000.yaml @@ -0,0 +1,6 @@ +kind: Documentation +body: 'The coverage page now states its two targets separately — routing, which is + 431 of 431 and is a leak-zero safety property, and depth, which stays at the 205 + services the 2026-09-05 demand study settled — so a service count can no longer + be read as a promise of fidelity' +Issue: "163" diff --git a/cmd/devcloud/budget_test.go b/cmd/devcloud/budget_test.go new file mode 100644 index 00000000..2a8588cd --- /dev/null +++ b/cmd/devcloud/budget_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/skyoo2003/devcloud/internal/plugin" +) + +// fleetBudget is an order-of-magnitude ceiling, deliberately far above the +// figure docs/coverage.md publishes. The published number is a measurement on a +// quiet machine; this runs on a shared CI runner, where a 2x reading is +// indistinguishable from a noisy neighbour. A budget tight enough to catch a 2x +// regression would therefore fail for reasons that have nothing to do with +// DevCloud, so this one catches the regression that is real — a provider that +// starts doing per-call work at startup, which shows up as 10x, not 2x. +// +// Measured at 135-155 ms locally for all 431 services. Injecting 8 ms of work +// per service takes it to 4.0 s. +const fleetBudget = 2 * time.Second + +// TestRegisteredFleetComesUpWithinItsBudget gates the startup half of the +// runtime cost docs/coverage.md publishes. +// +// It brings every registered service up the way main.go does — factory, then +// Init against a per-service data directory — because that is where the cost +// is. Registration itself is a map insert and does not get slower. What gets +// slower is Init: s3 alone does an os.MkdirAll and opens a SQLite database +// there, and 431 of those is how a 42 ms startup becomes a second. +// +// An earlier version of this test timed Construct instead, which only calls the +// factory. Every factory in the tree is a struct literal, so it measured 84 µs +// against a 150 ms budget and would have passed unchanged while startup +// regressed arbitrarily — verified by injecting 8 ms per service, which that +// version did not notice and this one fails on. +// +// The wall-clock figures in docs/coverage.md stay a measurement, re-taken per +// release. This is the gate that notices between measurements. +func TestRegisteredFleetComesUpWithinItsBudget(t *testing.T) { + ids := plugin.DefaultRegistry.RegisteredServices() + if len(ids) == 0 { + t.Fatal("no services are registered; imports.go is not linking the service packages") + } + + // A fresh registry rather than DefaultRegistry: Init records the instance as + // active, and leaving 431 initialized services behind would leak into every + // other test in this package. + fresh := plugin.NewRegistry() + for _, id := range ids { + fresh.Register(id, func() plugin.ServicePlugin { + p, ok := plugin.DefaultRegistry.Construct(id) + if !ok { + t.Errorf("%s is registered but its factory would not construct", id) + return nil + } + return p + }) + } + t.Cleanup(func() { _ = fresh.ShutdownAll(context.Background()) }) + + root := t.TempDir() + + start := time.Now() + for _, id := range ids { + if _, err := fresh.Init(id, plugin.PluginConfig{DataDir: filepath.Join(root, id)}); err != nil { + // Not a timing failure but a real one: main.go treats a failed Init + // outside initOrder as a warning, so a service that stops coming up + // would otherwise only be noticed as a fast run. + t.Errorf("%s is registered but failed to initialize: %v", id, err) + } + } + elapsed := time.Since(start) + + t.Logf("brought %d services up in %s", len(ids), elapsed.Round(time.Millisecond)) + if elapsed > fleetBudget { + t.Errorf("bringing %d services up took %s, over the %s budget. Something in a "+ + "provider's Init is doing work it did not do before. Find it rather than "+ + "raising the budget; the startup figure is published in docs/coverage.md.", + len(ids), elapsed.Round(time.Millisecond), fleetBudget) + } +} diff --git a/cmd/devcloud/coverage_test.go b/cmd/devcloud/coverage_test.go index 2d28ae52..795facaa 100644 --- a/cmd/devcloud/coverage_test.go +++ b/cmd/devcloud/coverage_test.go @@ -241,6 +241,72 @@ func TestOtherDocsQuoteTheSameFigure(t *testing.T) { } } +// targetTableRow matches one row of the two-axis table in +// docs/coverage.md#the-target: +// +// | **Routing target** | **431 / 431 — met** | leak-zero; … | +// +// Only the first number in the value cell is captured. "431 / 431 — met" and +// "205 — met" are written for a reader; the gate reads the figure the reader +// sees rather than asking the table to be machine-shaped, which is the same +// trade tierRow makes. +func targetTableRow(t *testing.T, doc, label string) int { + t.Helper() + + pattern := regexp.MustCompile(`(?m)^\|\s*\*?\*?` + regexp.QuoteMeta(label) + + `\*?\*?\s*\|\s*\*?\*?([\d,]+)`) + matches := pattern.FindAllStringSubmatch(doc, -1) + if len(matches) != 1 { + t.Fatalf("docs/coverage.md: found %d target rows for %q, want exactly 1. "+ + "The two-axis table is what this gate reads; if it was restructured, "+ + "move the pattern deliberately rather than letting the target stop "+ + "being checked.", len(matches), label) + } + + n, err := strconv.Atoi(strings.ReplaceAll(matches[0][1], ",", "")) + if err != nil { + t.Fatalf("docs/coverage.md: target row %q has an unreadable number %q", label, matches[0][1]) + } + return n +} + +// TestPublishedTargetTableMatchesTheBinary gates the target itself, which is the +// half of this page nothing read until now. +// +// The summary table at the top has been gated since Milestone 6, and the target +// table under #the-target has not — so the page once reached a state where it +// published 431 registered and, further down, called the target "205 services, +// not 431" and described those 431 as not targeted. Every number there was wrong +// and every gate was green. +// +// Two of the three numbers are derivable from the binary and are checked against +// it. The serving target is a decision, not a measurement — it is read from the +// page and used as the arithmetic the third number must satisfy, so the table +// cannot be internally inconsistent either. +func TestPublishedTargetTableMatchesTheBinary(t *testing.T) { + raw, err := os.ReadFile(coveragePath) + if err != nil { + t.Fatalf("read the published coverage claim: %v", err) + } + doc := string(raw) + + registered := len(plugin.DefaultRegistry.RegisteredServices()) + + if got := targetTableRow(t, doc, "Routing target"); got != registered { + t.Errorf("docs/coverage.md publishes a routing target of %d services, the binary "+ + "registers %d. The routing target is every service AWS publishes, so these "+ + "move together or the leak-zero claim is no longer true.", got, registered) + } + + serving := targetTableRow(t, doc, "Serving target") + outside := targetTableRow(t, doc, "Registered and engine-served, outside the serving target") + if got, want := outside, registered-serving; got != want { + t.Errorf("docs/coverage.md publishes %d services outside the serving target; "+ + "%d registered minus a serving target of %d is %d. One of the three "+ + "numbers moved without the others.", got, registered, serving, want) + } +} + // demandPath is the evidence behind the published target. See docs/demand.md. const demandPath = "../../docs/demand.md" diff --git a/cmd/devcloud/sync_test.go b/cmd/devcloud/sync_test.go index 60929625..c4a6bda0 100644 --- a/cmd/devcloud/sync_test.go +++ b/cmd/devcloud/sync_test.go @@ -188,9 +188,10 @@ func TestSyncPullRequestBodyReportsTheTestResult(t *testing.T) { // TestSyncPullRequestBodySummarisesTheChurn is the guarantee that makes the PR // reviewable rather than merely present. // -// The sync regenerates from all 194 models at once, so its diff is whole-tree -// whether upstream moved one model or ninety — measured on 2026-09-06, a real -// refresh changed 93 models and 134 generated files. "Please review" over that +// The sync regenerates from all 420 models at once, so its diff is whole-tree +// whether upstream moved one model or ninety — measured on 2026-09-06, when 194 +// were vendored, a real refresh changed 93 models and 134 generated files, and +// the vendored set has more than doubled since. "Please review" over that // is not an action anyone performs. scripts/model_churn.py reduces it to the // question a reviewer has, which operations moved, and the body must carry that // answer or the reviewer is back to reading the regeneration. diff --git a/docs/README.md b/docs/README.md index 70a9c48e..472e07c1 100644 --- a/docs/README.md +++ b/docs/README.md @@ -13,7 +13,7 @@ | Page | What it covers | |---|---| -| [Coverage](coverage.md) | 431 registered / 426 serving — what the counts promise, and the target | +| [Coverage](coverage.md) | 431 registered / 426 serving — the routing and depth targets, and what each promises | | [Compatibility Policy](compatibility-policy.md) | What v1.0 guarantees across 1.x, what it does not, and how deprecation works | | [Fidelity Manifest](fidelity-manifest.md) | Per-operation tiers: how much to trust any given call | | [CRUD Engine](crud-engine.md) | How engine-served operations behave, and where they stop | diff --git a/docs/contributing.md b/docs/contributing.md index 628cb3ff..23f0f81f 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -89,11 +89,12 @@ serializer or deserializer. See [architecture.md](architecture.md#code-generatio ### Reviewing the weekly model sync -[`smithy-sync.yml`](../.github/workflows/smithy-sync.yml) refreshes all 194 +[`smithy-sync.yml`](../.github/workflows/smithy-sync.yml) refreshes all 420 vendored models every Monday and opens a pull request. The diff is whole-tree — a -measured refresh moved 93 models and 134 generated files — so do not try to read -it. Read the PR body instead: it is generated by `scripts/model_churn.py` and -lists which services gained or lost operations. +refresh measured at 194 models moved 93 of them and 134 generated files, and the +vendored set has more than doubled since — so do not try to read it. Read the PR +body instead: it is generated by `scripts/model_churn.py` and lists which +services gained or lost operations. Three things to check, in order: diff --git a/docs/coverage.md b/docs/coverage.md index 165674ca..ff39abc1 100644 --- a/docs/coverage.md +++ b/docs/coverage.md @@ -20,10 +20,11 @@ Per operation, from the [fidelity manifest](fidelity-manifest.md): | `unimplemented` | 3,802 | | **total known** | **19,201** | -> **The target is depth for 205 services, not breadth for 431.** All 431 are -> registered — the codegen scaffold made breadth nearly free — but registration is -> not the promise. What the evidence refused was a promise of *depth* across 431, -> and it still refuses it. See [The target](#the-target). +> **Two targets, not one: routing is 431 of 431, depth is 205.** Every service +> AWS publishes is registered, so no call can leave for a billable account — that +> is a safety property and it admits no smaller number. Depth is the separate, +> smaller promise, and the evidence still puts it at 205. See +> [The target](#the-target). Every figure on this page is asserted against the binary by `go test ./cmd/devcloud/`. Editing one here without the code moving fails CI, and @@ -130,7 +131,25 @@ not packaging artefacts. ## The target -**Decided 2026-09-05. The target is 205 services, not 431 — and it is met.** +DevCloud publishes **two** targets. They answer different questions, and reading +one as the other is the mistake this page exists to prevent. + +- **Routing — every service AWS publishes, and it is met.** A registered service + is answered at `localhost:4747`; an unregistered one is not routed, so the SDK + call leaves the machine and bills a real AWS account. That is a safety + property, not a capability claim, and the only number that satisfies it is all + of them. +- **Serving depth — 205 services, decided 2026-09-05, and it is met.** Depth is + what costs, so it follows evidence of demand rather than the shape of AWS's + catalogue. The study below is that evidence, and it is unchanged. + +| Axis | Services | Governed by | +|---|---|---| +| **Routing target** | **431 / 431 — met** | leak-zero; every published model is registered | +| **Serving target** | **205 — met** | the demand study below, sampled 2026-09-05 | +| Registered and engine-served, outside the serving target | 226 | no depth promise — see the [CRUD engine](crud-engine.md) | + +### How the depth target was set The old target was every service AWS publishes. It rested on an assumption nobody had tested: that the services DevCloud does not register are services anyone @@ -138,16 +157,14 @@ wants. Before committing to building ~283 of them, the assumption was tested against three independent projects that each only add a service when someone asks. It did not hold. -**All 431 are now registered, and the decision above still stands.** What it +**Registering all 431 later did not overturn that decision.** What the study refused was the *cost* — hand-building 283 services on the assumption someone wanted them. The codegen scaffold removed that cost: registering the remaining 226 became a flag on `make codegen`, not a programme of work, and the services it reached are served by the generic [CRUD engine](crud-engine.md) at engine -fidelity. So breadth was taken because it turned out to be nearly free, and the -target stayed where the evidence put it, because the target was never a count of -registrations — it is where DevCloud promises to be worth trusting. Read the -table below as two different claims, not one: 431 services answer locally instead -of billing a real account, and 205 are the ones whose depth is a commitment. +fidelity. So routing was taken because it turned out to be nearly free, and the +depth target stayed where the evidence put it, because it was never a count of +registrations — it is where DevCloud promises to be worth trusting. **The rule was fixed before the numbers were seen** — four outcomes written down in advance, including one for "the method itself failed", specifically so the @@ -164,19 +181,14 @@ evidence in [demand.md](demand.md); re-derive with | `M` built by none | 115 | | DevCloud's own service requests, all time | **0** | -The rule kept the 100% target only if ≥60% of `M` had support ≥2, and narrowed to -a demand set if ≥100 did. 57 cleared neither bar, so the pre-registered -consequence applied: **the 100% claim is dropped and the published target becomes -the demand set.** All 57 are registered — 56 serve at least one operation, and -`rds-data` is the exception named above. It is supported by all three projects, -the strongest signal in the set, and still cannot be served generically. Breadth -does not reach every service, and saying so is cheaper than a fabricated success. - -| | Services | -|---|---| -| Registered today | **431** | -| Target: registered + demonstrated demand | **205 — met** | -| Registered, scaffold-served, outside the target | 226 | +The rule kept the 100% depth target only if ≥60% of `M` had support ≥2, and +narrowed to a demand set if ≥100 did. 57 cleared neither bar, so the +pre-registered consequence applied: **the 100% depth claim is dropped and the +published depth target becomes the demand set.** All 57 are registered — 56 serve +at least one operation, and `rds-data` is the exception named above. It is +supported by all three projects, the strongest signal in the set, and still +cannot be served generically. The engine does not reach every service it routes, +and saying so is cheaper than a fabricated success. Four fifths of the AWS surface is surface that three projects with far more history and staffing have collectively declined to build. That is what a long @@ -236,32 +248,59 @@ anywhere. Apple Silicon, `CGO_ENABLED=0`, measured at each step of the roadmap: -| | 105 services | 147 services | 205 services | -|---|---|---|---| -| Binary | 30.8 MiB | 31.3 MiB | **33.1 MiB** | -| Peak RSS (`/usr/bin/time -l`) | 57.3 MiB | 57.7 MiB | **43.7 MiB** | -| Service registration | — | — | **49 ms for all 205** | - -The single-binary, zero-config property holds at the target with room to spare. -Startup does not scale meaningfully with service count: registration is a map -insert per service in `init()`, and the generated type definitions are mostly -dead-code-eliminated by the linker, which is why 58 more services cost 1.8 MiB. +| | 105 services | 147 services | 205 services | 431 services | +|---|---|---|---|---| +| Binary | 30.8 MiB | 31.3 MiB | 33.1 MiB | **36.8 MiB** | +| Peak RSS (`/usr/bin/time -l`) | 57.3 MiB | 57.7 MiB | 43.7 MiB | **51.2 MiB** | +| Service startup | — | — | 49 ms for all 205 | **42 ms for all 431** | + +The single-binary, zero-config property holds at 431 with room to spare. Startup +does not scale meaningfully with service count: registration is a map insert per +service in `init()`, and the generated type definitions are mostly +dead-code-eliminated by the linker, which is why 58 more services cost 1.8 MiB — +and why 226 more, nearly quadrupling the fleet, cost 3.7 MiB rather than the +7 MiB a linear reading of that figure predicts. + +Two ceilings on the startup reading: + +1. **It is timed from the first `service initialized` line to `DevCloud ready`**, + so it covers bringing every service up, not the `init()` registration alone. + That is the number an operator waits for. +2. **A first run costs about three times as much** — 118 to 148 ms across four + measurements, against 42 ms once the data directories exist, because each of + the 431 services creates its own on the way up. The cold path is the one to + watch, and it is the one the gate below reproduces. + +Neither figure above is asserted at its measured value — a wall-clock reading +taken on a shared CI runner would fail for reasons that have nothing to do with +DevCloud. What is asserted is an order of magnitude. +`cmd/devcloud/budget_test.go` brings every registered service up the way +`main.go` does — factory, then `Init` against a per-service data directory — and +fails past 2 s, roughly fourteen times the 141 ms that same path measures +locally. The gap is the room a shared runner needs; what survives it is the +regression that matters, a provider that starts doing per-call work at startup. +That shows up as 10x, not 2x. + +The binary is gated the same way: CI fails the build past **45 MiB**, against the +36.8 MiB above. Both budgets are loose on purpose and both are decisions — if one +starts failing, find what grew before raising it. The measured numbers stay +measurements, re-taken per release. The RSS readings were taken on different days and are not a controlled -comparison. Read them as "memory is not the constraint at 205" rather than as a -saving — what they agree on is the shape: memory is dominated by the runtime and -the store, not by how many services are registered. +comparison. Read them as "memory is not the constraint" rather than as a saving — +what they agree on is the shape: memory is dominated by the runtime and the +store, not by how many services are registered. ## Keeping up with upstream -The 205 services are vendored from 194 Smithy models, and AWS keeps changing +The 431 services are vendored from 420 Smithy models, and AWS keeps changing them. A [weekly workflow](../.github/workflows/smithy-sync.yml) refreshes all of them and opens a pull request. What that review costs was measured once, on **2026-09-06**: | Reading | Value | |---|---| -| Vendored models refreshed | 194 | +| Vendored models refreshed | 194 (of 420 vendored today) | | Models that changed | 93 | | Of those, models that added or removed an operation | 32 | | Of those, models that changed only documentation | 0 | @@ -274,6 +313,13 @@ changed were all vendored 141 days earlier; the other 101 were vendored the day before, and not one of them changed. So the reading is an accumulated backlog, and the weekly rate is still unknown. +**And it was taken at 194 models, not 420.** The vendored set has since more than +doubled, so the diff a reviewer faces is larger than anything measured here by +roughly the same factor, and the operation counts in the table predate the 6,763 +operations the long tail brought with it. What does not change is the shape of +the review: it is driven by which operations moved, not by how many files did. +Reducing that cost rather than restating it is tracked separately. + What the sample does settle is the *shape* of the work. None of the 93 was documentation-only, so no sync can be waved through on the assumption that AWS only reworded things. Thirty-two services gained operations — `ec2` alone gained diff --git a/docs/demand.md b/docs/demand.md index ee43f43b..e8e139d4 100644 --- a/docs/demand.md +++ b/docs/demand.md @@ -48,7 +48,10 @@ at any point would have stopped on the best surface available. `workspaces-web`; `TestDemandSetIsRegistered` fails if any of them stops being. 56 serve at least one operation, and the one that does not (`rds-data`) is named with its reason in [coverage.md](coverage.md). Everything below them — support 1 -and support 0 — is the 226 services that remain explicitly not targeted. +and support 0 — is the 226 services that carry no depth commitment. They are +registered and engine-served, so a call to one is answered locally rather than +billed; what this study declined to promise is that it is answered *faithfully*. +See [the two axes](coverage.md#the-target). | Service | moto | LocalStack | terraform-provider-aws | Support | |---|---|---|---|---| diff --git a/docs/testing/phase-4-review-followup.tdd.md b/docs/testing/phase-4-review-followup.tdd.md new file mode 100644 index 00000000..e3f10334 --- /dev/null +++ b/docs/testing/phase-4-review-followup.tdd.md @@ -0,0 +1,158 @@ +# TDD evidence — Phase 4 code review follow-up + +Date: 2026-09-13 +Branch: `feat/phase-4-two-axis-docs-and-gates` + +## Source plan + +No `*.plan.md`. The work items are the findings from the local code review of the +uncommitted Phase 4 changes (the two-axis coverage docs, the target-table gate, +the binary-size CI check, and the startup budget test). + +One finding raised in that review — that the runtime table's delimiter row had +fallen out of sync with its header — **was wrong**. The delimiter row has five +cells and matches. It is recorded here so the correction is not lost: + +``` +$ awk 'NR==251||NR==252 {n=gsub(/\|/,"|"); print NR": "n-1" cells: "$0}' docs/coverage.md +251: 5 cells: | | 105 services | 147 services | 205 services | 431 services | +252: 5 cells: |---|---|---|---|---| +``` + +## User journeys + +1. As a maintainer, I want the published startup figure to be gated by something + that would actually fail if startup regressed, so a green CI is evidence and + not decoration. +2. As a maintainer hitting the binary-size failure in CI, I want the error to + name a number I can find in the docs, so the message is actionable. +3. As a reader of `docs/coverage.md`, I want the text describing what is gated to + describe what is in fact gated. + +## Task report + +### Finding (HIGH) — the startup gate measured the wrong phase + +`cmd/devcloud/budget_test.go` timed `Registry.Construct`, which only calls the +factory. Every factory in the tree is a struct literal — `internal/services/s3/provider.go:1415` +is `return &S3Provider{}` — while the real startup cost is in `Init`, where +`S3Provider.Init` does an `os.MkdirAll` and opens a SQLite database +(`internal/services/s3/provider.go:48`). The gate measured 84 µs against a 150 ms +budget: roughly 1,800x slack over work that could not regress. + +The test now brings every service up the way `main.go:86` does — factory, then +`Init` against a per-service `DataDir` under `t.TempDir()` — into a fresh +registry, so `DefaultRegistry.active` is not polluted for the rest of the +package. A `t.Cleanup` runs `ShutdownAll` to close the SQLite handles. + +**RED** was produced by injecting a regression of the exact shape the gate claims +to catch: `time.Sleep(8 * time.Millisecond)` inside `Registry.Init` +(`internal/plugin/registry.go:47`), simulating every provider gaining per-call +work at startup. Both the old and the new gate were run against it. + +``` +$ go test ./cmd/devcloud/ -run 'TestOldGateConstructOnly|TestRegisteredFleetComesUpWithinItsBudget' -v -count=1 +=== RUN TestOldGateConstructOnly + budget_old_test.go:30: OLD GATE: constructed 431 services in 79µs +--- PASS: TestOldGateConstructOnly (0.00s) <-- the blind gate, unmoved +=== RUN TestRegisteredFleetComesUpWithinItsBudget + budget_test.go:47: initialized 431 services (0 declined) in 4.018s + budget_test.go:52: bringing 431 services up took 4.018s, over the 2s budget +--- FAIL: TestRegisteredFleetComesUpWithinItsBudget (4.07s) +``` + +The injection was then reverted (`git diff --quiet internal/plugin/registry.go` +reports clean) and the temporary `budget_old_test.go` deleted. + +**GREEN**: + +``` +$ go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget -v -count=1 + budget_test.go:78: brought 431 services up in 141ms +--- PASS: TestRegisteredFleetComesUpWithinItsBudget (0.17s) +``` + +Baseline across three runs before the budget was chosen: 135 ms, 155 ms, 142 ms, +with 0 of 431 services declining to initialize. The 141 ms path reproduces the +cold-path figure `docs/coverage.md` publishes (118–148 ms) rather than +approximating it. + +**Why the budget is 2 s and not 150 ms.** The published number is a measurement +on a quiet machine; the gate runs on a shared CI runner where a 2x reading is +indistinguishable from a noisy neighbour. A budget tight enough to catch 2x would +fail for reasons unrelated to DevCloud — the objection `docs/coverage.md` itself +raises. 2 s is ~14x the local reading and catches the regression that is real: a +provider doing per-call work at startup, which the injection above shows arrives +as 10x, not 2x. The looseness is stated in the test comment so it is not mistaken +for a tight gate. + +### Finding (MEDIUM) — the CI size error named a figure the docs did not carry + +`45 MiB` appeared nowhere under `docs/`, yet the failure text read "exceeds the +size budget docs/coverage.md publishes". `coverage.md` publishes 36.8 MiB on +Apple Silicon while CI builds for linux, so they are not the same number and +never will be. + +Fixed on both sides rather than either: `docs/coverage.md` now names the 45 MiB +ceiling next to the 36.8 MiB measurement, and the CI step is renamed to +"regression ceiling" with an error that quotes both numbers. No test — the check +is a shell comparison in `.github/workflows/ci.yml` and its own failure is the +evidence. + +### Finding (MEDIUM/LOW) — documentation follow-through + +- `docs/coverage.md` runtime section rewritten: it described gating "constructing + all 431 providers", which is what the test no longer does. It now states what + is asserted (an order of magnitude) and why that differs from what is measured. +- The "published ceiling is 150 ms" sentence was removed; that ceiling no longer + exists. +- Duplicate row in the upstream-sync table collapsed — "Models vendored when + sampled | 194 (of 420 today)" and "Vendored models refreshed | 194" said the + same thing twice. +- `docs/contributing.md:95` rewrapped from 144 characters to the ~80 used + throughout. No words changed. + +## Test specification + +| # | What is guaranteed | Test file or command | Test type | Result | Evidence | +|---|---|---|---|---|---| +| 1 | Every registered service initializes; none is registered-but-broken | `cmd/devcloud/budget_test.go:TestRegisteredFleetComesUpWithinItsBudget` | integration | PASS | `go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget` — 431 up, 0 declined | +| 2 | Bringing the whole fleet up stays within an order of magnitude of the published figure | same | integration | PASS | 141 ms against a 2 s ceiling; fails at 4.0 s under an 8 ms/service injection | +| 3 | The routing target on the coverage page equals the count the binary registers | `cmd/devcloud/coverage_test.go:TestPublishedTargetTableMatchesTheBinary` | unit | PASS | `go test ./cmd/devcloud/` | +| 4 | The three numbers in the two-axis table are arithmetically consistent | same | unit | PASS | registered − serving target = outside-target row | +| 5 | The shipped linux binary stays under 45 MiB | `.github/workflows/ci.yml` "Check the binary stays within its regression ceiling" | CI check | not run locally | darwin host cannot produce the linux figure; runs on both ubuntu matrix arches | + +## Coverage and known gaps + +``` +$ go vet ./... # clean +$ gofmt -l ./cmd ./internal # clean +$ go test ./... -count=1 # all packages ok +$ go test ./cmd/devcloud/ -cover # coverage: 0.0% of statements +``` + +The 0.0% figure on `cmd/devcloud` is expected and is not a gap this cycle +introduced. That package's tests assert the binary's *published claims* — the +coverage page, the fidelity manifest, the demand set, the startup budget — +against the registry the binary ships. They exercise `internal/...` through the +registry, not `main.go`'s own statements, so a statement-coverage reading of the +package is not a meaningful number. The 80% target applies to the service and +codegen packages, which `go test ./...` covers. + +Known gaps, deliberate: + +- **The 45 MiB check is unverified locally.** It needs a linux build; the host is + darwin. It will first run on this branch's CI. +- **The gate cannot catch a 2x startup regression**, only ~10x. See the reasoning + above; a tighter budget would be flaky rather than strict. +- **`Init` is exercised with default options** (no `db_path`, no `server_port`), + which is the first-run configuration. A service whose cost only appears under a + non-default option is not covered. + +## Merge evidence + +If these commits are squashed, the RED/GREEN summary is: the startup gate timed +factory calls (84 µs / 150 ms budget) and was proven blind by an 8 ms-per-service +injection it did not notice; it now times `Init` the way `main.go` does (141 ms / +2 s ceiling) and fails at 4.0 s under the same injection. The reasoning is also +carried in the doc comment on `TestRegisteredFleetComesUpWithinItsBudget`. From 17d24156391ca7ddb95be69790e79a1b8b504369 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 13 Sep 2026 09:06:12 +0900 Subject: [PATCH 2/4] fix: stop gating startup on wall-clock, which CI disproved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2 s ceiling this branch added was chosen as ~14x the 141 ms the fleet takes to come up locally, on the reasoning that 14x was room enough for a shared runner. CI measured the same path at 2.048 s on arm64. The runner is itself 14x slower, so the whole margin was the machine and none of it was headroom. Raising the number does not rescue the design. What is worth catching is a provider that starts opening a file or a database per service at startup, and that costs 1.2x to 2x, because that is what 431 extra file opens are worth. A ceiling loose enough to clear a 14x difference between machines plus run-to-run variance cannot also fail at 2x. Even the 8 ms-per-service injection used to prove the gate works — far larger than a realistic regression — only reaches 5.5 s against a 2.05 s baseline, so it would pass under any ceiling that does not flake. docs/coverage.md raised this objection before the gate was written; the gate overrode it and CI settled it. So the ceiling is gone and the test keeps the half that holds. Every one of the 431 registered services must initialize, which is new and was never covered: main.go brings the long tail up with fatal=false, so a service that is registered but can no longer initialize degrades to a warning in a log nobody reads. The elapsed time is logged beside it and becomes an assertion only when DEVCLOUD_STARTUP_BUDGET names one, which is how the published figure is re-taken on a machine whose speed is known. The binary size gate is untouched. Size does not vary with how busy a runner is. --- changes/unreleased/Added-20260913-170500.yaml | 10 ++- cmd/devcloud/budget_test.go | 73 +++++++++-------- docs/coverage.md | 43 ++++++---- docs/testing/phase-4-review-followup.tdd.md | 80 +++++++++++++++---- 4 files changed, 138 insertions(+), 68 deletions(-) diff --git a/changes/unreleased/Added-20260913-170500.yaml b/changes/unreleased/Added-20260913-170500.yaml index eac474fa..beac1941 100644 --- a/changes/unreleased/Added-20260913-170500.yaml +++ b/changes/unreleased/Added-20260913-170500.yaml @@ -1,5 +1,9 @@ kind: Added -body: 'Three published figures are now gated against the binary that were not: the - routing and depth targets on the coverage page, the size of the shipped binary - (45 MiB ceiling, checked in CI), and the cost of bringing all 431 providers up' +body: 'Two published figures are now gated against the binary that were not: the + routing and depth targets on the coverage page, and the size of the shipped + binary (45 MiB ceiling, checked in CI). A third gate is new but narrower than + intended — every one of the 431 registered services is now asserted to + initialize, which main.go only warned about, while its startup time is logged + rather than gated because a shared CI runner is 14x slower than the machine the + published figure comes from' Issue: "163" diff --git a/cmd/devcloud/budget_test.go b/cmd/devcloud/budget_test.go index 2a8588cd..f868e810 100644 --- a/cmd/devcloud/budget_test.go +++ b/cmd/devcloud/budget_test.go @@ -4,6 +4,7 @@ package main import ( "context" + "os" "path/filepath" "testing" "time" @@ -11,35 +12,37 @@ import ( "github.com/skyoo2003/devcloud/internal/plugin" ) -// fleetBudget is an order-of-magnitude ceiling, deliberately far above the -// figure docs/coverage.md publishes. The published number is a measurement on a -// quiet machine; this runs on a shared CI runner, where a 2x reading is -// indistinguishable from a noisy neighbour. A budget tight enough to catch a 2x -// regression would therefore fail for reasons that have nothing to do with -// DevCloud, so this one catches the regression that is real — a provider that -// starts doing per-call work at startup, which shows up as 10x, not 2x. +// startupBudgetEnv opts the timing half of TestRegisteredFleetComesUpWithinItsBudget +// into being an assertion. Unset — which is every CI run — the elapsed time is +// logged and nothing is asserted about it. See the test's comment for why. // -// Measured at 135-155 ms locally for all 431 services. Injecting 8 ms of work -// per service takes it to 4.0 s. -const fleetBudget = 2 * time.Second +// DEVCLOUD_STARTUP_BUDGET=300ms go test ./cmd/devcloud/ +const startupBudgetEnv = "DEVCLOUD_STARTUP_BUDGET" -// TestRegisteredFleetComesUpWithinItsBudget gates the startup half of the -// runtime cost docs/coverage.md publishes. +// TestRegisteredFleetComesUpWithinItsBudget asserts that every registered +// service actually comes up, and measures what that costs. // -// It brings every registered service up the way main.go does — factory, then -// Init against a per-service data directory — because that is where the cost -// is. Registration itself is a map insert and does not get slower. What gets -// slower is Init: s3 alone does an os.MkdirAll and opens a SQLite database -// there, and 431 of those is how a 42 ms startup becomes a second. +// The correctness half is the part that holds unconditionally. main.go brings +// the long tail up with fatal=false, so a service that is registered but can no +// longer initialize degrades to a warning in a log nobody reads. Here it fails. // -// An earlier version of this test timed Construct instead, which only calls the -// factory. Every factory in the tree is a struct literal, so it measured 84 µs -// against a 150 ms budget and would have passed unchanged while startup -// regressed arbitrarily — verified by injecting 8 ms per service, which that -// version did not notice and this one fails on. +// The timing half is a measurement, not a gate, and the reason is a number. The +// fleet comes up in ~141 ms locally and took 2.048 s on a GitHub arm64 runner: +// the shared runner is 14x slower, and that is before accounting for variance +// between runs on it. The regression actually worth catching — a provider that +// starts opening a file or a database per service at startup — is 1.2x to 2x, +// because that is what 431 extra file opens cost. There is no absolute ceiling +// that clears a 14x machine difference and still fails on a 2x regression, so an +// absolute ceiling in CI would only ever have been decoration that flakes. It is +// better to say startup is measured than to claim a gate that cannot fire. // -// The wall-clock figures in docs/coverage.md stay a measurement, re-taken per -// release. This is the gate that notices between measurements. +// Set DEVCLOUD_STARTUP_BUDGET to assert on a machine whose speed you know — +// that is how the published figure in docs/coverage.md is re-taken per release. +// +// An earlier version of this test timed Construct instead of Init, which only +// calls the factory. Every factory in the tree is a struct literal, so it +// measured 84 µs and could not have failed for any reason. What costs is Init: +// S3Provider.Init does an os.MkdirAll and opens a SQLite database. func TestRegisteredFleetComesUpWithinItsBudget(t *testing.T) { ids := plugin.DefaultRegistry.RegisteredServices() if len(ids) == 0 { @@ -67,19 +70,25 @@ func TestRegisteredFleetComesUpWithinItsBudget(t *testing.T) { start := time.Now() for _, id := range ids { if _, err := fresh.Init(id, plugin.PluginConfig{DataDir: filepath.Join(root, id)}); err != nil { - // Not a timing failure but a real one: main.go treats a failed Init - // outside initOrder as a warning, so a service that stops coming up - // would otherwise only be noticed as a fast run. t.Errorf("%s is registered but failed to initialize: %v", id, err) } } elapsed := time.Since(start) t.Logf("brought %d services up in %s", len(ids), elapsed.Round(time.Millisecond)) - if elapsed > fleetBudget { - t.Errorf("bringing %d services up took %s, over the %s budget. Something in a "+ - "provider's Init is doing work it did not do before. Find it rather than "+ - "raising the budget; the startup figure is published in docs/coverage.md.", - len(ids), elapsed.Round(time.Millisecond), fleetBudget) + + raw, ok := os.LookupEnv(startupBudgetEnv) + if !ok { + return + } + budget, err := time.ParseDuration(raw) + if err != nil { + t.Fatalf("%s=%q is not a duration: %v", startupBudgetEnv, raw, err) + } + if elapsed > budget { + t.Errorf("bringing %d services up took %s, over the %s asked for. Something in a "+ + "provider's Init is doing work it did not do before — that is what scales with "+ + "service count. If this is a slower machine than the one the budget was set on, "+ + "it is the budget that is wrong.", len(ids), elapsed.Round(time.Millisecond), budget) } } diff --git a/docs/coverage.md b/docs/coverage.md index ff39abc1..2dd254c5 100644 --- a/docs/coverage.md +++ b/docs/coverage.md @@ -269,22 +269,33 @@ Two ceilings on the startup reading: 2. **A first run costs about three times as much** — 118 to 148 ms across four measurements, against 42 ms once the data directories exist, because each of the 431 services creates its own on the way up. The cold path is the one to - watch, and it is the one the gate below reproduces. - -Neither figure above is asserted at its measured value — a wall-clock reading -taken on a shared CI runner would fail for reasons that have nothing to do with -DevCloud. What is asserted is an order of magnitude. -`cmd/devcloud/budget_test.go` brings every registered service up the way -`main.go` does — factory, then `Init` against a per-service data directory — and -fails past 2 s, roughly fourteen times the 141 ms that same path measures -locally. The gap is the room a shared runner needs; what survives it is the -regression that matters, a provider that starts doing per-call work at startup. -That shows up as 10x, not 2x. - -The binary is gated the same way: CI fails the build past **45 MiB**, against the -36.8 MiB above. Both budgets are loose on purpose and both are decisions — if one -starts failing, find what grew before raising it. The measured numbers stay -measurements, re-taken per release. + watch, and it is the one `cmd/devcloud/budget_test.go` reproduces. + +**Startup is measured, not gated, and that is a deliberate retreat.** The gate +was written as a wall-clock ceiling and CI disproved it: the same path that takes +141 ms locally took 2.048 s on a GitHub arm64 runner. The regression worth +catching — a provider that starts opening a file or a database per service at +startup — costs 1.2x to 2x, because that is what 431 extra file opens are worth. +No absolute ceiling clears a 14x difference between machines and still fails on a +2x regression, so a ceiling in CI would have been decoration that flakes. Saying +the figure is measured is cheaper than claiming a gate that cannot fire. + +What `budget_test.go` *does* assert on every run is that all 431 services come +up at all. `main.go` initializes the long tail non-fatally, so a service that is +registered but can no longer initialize would otherwise degrade to a warning in a +log nobody reads. The timing is logged beside it, and becomes an assertion when +you name a budget on a machine whose speed you know: + +``` +DEVCLOUD_STARTUP_BUDGET=300ms go test ./cmd/devcloud/ +``` + +That is how the figures above are re-taken per release. + +The binary size *is* gated, because size does not vary with how busy a runner is: +CI fails the build past **45 MiB**, against the 36.8 MiB above. The gap is the +platform difference — that figure is Apple Silicon, CI builds for linux — not +slack. It is a decision; if it starts failing, find what grew before raising it. The RSS readings were taken on different days and are not a controlled comparison. Read them as "memory is not the constraint" rather than as a saving — diff --git a/docs/testing/phase-4-review-followup.tdd.md b/docs/testing/phase-4-review-followup.tdd.md index e3f10334..da227eff 100644 --- a/docs/testing/phase-4-review-followup.tdd.md +++ b/docs/testing/phase-4-review-followup.tdd.md @@ -77,14 +77,46 @@ with 0 of 431 services declining to initialize. The 141 ms path reproduces the cold-path figure `docs/coverage.md` publishes (118–148 ms) rather than approximating it. -**Why the budget is 2 s and not 150 ms.** The published number is a measurement -on a quiet machine; the gate runs on a shared CI runner where a 2x reading is -indistinguishable from a noisy neighbour. A budget tight enough to catch 2x would -fail for reasons unrelated to DevCloud — the objection `docs/coverage.md` itself -raises. 2 s is ~14x the local reading and catches the regression that is real: a -provider doing per-call work at startup, which the injection above shows arrives -as 10x, not 2x. The looseness is stated in the test comment so it is not mistaken -for a tight gate. +**The 2 s budget was then disproved by CI, and removed.** It was chosen as ~14x +the 141 ms local reading, on the reasoning that 14x was room enough for a shared +runner. CI on PR #163 measured the same path at **2.048 s on arm64** — the runner +is itself 14x slower, so the entire margin was the machine and none of it was +headroom: + +``` +budget_test.go:78: brought 431 services up in 2.048s +budget_test.go:80: bringing 431 services up took 2.048s, over the 2s budget +--- FAIL: TestRegisteredFleetComesUpWithinItsBudget (2.07s) +``` + +Raising the number does not rescue the design. The regression worth catching is a +provider that starts opening a file or a database per service, which costs 1.2x +to 2x — 431 extra file opens. A ceiling loose enough to clear a 14x machine +difference plus run-to-run variance cannot also fail at 2x. Even the 8 ms +injection above, which is far larger than a realistic regression, only reaches +5.5 s on a runner whose baseline is 2.05 s, so it would pass under any ceiling +that does not flake. + +`docs/coverage.md` made this objection before the gate was written. The gate +overrode it and CI settled the question. + +**Resolution.** The correctness half is asserted on every run: all 431 services +must initialize, which `main.go` only warns about. The timing half is logged, and +becomes an assertion only when `DEVCLOUD_STARTUP_BUDGET` names a budget — used on +a machine whose speed is known, which is how the published figure is re-taken. + +``` +$ go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget -v + budget_test.go:78: brought 431 services up in 168ms +--- PASS # timing logged, not asserted + +$ DEVCLOUD_STARTUP_BUDGET=50ms go test ./cmd/devcloud/ -run ... + budget_test.go:89: bringing 431 services up took 168ms, over the 50ms asked for +--- FAIL # the assertion still works + +$ DEVCLOUD_STARTUP_BUDGET=300ms go test ./cmd/devcloud/ -run ... +ok # and passes when it should +``` ### Finding (MEDIUM) — the CI size error named a figure the docs did not carry @@ -117,7 +149,7 @@ evidence. | # | What is guaranteed | Test file or command | Test type | Result | Evidence | |---|---|---|---|---|---| | 1 | Every registered service initializes; none is registered-but-broken | `cmd/devcloud/budget_test.go:TestRegisteredFleetComesUpWithinItsBudget` | integration | PASS | `go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget` — 431 up, 0 declined | -| 2 | Bringing the whole fleet up stays within an order of magnitude of the published figure | same | integration | PASS | 141 ms against a 2 s ceiling; fails at 4.0 s under an 8 ms/service injection | +| 2 | Startup time is reported on every run, and assertable on a known machine | same | measurement | PASS | 168 ms logged; `DEVCLOUD_STARTUP_BUDGET=50ms` fails, `=300ms` passes | | 3 | The routing target on the coverage page equals the count the binary registers | `cmd/devcloud/coverage_test.go:TestPublishedTargetTableMatchesTheBinary` | unit | PASS | `go test ./cmd/devcloud/` | | 4 | The three numbers in the two-axis table are arithmetically consistent | same | unit | PASS | registered − serving target = outside-target row | | 5 | The shipped linux binary stays under 45 MiB | `.github/workflows/ci.yml` "Check the binary stays within its regression ceiling" | CI check | not run locally | darwin host cannot produce the linux figure; runs on both ubuntu matrix arches | @@ -142,17 +174,31 @@ codegen packages, which `go test ./...` covers. Known gaps, deliberate: - **The 45 MiB check is unverified locally.** It needs a linux build; the host is - darwin. It will first run on this branch's CI. -- **The gate cannot catch a 2x startup regression**, only ~10x. See the reasoning - above; a tighter budget would be flaky rather than strict. + darwin. It runs first on this branch's CI. +- **Startup is not gated in CI at all**, by decision rather than oversight. See + the CI disproof above. A regression that slows startup 2x will not be caught by + anything automated; it will be caught when the published figure is re-taken. - **`Init` is exercised with default options** (no `db_path`, no `server_port`), which is the first-run configuration. A service whose cost only appears under a non-default option is not covered. ## Merge evidence -If these commits are squashed, the RED/GREEN summary is: the startup gate timed -factory calls (84 µs / 150 ms budget) and was proven blind by an 8 ms-per-service -injection it did not notice; it now times `Init` the way `main.go` does (141 ms / -2 s ceiling) and fails at 4.0 s under the same injection. The reasoning is also -carried in the doc comment on `TestRegisteredFleetComesUpWithinItsBudget`. +If these commits are squashed, the summary is: + +1. The startup gate timed `Construct`, which only calls the factory — 84 µs + against a 150 ms budget. Proven blind by an 8 ms-per-service injection into + `Registry.Init` that left it at 79 µs and passing. +2. Rewritten to time `Init` the way `main.go` does, with a 2 s ceiling. Locally + 141 ms; the injection failed it at 4.0 s. +3. CI disproved the ceiling: 2.048 s on arm64, because the runner is 14x slower + than the machine the budget came from. No absolute ceiling can clear a 14x + machine difference and still fail on the 1.2–2x regression that is realistic. +4. The ceiling was removed. What is asserted on every run is that all 431 + services initialize — which `main.go` only warns about. The timing is logged, + and assertable via `DEVCLOUD_STARTUP_BUDGET` on a machine whose speed is + known. + +The reasoning is also carried in the doc comment on +`TestRegisteredFleetComesUpWithinItsBudget`, and in `docs/coverage.md`'s runtime +section. From 16c279f2eafa3a477afb90bc8539a68faccfe01f Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 13 Sep 2026 09:11:44 +0900 Subject: [PATCH 3/4] docs: replace the runtime claims CI could now measure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two figures in the runtime section were written before CI had produced the numbers they describe, and one of them was wrong. The binary ceiling was justified as absorbing a platform difference: 45 MiB against a published 36.8 MiB measured on Apple Silicon, with CI building for linux. CI measures 35 MiB on arm64 and 37 MiB on amd64, so the platform accounts for under 2 MiB and the remaining 8 MiB is slack. Saying so is the point of publishing it. The startup retreat was argued from one reading — 2.048 s on an arm64 runner against 141 ms locally. The run after it measured 1.414 s on the same runner type and 1.375 s on amd64, on identical code. A 45% swing between runs is a better argument than a slow machine: the regression worth catching costs 1.2x to 2x, so it is smaller than the noise a ceiling would have to clear. The docs and the TDD evidence now argue from the variance rather than from the one failure. --- docs/coverage.md | 24 ++++++++-------- docs/testing/phase-4-review-followup.tdd.md | 31 ++++++++++++++------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/docs/coverage.md b/docs/coverage.md index 2dd254c5..93d88d56 100644 --- a/docs/coverage.md +++ b/docs/coverage.md @@ -272,13 +272,14 @@ Two ceilings on the startup reading: watch, and it is the one `cmd/devcloud/budget_test.go` reproduces. **Startup is measured, not gated, and that is a deliberate retreat.** The gate -was written as a wall-clock ceiling and CI disproved it: the same path that takes -141 ms locally took 2.048 s on a GitHub arm64 runner. The regression worth -catching — a provider that starts opening a file or a database per service at -startup — costs 1.2x to 2x, because that is what 431 extra file opens are worth. -No absolute ceiling clears a 14x difference between machines and still fails on a -2x regression, so a ceiling in CI would have been decoration that flakes. Saying -the figure is measured is cheaper than claiming a gate that cannot fire. +was written as a wall-clock ceiling and CI disproved it. The same path that takes +141 ms here took 2.048 s on a GitHub arm64 runner, and 1.414 s on the same runner +type one commit later — so the shared runner is roughly ten times slower *and* +swings 45% between runs on identical code. The regression worth catching is +smaller than that swing: a provider that starts opening a file or a database per +service at startup costs 1.2x to 2x, because that is what 431 extra file opens +are worth. A ceiling that survives the variance cannot fail on the regression. +Saying the figure is measured is cheaper than claiming a gate that cannot fire. What `budget_test.go` *does* assert on every run is that all 431 services come up at all. `main.go` initializes the long tail non-fatally, so a service that is @@ -292,10 +293,11 @@ DEVCLOUD_STARTUP_BUDGET=300ms go test ./cmd/devcloud/ That is how the figures above are re-taken per release. -The binary size *is* gated, because size does not vary with how busy a runner is: -CI fails the build past **45 MiB**, against the 36.8 MiB above. The gap is the -platform difference — that figure is Apple Silicon, CI builds for linux — not -slack. It is a decision; if it starts failing, find what grew before raising it. +The binary size *is* gated, because size does not vary with how busy a runner is. +CI fails the build past **45 MiB** and measures 35 MiB on arm64, 37 MiB on amd64 +— close enough to the 36.8 MiB above that the platform barely registers, so the +remaining 8 MiB is genuine slack rather than a correction for anything. It is a +decision; if it starts failing, find what grew before raising it. The RSS readings were taken on different days and are not a controlled comparison. Read them as "memory is not the constraint" rather than as a saving — diff --git a/docs/testing/phase-4-review-followup.tdd.md b/docs/testing/phase-4-review-followup.tdd.md index da227eff..5d028ff4 100644 --- a/docs/testing/phase-4-review-followup.tdd.md +++ b/docs/testing/phase-4-review-followup.tdd.md @@ -89,13 +89,22 @@ budget_test.go:80: bringing 431 services up took 2.048s, over the 2s budget --- FAIL: TestRegisteredFleetComesUpWithinItsBudget (2.07s) ``` -Raising the number does not rescue the design. The regression worth catching is a -provider that starts opening a file or a database per service, which costs 1.2x -to 2x — 431 extra file opens. A ceiling loose enough to clear a 14x machine -difference plus run-to-run variance cannot also fail at 2x. Even the 8 ms -injection above, which is far larger than a realistic regression, only reaches -5.5 s on a runner whose baseline is 2.05 s, so it would pass under any ceiling -that does not flake. +Raising the number does not rescue the design, and the next CI run showed why. +With the ceiling removed, the same runner type measured **1.414 s** on arm64 and +**1.375 s** on amd64 — a 45% swing against the 2.048 s reading on identical code +one commit earlier: + +| Run | arm64 | amd64 | +|---|---|---| +| `8e95b52` | 2.048 s (failed the 2 s ceiling) | canceled by fail-fast | +| `17d2415` | 1.414 s | 1.375 s | +| local | 0.141–0.168 s | — | + +The regression worth catching is a provider that starts opening a file or a +database per service, which costs 1.2x to 2x — 431 extra file opens. That is +smaller than the runner's own variance. A ceiling that survives 45% noise cannot +fail on a 2x regression. Even the 8 ms injection above, far larger than anything +realistic, only reaches 5.5 s against a ~2 s baseline. `docs/coverage.md` made this objection before the gate was written. The gate overrode it and CI settled the question. @@ -152,7 +161,7 @@ evidence. | 2 | Startup time is reported on every run, and assertable on a known machine | same | measurement | PASS | 168 ms logged; `DEVCLOUD_STARTUP_BUDGET=50ms` fails, `=300ms` passes | | 3 | The routing target on the coverage page equals the count the binary registers | `cmd/devcloud/coverage_test.go:TestPublishedTargetTableMatchesTheBinary` | unit | PASS | `go test ./cmd/devcloud/` | | 4 | The three numbers in the two-axis table are arithmetically consistent | same | unit | PASS | registered − serving target = outside-target row | -| 5 | The shipped linux binary stays under 45 MiB | `.github/workflows/ci.yml` "Check the binary stays within its regression ceiling" | CI check | not run locally | darwin host cannot produce the linux figure; runs on both ubuntu matrix arches | +| 5 | The shipped linux binary stays under 45 MiB | `.github/workflows/ci.yml` "Check the binary stays within its regression ceiling" | CI check | PASS | 35 MiB arm64, 37 MiB amd64, against a published 36.8 MiB on Apple Silicon | ## Coverage and known gaps @@ -173,8 +182,10 @@ codegen packages, which `go test ./...` covers. Known gaps, deliberate: -- **The 45 MiB check is unverified locally.** It needs a linux build; the host is - darwin. It runs first on this branch's CI. +- **The 45 MiB ceiling has ~8 MiB of slack.** CI measures 35–37 MiB, so the + ceiling catches a large regression and nothing smaller. It was set before the + linux figures were known; now that they are, the platform difference it was + partly meant to absorb turns out to be under 2 MiB. - **Startup is not gated in CI at all**, by decision rather than oversight. See the CI disproof above. A regression that slows startup 2x will not be caught by anything automated; it will be caught when the published figure is re-taken. From 105be069f4b96c760326df83956d573bb65ee2fe Mon Sep 17 00:00:00 2001 From: Sungkyu Yoo Date: Sun, 13 Sep 2026 11:47:02 +0900 Subject: [PATCH 4/4] =?UTF-8?q?phase-4-review-followup.tdd.md=20=EC=82=AD?= =?UTF-8?q?=EC=A0=9C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/testing/phase-4-review-followup.tdd.md | 215 -------------------- 1 file changed, 215 deletions(-) delete mode 100644 docs/testing/phase-4-review-followup.tdd.md diff --git a/docs/testing/phase-4-review-followup.tdd.md b/docs/testing/phase-4-review-followup.tdd.md deleted file mode 100644 index 5d028ff4..00000000 --- a/docs/testing/phase-4-review-followup.tdd.md +++ /dev/null @@ -1,215 +0,0 @@ -# TDD evidence — Phase 4 code review follow-up - -Date: 2026-09-13 -Branch: `feat/phase-4-two-axis-docs-and-gates` - -## Source plan - -No `*.plan.md`. The work items are the findings from the local code review of the -uncommitted Phase 4 changes (the two-axis coverage docs, the target-table gate, -the binary-size CI check, and the startup budget test). - -One finding raised in that review — that the runtime table's delimiter row had -fallen out of sync with its header — **was wrong**. The delimiter row has five -cells and matches. It is recorded here so the correction is not lost: - -``` -$ awk 'NR==251||NR==252 {n=gsub(/\|/,"|"); print NR": "n-1" cells: "$0}' docs/coverage.md -251: 5 cells: | | 105 services | 147 services | 205 services | 431 services | -252: 5 cells: |---|---|---|---|---| -``` - -## User journeys - -1. As a maintainer, I want the published startup figure to be gated by something - that would actually fail if startup regressed, so a green CI is evidence and - not decoration. -2. As a maintainer hitting the binary-size failure in CI, I want the error to - name a number I can find in the docs, so the message is actionable. -3. As a reader of `docs/coverage.md`, I want the text describing what is gated to - describe what is in fact gated. - -## Task report - -### Finding (HIGH) — the startup gate measured the wrong phase - -`cmd/devcloud/budget_test.go` timed `Registry.Construct`, which only calls the -factory. Every factory in the tree is a struct literal — `internal/services/s3/provider.go:1415` -is `return &S3Provider{}` — while the real startup cost is in `Init`, where -`S3Provider.Init` does an `os.MkdirAll` and opens a SQLite database -(`internal/services/s3/provider.go:48`). The gate measured 84 µs against a 150 ms -budget: roughly 1,800x slack over work that could not regress. - -The test now brings every service up the way `main.go:86` does — factory, then -`Init` against a per-service `DataDir` under `t.TempDir()` — into a fresh -registry, so `DefaultRegistry.active` is not polluted for the rest of the -package. A `t.Cleanup` runs `ShutdownAll` to close the SQLite handles. - -**RED** was produced by injecting a regression of the exact shape the gate claims -to catch: `time.Sleep(8 * time.Millisecond)` inside `Registry.Init` -(`internal/plugin/registry.go:47`), simulating every provider gaining per-call -work at startup. Both the old and the new gate were run against it. - -``` -$ go test ./cmd/devcloud/ -run 'TestOldGateConstructOnly|TestRegisteredFleetComesUpWithinItsBudget' -v -count=1 -=== RUN TestOldGateConstructOnly - budget_old_test.go:30: OLD GATE: constructed 431 services in 79µs ---- PASS: TestOldGateConstructOnly (0.00s) <-- the blind gate, unmoved -=== RUN TestRegisteredFleetComesUpWithinItsBudget - budget_test.go:47: initialized 431 services (0 declined) in 4.018s - budget_test.go:52: bringing 431 services up took 4.018s, over the 2s budget ---- FAIL: TestRegisteredFleetComesUpWithinItsBudget (4.07s) -``` - -The injection was then reverted (`git diff --quiet internal/plugin/registry.go` -reports clean) and the temporary `budget_old_test.go` deleted. - -**GREEN**: - -``` -$ go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget -v -count=1 - budget_test.go:78: brought 431 services up in 141ms ---- PASS: TestRegisteredFleetComesUpWithinItsBudget (0.17s) -``` - -Baseline across three runs before the budget was chosen: 135 ms, 155 ms, 142 ms, -with 0 of 431 services declining to initialize. The 141 ms path reproduces the -cold-path figure `docs/coverage.md` publishes (118–148 ms) rather than -approximating it. - -**The 2 s budget was then disproved by CI, and removed.** It was chosen as ~14x -the 141 ms local reading, on the reasoning that 14x was room enough for a shared -runner. CI on PR #163 measured the same path at **2.048 s on arm64** — the runner -is itself 14x slower, so the entire margin was the machine and none of it was -headroom: - -``` -budget_test.go:78: brought 431 services up in 2.048s -budget_test.go:80: bringing 431 services up took 2.048s, over the 2s budget ---- FAIL: TestRegisteredFleetComesUpWithinItsBudget (2.07s) -``` - -Raising the number does not rescue the design, and the next CI run showed why. -With the ceiling removed, the same runner type measured **1.414 s** on arm64 and -**1.375 s** on amd64 — a 45% swing against the 2.048 s reading on identical code -one commit earlier: - -| Run | arm64 | amd64 | -|---|---|---| -| `8e95b52` | 2.048 s (failed the 2 s ceiling) | canceled by fail-fast | -| `17d2415` | 1.414 s | 1.375 s | -| local | 0.141–0.168 s | — | - -The regression worth catching is a provider that starts opening a file or a -database per service, which costs 1.2x to 2x — 431 extra file opens. That is -smaller than the runner's own variance. A ceiling that survives 45% noise cannot -fail on a 2x regression. Even the 8 ms injection above, far larger than anything -realistic, only reaches 5.5 s against a ~2 s baseline. - -`docs/coverage.md` made this objection before the gate was written. The gate -overrode it and CI settled the question. - -**Resolution.** The correctness half is asserted on every run: all 431 services -must initialize, which `main.go` only warns about. The timing half is logged, and -becomes an assertion only when `DEVCLOUD_STARTUP_BUDGET` names a budget — used on -a machine whose speed is known, which is how the published figure is re-taken. - -``` -$ go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget -v - budget_test.go:78: brought 431 services up in 168ms ---- PASS # timing logged, not asserted - -$ DEVCLOUD_STARTUP_BUDGET=50ms go test ./cmd/devcloud/ -run ... - budget_test.go:89: bringing 431 services up took 168ms, over the 50ms asked for ---- FAIL # the assertion still works - -$ DEVCLOUD_STARTUP_BUDGET=300ms go test ./cmd/devcloud/ -run ... -ok # and passes when it should -``` - -### Finding (MEDIUM) — the CI size error named a figure the docs did not carry - -`45 MiB` appeared nowhere under `docs/`, yet the failure text read "exceeds the -size budget docs/coverage.md publishes". `coverage.md` publishes 36.8 MiB on -Apple Silicon while CI builds for linux, so they are not the same number and -never will be. - -Fixed on both sides rather than either: `docs/coverage.md` now names the 45 MiB -ceiling next to the 36.8 MiB measurement, and the CI step is renamed to -"regression ceiling" with an error that quotes both numbers. No test — the check -is a shell comparison in `.github/workflows/ci.yml` and its own failure is the -evidence. - -### Finding (MEDIUM/LOW) — documentation follow-through - -- `docs/coverage.md` runtime section rewritten: it described gating "constructing - all 431 providers", which is what the test no longer does. It now states what - is asserted (an order of magnitude) and why that differs from what is measured. -- The "published ceiling is 150 ms" sentence was removed; that ceiling no longer - exists. -- Duplicate row in the upstream-sync table collapsed — "Models vendored when - sampled | 194 (of 420 today)" and "Vendored models refreshed | 194" said the - same thing twice. -- `docs/contributing.md:95` rewrapped from 144 characters to the ~80 used - throughout. No words changed. - -## Test specification - -| # | What is guaranteed | Test file or command | Test type | Result | Evidence | -|---|---|---|---|---|---| -| 1 | Every registered service initializes; none is registered-but-broken | `cmd/devcloud/budget_test.go:TestRegisteredFleetComesUpWithinItsBudget` | integration | PASS | `go test ./cmd/devcloud/ -run TestRegisteredFleetComesUpWithinItsBudget` — 431 up, 0 declined | -| 2 | Startup time is reported on every run, and assertable on a known machine | same | measurement | PASS | 168 ms logged; `DEVCLOUD_STARTUP_BUDGET=50ms` fails, `=300ms` passes | -| 3 | The routing target on the coverage page equals the count the binary registers | `cmd/devcloud/coverage_test.go:TestPublishedTargetTableMatchesTheBinary` | unit | PASS | `go test ./cmd/devcloud/` | -| 4 | The three numbers in the two-axis table are arithmetically consistent | same | unit | PASS | registered − serving target = outside-target row | -| 5 | The shipped linux binary stays under 45 MiB | `.github/workflows/ci.yml` "Check the binary stays within its regression ceiling" | CI check | PASS | 35 MiB arm64, 37 MiB amd64, against a published 36.8 MiB on Apple Silicon | - -## Coverage and known gaps - -``` -$ go vet ./... # clean -$ gofmt -l ./cmd ./internal # clean -$ go test ./... -count=1 # all packages ok -$ go test ./cmd/devcloud/ -cover # coverage: 0.0% of statements -``` - -The 0.0% figure on `cmd/devcloud` is expected and is not a gap this cycle -introduced. That package's tests assert the binary's *published claims* — the -coverage page, the fidelity manifest, the demand set, the startup budget — -against the registry the binary ships. They exercise `internal/...` through the -registry, not `main.go`'s own statements, so a statement-coverage reading of the -package is not a meaningful number. The 80% target applies to the service and -codegen packages, which `go test ./...` covers. - -Known gaps, deliberate: - -- **The 45 MiB ceiling has ~8 MiB of slack.** CI measures 35–37 MiB, so the - ceiling catches a large regression and nothing smaller. It was set before the - linux figures were known; now that they are, the platform difference it was - partly meant to absorb turns out to be under 2 MiB. -- **Startup is not gated in CI at all**, by decision rather than oversight. See - the CI disproof above. A regression that slows startup 2x will not be caught by - anything automated; it will be caught when the published figure is re-taken. -- **`Init` is exercised with default options** (no `db_path`, no `server_port`), - which is the first-run configuration. A service whose cost only appears under a - non-default option is not covered. - -## Merge evidence - -If these commits are squashed, the summary is: - -1. The startup gate timed `Construct`, which only calls the factory — 84 µs - against a 150 ms budget. Proven blind by an 8 ms-per-service injection into - `Registry.Init` that left it at 79 µs and passing. -2. Rewritten to time `Init` the way `main.go` does, with a 2 s ceiling. Locally - 141 ms; the injection failed it at 4.0 s. -3. CI disproved the ceiling: 2.048 s on arm64, because the runner is 14x slower - than the machine the budget came from. No absolute ceiling can clear a 14x - machine difference and still fail on the 1.2–2x regression that is realistic. -4. The ceiling was removed. What is asserted on every run is that all 431 - services initialize — which `main.go` only warns about. The timing is logged, - and assertable via `DEVCLOUD_STARTUP_BUDGET` on a machine whose speed is - known. - -The reasoning is also carried in the doc comment on -`TestRegisteredFleetComesUpWithinItsBudget`, and in `docs/coverage.md`'s runtime -section.