From 91e7e5f3214157da7c174e3c9b2b9bbc5e4ec888 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 17:33:30 +0000 Subject: [PATCH] Complete the Stage 22 efficiency audit --- docs/roadmap/audit.md | 209 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 207 insertions(+), 2 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index dc6fc64..5705d99 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -325,7 +325,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 19 | Audit test hermeticity and execution hygiene | Complete | | 20 | Audit test risk coverage and ownership | Complete | | 21 | Audit test durability, duplication, and maintenance cost | Complete | -| 22 | Audit cross-cutting efficiency and complexity | Pending | +| 22 | Audit cross-cutting efficiency and complexity | Complete | | 23 | Audit cross-cutting refactoring and deduplication opportunities | Pending | | 24 | Audit documentation coherence and executable contracts | Pending | | 25 | Run dynamic robustness and final diagnostic validation | Pending | @@ -359,7 +359,8 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Test hermeticity and execution hygiene | All Go tests, fixtures, test assets, and helper mechanisms | 19 | Partly insufficient. The suite is offline, uses controlled clocks/roots/environment for almost every stateful case, has no subprocesses or automatic golden updates, passes race and shuffled-repeat checks, and succeeds with a scrubbed environment and broken external proxies. `AUD-059` records one ambient credential assumption; `AUD-060` records unguarded symlink-capability assumptions. | | Test risk coverage and ownership | Repository-wide suite | 20 | Partly insufficient. Consequential contracts have narrow owners and representative cross-boundary workflows; every identified missing regression is already attached to the production or test-gap finding for the defect it would catch. Coverage diagnostics found no additional unowned critical branch. Accepted omissions are thin entry-point/delegation wrappers, defensive accessors, and dormant helpers already routed under `AUD-001`; broad duplication and durability remain Stage 21. | | Test durability, duplication, and maintenance cost | Repository-wide suite | 21 | Partly insufficient. Most tests assert stable behavior with direct fixtures and appropriately narrow fakes. `AUD-039`, `AUD-061`, and `AUD-063` identify duplicated or implementation-coupled assertions; `AUD-062` identifies avoidable production waits; `AUD-064` identifies incomplete failure cleanup. Each consolidation retains a named behavioral owner. | -| Cross-cutting efficiency and maintainability | Graph metrics plus focused tests | 22-23 | Pending | +| Cross-cutting efficiency | Graph metrics, workflow traces, and focused latency/allocation diagnostics | 22 | Partly insufficient. Fixed-size registries, service-sized derivation, template parsing, batch composition, and ordinary profile fan-out are proportionate. `AUD-065` records one redundant Weather API request plus serial independent source fetches; `AUD-066` records repeated full comparison-bundle reads during replacement. | +| Cross-cutting maintainability | Graph metrics plus focused structural review | 23 | Pending | | Documentation and executable-contract coherence | Canonical documents, code, schemas, templates, examples | 24 | Pending | | Dynamic robustness and diagnostic checks | Repository-wide deterministic checks | 25 | Pending | @@ -2805,6 +2806,97 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: `AUD-058` - Remediation reference: pending +### AUD-065: Weather collection duplicates one source and serializes independent requests + +- Stage: 22 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: efficiency +- Area: `internal/adapters/weatherapi.Client.FetchBundle`, `warmup`, and the + eight `bundleBuilder.fetch*` calls +- Evidence: Every collection first performs a warmup GET against the default + `/conditions/current` endpoint, discards its body, and immediately fetches + `/conditions/current` again as one of eight source products. The remaining + seven products are independent HTTP endpoints, but `FetchBundle` waits for + all eight in a fixed serial chain. A temporary controlled diagnostic added + 25 milliseconds of server latency to every healthy endpoint: five runs took + 236-240 milliseconds apiece, made nine requests, and requested current + conditions twice. The diagnostic was removed after measurement. +- Contract at risk: Collection should preserve source policy, deterministic + normalized output, and prompt cancellation without making healthy latency + the sum of independent network round trips or issuing a request whose data + is always discarded. +- Impact: For approximately equal endpoint latency `L`, the current healthy + transport floor is about `9L`; one full `L` and one upstream response are + unconditionally redundant, while serialization adds roughly another `7L` + over concurrent independent acquisition. Generate, each batch, and each + comparison collect once, so every user-visible workflow pays this cost even + though later model latency is separate. Slow optional products also delay + all otherwise usable source results and their eventual policy decision. +- Recommendation: Remove the duplicate current-conditions warmup, or make the + successful current fetch satisfy readiness as well as collection. Acquire + independent products with a bounded, cancellation-aware group into + source-local results, then merge them in canonical source order so warning, + provenance, required-source, and error-precedence behavior stays explicit. + Do not share-mutate the current builder from request goroutines. +- Test implications: Add a delayed local-server case proving independent + requests overlap and current conditions are requested once. Retain focused + cases for warmup/retry policy if a distinct readiness operation remains, and + assert deterministic warning/provenance order, required-source failure, + optional-source policy, cancellation, and worker joining. +- Validation: A healthy collection performs only necessary requests and its + post-readiness duration approaches the slowest independent source rather + than their sum; existing normalized fixture output and failure policy remain + byte-for-byte or semantically unchanged as appropriate. +- Related findings: `AUD-013`, `AUD-015` +- Remediation reference: pending + +### AUD-066: Comparison replacement repeatedly rereads complete bundles + +- Stage: 22 +- Status: candidate +- Severity: low +- Confidence: high +- Category: efficiency +- Area: `internal/app.compareDetailed`, + `internal/comparison.PlanDestination`, `RecognizeBundle`, and `publish` +- Evidence: Replacing an existing bundle invokes `PlanDestination` before + prompt work, again after execution, at entry to `publish`, and once more at + the commit boundary. Every nonempty replacement plan calls + `RecognizeBundle`. One recognition reads the manifest once to decode it, + reads every declared directory entry in full (including that manifest and + the data package), then reads the data package a second time for its digest. + For `N` successful reports, one ordinary replacement therefore performs + four recognitions and `4N+16` complete file reads: even a two-profile bundle + causes 24 reads, while a ten-profile bundle causes 56. Each report is read + four times and the manifest and data package eight times. +- Contract at risk: Early destination rejection and commit-time + reauthorization are necessary, but each authorization pass should perform + only the content work needed to establish the recognized-bundle invariant. +- Impact: Small local bundles make the overhead modest, but the cost scales + linearly with profile count and full artifact size, including generated + Markdown already unbounded under `AUD-038`. Replacement on slower or remote + filesystems multiplies both I/O and pre-publication latency; the adjacent + post-execution and publisher-entry checks repeat the same scan with almost + no intervening stateful work. +- Recommendation: Preserve one early preflight and a final commit-time + reauthorization, but remove or combine adjacent ownership checks after + defining which layer owns each. Within recognition, decode the manifest and + hash the data package from their first reads, and validate report entries by + metadata plus a minimal open/readability check unless the bundle contract + adds report-content digests. Keep the final namespace/type checks needed by + the existing replacement threat model. +- Test implications: Instrument file-open/read operations or use a focused + filesystem seam to assert bounded passes for two and many reports. Retain + all current symlink, unexpected-entry, digest, concurrent replacement, + rollback, and commit-time reauthorization cases. +- Validation: Replacement performs one deliberate early scan and one + deliberate commit authorization without rereading any artifact inside a + scan; recognized and adversarial bundles retain their current outcomes. +- Related findings: `AUD-038`, `AUD-053`, `AUD-055` +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -3382,6 +3474,47 @@ template changes reviewable without memorializing incidental whitespace. Retain that style while consolidating the cross-owner render checks in `AUD-063`. +### RET-050: Keep service-sized derivation and registries straightforward + +Graph loop propagation led through daily context construction into fact and +forecast selection, but the concrete workload is bounded by four default +dayparts and Weather API runs of a few hundred periods. The work is linear per +selection, and Stage 7 already counted only low-thousands of simple overlap +checks for an ordinary derived build. Report and generated-text registries +contain four definitions, while the module registry is a fixed small catalog; +rebuilding those values is not a credible hot path at current cardinalities. +Retain the direct loops and explicit registries. Reconsider indexing or caching +only if forecast horizons, configured dayparts, or registries become dynamic +and materially larger. + +### RET-051: Keep embedded template parsing until render volume changes + +`reporttemplate.Render` reads and parses one top-level template plus four +embedded partials on each call. A temporary benchmark that deliberately +reached execution failure only after parsing measured 0.283-0.291 milliseconds +and about 91 KiB across 1,898 allocations per render on the audit host. The +fixed three-report batches therefore spend under one millisecond parsing, and +ordinary comparisons select only a handful of profiles; provider execution is +orders of magnitude slower. Keep the simple per-call ownership and error +context for now. A pre-parsed immutable set becomes justified if rendering +moves into a high-volume service loop or profiles routinely number in the +hundreds. The benchmark file was removed after measurement. + +### RET-052: Keep prepare-once profile execution and fixed batch sequencing + +Comparison builds facts, modules, and prompt input once, then starts one +profile-local execution per explicitly selected profile; the durable order is +held in a preallocated result slice and Promptkit owns provider capacity. +Per-profile JSON cloning and rendering provide mutation isolation, while the +documented and example workload is a human-selected comparison of at least two +profiles rather than an unbounded service queue. Morning and evening batches +likewise contain a fixed small report set and intentionally preserve +independent sequential publication before one notification. Retain both +designs at current workload. Treat routine selections in the tens or hundreds, +observable clone contention, or a larger dynamic batch as triggers for a +bounded worker design rather than inferring a defect from goroutine or graph +counts alone. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -3757,6 +3890,25 @@ Stage 21 routed these investigation leads to their assigned later stages: recorded by Stage 20; Stage 21 did not treat it as a durability or test-count issue. +Stage 22 routed these investigation leads to their assigned later stages: + +- Fixed registries are cheap but are reconstructed through several public + helpers. Stage 23 should decide ownership and API shape from duplication and + coherence evidence, not introduce caches for a performance problem Stage 22 + did not find. +- `AUD-065` needs deterministic error precedence and ordered provenance if + acquisition becomes concurrent. Stage 23 may identify a small result-merge + abstraction; Stage 26 should preserve Stage 6 source-policy findings instead + of treating concurrency alone as the remediation. +- `AUD-066` must retain early rejection and commit-time namespace + reauthorization. Stage 23 should clarify app-versus-publisher ownership, and + Stage 25 should exercise concurrent destination changes before Stage 26 + consolidates reads. +- Profile fan-out remains proportionate for documented human-selected sets + under `RET-052`. Stage 24 should not imply an arbitrary scalability promise; + Stage 26 may consider a limit or worker bound only if operational evidence + establishes a larger supported workload. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -4931,3 +5083,56 @@ Stage 21 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-047` through `RET-049`. - Open questions: the four leads recorded above are routed to their assigned later stages. + +### Stage 22: Audit Cross-Cutting Efficiency And Complexity + +- Status: Complete. +- Scope reviewed: production graph complexity, fan-in/fan-out, loop depth, and + repeated work; Weather API collection; forecast/fact/module construction; + prompt-input serialization; generated-text validation and rendering; + prepare-once execution; single and fixed-batch workflows; comparison profile + fan-out; comparison bundle construction, recognition, and transactional + publication; fixed registry construction; allocation and I/O boundaries; and + realistic service, report, profile, and artifact cardinalities. +- Exclusions: structural extraction, API simplification, and code deduplication + remain Stage 23; documentation promises remain Stage 24; adversarial and + dynamic robustness remain Stage 25; no remediation was implemented. No + production code or tests were changed. + +#### Efficiency And Workload Accounting + +| Candidate | Realistic workload and measured/defensible cost | Disposition | +| --- | --- | --- | +| Weather source acquisition | Every workflow collects once. Healthy collection performs a discarded `/conditions/current` warmup, fetches current again, and serially fetches seven other independent products. With equal latency `L`, the transport floor is about `9L`. Five controlled 25-millisecond-delay runs took 236-240 milliseconds, made nine requests, and hit current twice. | Credible user-visible latency and redundant upstream work under `AUD-065`. | +| Forecast and fact derivation | Weather runs contain a few hundred periods; four default dayparts cause low-thousands of simple linear overlap/selection checks. The graph's propagated depth of three or four reflects composed scans, not one polynomial nested loop over the same growing collection. | Proportionate under `RET-050`; no finding. | +| Registry and catalog construction | Report/generated-text catalogs contain four definitions and the module catalog is fixed and small. Construction occurs per command/report boundary but does not grow with weather data or generated output. | Retain direct construction under `RET-050`; Stage 23 owns API/duplication questions. | +| Prompt input and generated validation | One package is built and YAML-serialized per prepared report. Generated output is decoded, semantically normalized, and encoded once per execution; no runtime schema compiler or repeated schema parse exists. Payload-size risk is already `AUD-038`. | Linear necessary work; no separate efficiency finding. | +| Template rendering | Every render reparses one template and four partials. A temporary parse-through-execution benchmark measured 0.283-0.291 ms and about 91 KiB/1,898 allocations per render. Fixed batches contain three reports and ordinary comparisons a handful of profiles. | Measurable but immaterial at supported volume; retain under `RET-051`. | +| Prepared comparison fan-out | Facts/modules/input are prepared once. Each explicit profile receives isolated JSON-cloned render inputs, one render, and one result slot; one goroutine starts per profile while Promptkit owns provider capacity. Documentation and examples describe human-selected sets beginning at two profiles. | Proportionate at current scale under `RET-052`; watch tens/hundreds rather than inventing a metric-only limit. | +| Fixed batch sequencing | Morning/evening batches collect once and sequentially prepare, execute, and publish a fixed three-report set before one notification. Provider calls dominate, but concurrency would change backend-capacity and partial-publication behavior for a bounded gain. | Retain under `RET-052`; no finding from sequentiality alone. | +| Comparison replacement recognition | Existing recognized destinations are fully scanned four times. Each scan performs `N+4` full reads for `N` successful reports, so ordinary replacement performs `4N+16` reads: 24 for two profiles and 56 for ten. Reports are read four times; manifest and data package eight times. | Credible scaling and filesystem-I/O issue under `AUD-066`; preserve early and commit-time authorization. | + +#### Commands And Evidence + +- Used graph complexity queries, search, symbol snippets, and call traces to + inspect the highest propagated loop depths and production fan-in/fan-out, + then followed the concrete generate, batch, comparison, collection, + derivation, rendering, and publication paths rather than treating graph + scores as findings. +- Reconciled prior-stage efficiency leads: service-sized derivation remained + bounded; template reparsing was measured and retained; extreme profile + counts remained a watch condition; full comparison reads became `AUD-066`; + and production waits were separated from the test-only delays in `AUD-062`. +- Ran a temporary local Weather API diagnostic with 25 milliseconds of + deterministic server latency for every healthy endpoint. Five runs took + 236-240 milliseconds, issued nine serial requests, and fetched current + conditions twice. Removed the diagnostic file immediately afterward. +- Ran a temporary report-template benchmark five times. Parsing through the + execution boundary took 0.283-0.291 milliseconds with about 91 KiB and 1,898 + allocations per call. Removed the benchmark file immediately afterward. +- Ran `go test ./...`, `go vet ./...`, + `go run ./cmd/weatherreporter --help`, and `git diff --check`; all passed. +- Findings: `AUD-065` and `AUD-066`. +- Retained decisions: `RET-050` through `RET-052`. +- Open questions: the four leads recorded above are routed to their assigned + later stages.