165 lines
8.5 KiB
Markdown
165 lines
8.5 KiB
Markdown
# Performance Implementation Remediation
|
||
|
||
**Status:** Remediation in progress; R1–R3 complete.
|
||
|
||
**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.
|
||
|
||
**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](../../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.
|
||
|
||
**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](../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.
|
||
|
||
**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](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.
|