9.2 KiB
Performance Implementation Remediation
Status: Complete; R1–R4 addressed.
Review baseline: df064bd, reviewed on 2026-10-09 against the
performance target and
implementation plan.
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, testing, and documentation policies govern the follow-up.
R1: Preserve Operation Cancellation Through Reader Shutdown
Priority: High.
Owners: candidate tree, validation executor, and Git batch reader.
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.
Completion: Complete. The tree retains its operation context for the reader lifetime. An EOF-synchronized regression now verifies shutdown cancellation, cancellation identity, and child reaping; it failed before the fix and passes afterward. Race tests passed for validation, Git, and process packages, including existing read cancellation, successful closure, and primary-failure coverage.
R2: Attribute Shutdown Failures To A Check That Read Content
Priority: Medium.
Owner: validation executor.
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.
Completion: Complete. Internal blob-read bookkeeping now identifies the last check that actually used the reader. The extended close-failure fixture preserves Go-format attribution when Markdown has no files, including the child exit code and stderr. The regression failed before the fix and passes afterward; validation race tests and compiled public API and dependency checks passed.
R3: Qualify Output Allocation Measurements By Their Workload
Priority: Low.
Owners: Go execution documentation, output benchmark, and the implementation results.
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.
Completion: Complete. The benchmark now includes equal 8 MiB output in one write and 32 KiB chunks, with untimed byte-for-byte capture and forwarding checks. Three repetitions passed and measured about 8.39 MB/2 allocations and 16.74 MB/10 allocations respectively. Go-output documentation and implementation results qualify the single-write figures and distinguish allocation totals, retained output, and unmeasured peak memory.
R4: Reconcile Feature And Implementation Status
Priority: Low.
Owner: performance roadmap.
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.
Completion: Complete. The feature roadmap identifies all five implemented components and links to their implementation results and review remediation. The implementation record links to completed remediation. Status, ownership, and local navigation across all three documents were checked against the code and completed R1–R3 work.
Remediation Validation
- The full offline Go test suite passed after the production fixes.
- Compiled public API and dependency checks passed.
- Race tests passed for validation, Git, and process packages.
- The output benchmarks passed three repetitions, including exact capture and forwarding checks for both chunking patterns.
- Changed documentation links, fragments, and whitespace checks passed.
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.