From 15dd58f5ac0daa5de2e9bd9558f34a6f980bc3d2 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 9 Oct 2026 02:15:56 +0000 Subject: [PATCH] Record performance implementation review findings --- docs/roadmap/remediation.md | 145 ++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..12fb28b --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,145 @@ +# Performance Implementation Remediation + +**Status:** Required follow-up; remediation pending. + +**Review baseline:** df064bd, reviewed on 2026-10-09 against the +[performance target](performance.md) and +[implementation plan](implementation.md). + +## Scope And Architecture + +The five intended improvements are present behind the existing public facades: +an internal piped-process boundary and lazy Git batch reader, operation-local +validation result caches, consolidated local asset preflight, completed Go-output +buffer ownership, and a cached runtime metadata fallback. Public API and +dependency checks pass. This review found no added public input limits, release +gates, or product restrictions in the implementation commits. + +The findings below cover correctness and documentation gaps in that work. +Implement fixes within the existing architecture and preserve the established +public contracts. The [agent](../policy/agents.md), +[testing](../policy/testing.md), and +[documentation](../policy/documentation.md) policies govern the follow-up. + +## R1: Preserve Operation Cancellation Through Reader Shutdown + +**Priority:** High. + +**Owners:** [candidate tree](../../internal/validation/tree.go), +[validation executor](../../internal/validation/validation.go), and +[Git batch reader](../../internal/git/blob_reader.go). + +CandidateTree.readBlob creates its reader with context.Background(). +BlobReader.Read registers cancellation for an individual exchange and removes +that registration when the exchange finishes. The executor's deferred cleanup +then calls tree.Close(), whose stdout drain and process wait have no connection +to the validation operation's context. + +Consequently, cancellation while the child is shutting down cannot interrupt +that cleanup. Validation can remain blocked after its caller cancels. If the +child is subsequently terminated independently, the resulting error reports +the process exit and loses the caller's cancellation cause. + +**Evidence:** A temporary offline fixture served valid committed Go content, +waited until the executor closed batch stdin, signalled readiness, and stalled +before exiting. Cancelling Run at that point left it blocked. After the probe +terminated the fixture child, Run returned a Go-format cleanup failure for +"signal: killed"; errors.Is(err, context.Canceled) was false. The existing +TestRunPreservesCancellationDuringReaderCleanup cancels during an unfinished +blob read, so it exercises the per-read callback rather than this shutdown gap. + +**Required outcome:** Associate shutdown with the validation operation context, +either through the reader's lifetime context or context-aware cleanup. Preserve +individual-read cancellation and the primary failure when a check has already +failed. Reap the child on all paths. Use the supplied context for cancellation; +this fix does not require a new runtime timeout or input restriction. + +**Validation:** Add a regression synchronized to batch-input EOF, cancel while +shutdown is still blocked, and verify completion, child cleanup, and preserved +cancellation identity. Retain coverage of read cancellation, successful closure, +and preservation of an earlier check failure. + +## R2: Attribute Shutdown Failures To A Check That Read Content + +**Priority:** Medium. + +**Owner:** [validation executor](../../internal/validation/validation.go). + +The lastContentCheck variable is assigned when a content-oriented check is +selected, even when that check has no applicable files and makes no content +request. Deferred cleanup therefore reports the last selected check rather than +the last check that used the reader, diverging from the implementation plan's +failure-attribution rule and the completion record's description. + +**Evidence:** A temporary fixture with only go.mod and main.go selected Go +formatting and Markdown checking. Only formatting read a blob. The child then +exited with code 23 during shutdown, and Run reported "markdown-links". + +**Required outcome:** Track actual reader use by each check and preserve the +last check that used content access when a later selected check performs no +reads. Keep check ordering and earlier failure attribution intact. Reader-use +tracking remains internal implementation bookkeeping. + +**Validation:** Extend the existing unexpected-close-failure fixture with a +later selected check that has no applicable files. Verify the earlier reader +user receives the cleanup cause and the child exit error remains discoverable. + +## R3: Qualify Output Allocation Measurements By Their Workload + +**Priority:** Low. + +**Owners:** [Go execution documentation](../internal/gotool.md), +[output benchmark](../../internal/gotool/performance_test.go), and the +[implementation results](implementation.md). + +The internal Go-output document says an 8 MiB forwarded capture currently +allocates about 8.39 MB per operation. The cited benchmark makes one 8 MiB +Write call. Actual command output can arrive in smaller chunks, which incur +additional bytes.Buffer growth allocations. The statement needs its workload +qualification to describe the measured behavior accurately. + +**Evidence:** Temporary component probes using the current writer and io.Discard +measured approximately 8,388,691 B/op and two allocations for one 8 MiB write, +versus 16,744,535 B/op and ten allocations for the same total output in 32 KiB +writes. These measurements exclude subprocess overhead and failure diagnostic +strings. Both paths correctly avoid the former final capture copy. + +**Required outcome:** Identify the single-write workload wherever that figure +is quoted. Include a representative chunked capture measurement in the benchmark +set and results. Distinguish allocated bytes across an operation from retained +output bytes and peak memory. Complete in-memory capture remains the selected +design; these findings do not require disk-backed diagnostics or a memory cap. + +**Validation:** Compare equal output lengths across the two chunking patterns +and confirm captured bytes and forwarding remain correct. Report allocation +results as workload-specific measurements with no numerical runtime guarantee. + +## R4: Reconcile Feature And Implementation Status + +**Priority:** Low. + +**Owner:** [performance roadmap](performance.md). + +The performance roadmap still says "Ready for implementation; implementation +pending." The implementation document marks Stages 1–11 complete, and the code +contains the five target components. Stage 11 explicitly called for updating the +feature roadmap status, and its completion record claims the work is complete. + +**Required outcome:** Update the feature roadmap to distinguish the implemented +target components from the outstanding review findings recorded here. Keep +remediation status in this document and performance results in the implementation +record, consistent with their documentation ownership. + +**Validation:** Check status and navigation across the three roadmap documents +against the actual implementation and remediation outcomes. + +## Review Validation + +- The full offline Go test suite passed; some unchanged packages used cached + results. +- The compiled public API and dependency checks passed. +- Focused race tests passed for internal/git, internal/process, + internal/runtimeinfo, internal/gotool, and internal/validation. +- The targeted probes used temporary Go overlays and local child-process + fixtures. They reproduced R1 and R2 and measured the workload distinction in + R3. No production code or permanent tests were changed during this review.