diff --git a/docs/roadmap/audit/05-resources.md b/docs/roadmap/audit/05-resources.md new file mode 100644 index 0000000..7b3ca86 --- /dev/null +++ b/docs/roadmap/audit/05-resources.md @@ -0,0 +1,132 @@ +# Resource management audit + +Status: one open P3 memory-retention finding. Reviewed source: +`8fe8b67b29bbd96e321a09d797b3df5a21cfd0bf`. + +## Architecture and resource ownership + +The [internal overview](../../internal/overview.md) inventories the implemented +owners. Core retains captured command declarations and immutable routing metadata +per App, constructs fresh backend state per invocation, prepares typed inputs, +and owns shared reporting/finalization. ckconfig coordinates configuration scopes +and comparisons; ckprompt adapts PromptKit inspection, request, and smoke +operations. Applications own executable lifecycle, contexts, loaders, builders, +runtime dependencies, and factory results. ConfigKit and PromptKit own their +native operations and resource handling. Release launchers delegate to ReleaseKit. + +The [architecture policy](../../policy/architecture.md) and public GoDoc govern +those boundaries. In particular, [Reporter](../../../reporter.go) borrows its +destinations: it closes its reporting handles before Execute returns, but does +not close or flush caller-owned writers. Releasing a reference and closing a +writer are separate operations. + +| Resource | Observed ownership and lifecycle | +| --- | --- | +| Files and network connections | Library runtime code opens no files or connections directly. Application loaders and adopted upstream operations supply configuration, artifacts, engines, and client effects. CommandKit does not assume ownership of their closers. | +| Contexts, timers, and workers | The caller supplies the invocation context. CommandKit creates no runtime cancel functions, timers, goroutines, channels, or worker pools. Integration callbacks and aggregate operations run synchronously; cancellation is cooperative at the selected checkpoints. | +| Command declarations and metadata | Captured bindings/callbacks and routing metadata have App/declaration lifetime. Backend sessions and parsed values are invocation-local; metadata caching does not store those mutable sessions on App. | +| Report buffers and pending diagnostics | Native conversion buffers, normalized report values, and projections are scoped to preparation/delivery. Deferred detail closures remain pending until finalization; processed queue entries are cleared and the queue is detached/cleared. Closed destination references remain retained as described in RM1. | +| Examples and tooling fixtures | Example caches are invocation state supplied by the application. The completion logging fixture explicitly closes its file; Go harnesses run and wait for subprocesses, and the interactive Python driver closes its shell in finally. Temporary fixture directories use t.TempDir. Release launchers use exec rather than starting background workers. | + +The review covered normal, error, and cancellation exits, retained callbacks and +binding values, destination ownership, buffers/encoders, registration/snapshot +replacement, integration acquisition/comparison lifetimes, and fixture cleanup. +Codebase-memory-mcp symbol/source search and call tracing were supplemented by +direct inspection of examples, shell/Python tooling, and acquisition/cleanup +calls. The indexed Go source matches the reviewed revision. + +## Validation evidence + +Validation used Go 1.27.0 and existing dependencies, with dependency downloads +disabled: + +- The aligned workspace suites passed with + `GOTOOLCHAIN=local go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`. + Successful cached results were reused for unchanged code, including maintained + examples and launcher tests. The optional interactive completion checks were + not enabled for this pass. +- A temporary external consumer used standard weak pointers and runtime.GC to + examine destination reachability after Execute. It retained one reporting + handle at a time and used runtime.KeepAlive to ensure that handle remained live + during the observation. Dropping it supplied the collection control. This was + a reachability check, not a performance benchmark or heap-size measurement. +- Existing reporter tests were inspected for caller-owned writer handling, + finalization/late-write behavior, sticky destination errors, and deferred + callback release. Other acquisition and cleanup paths were reviewed statically. + +No production files or repository tests were changed. No external services, +credentials, or publication operations were used. + +## RM1 — P3: Closed reporting handles retain both destination objects + +**Code:** [reporter.go](../../../reporter.go), reportingState/destinationState +lines 38–57; newReporter lines 106–113; Output/managedOutputWriter lines 144–156; +AtEnd lines 226–231; Reporter.finalize lines 282–329, particularly closure at +324–326. The private close helper at 679–685 also leaves destination references +intact. + +**Circumstances:** a caller retains a Reporter, its Output writer, or an +AtEnd view after Execute returns, while otherwise releasing the destinations. +For example, a longer-lived worker or job record may keep a handle so late +submissions can receive ErrReporterClosed. All three handles retain the same +reportingState, which continues to hold both Streams.Out and Streams.Err even +after finalization. No successful future operation needs those destination +objects: closed operations reject before using them. + +**Observed behavior:** the temporary consumer supplied separate output and +diagnostic writer objects, each carrying a 1 MiB payload. It retained only one +closed handle, with weak references to the two destinations. For each of the +three handle kinds, it exercised successful, failed, and canceled invocations. +Every late submission correctly returned ErrReporterClosed, but both destination +objects remained reachable after garbage collection in all nine cases. After +dropping the handle and collecting again, both weak references became nil in +all nine cases. + +The reference chain is: + +```text +retained closed handle + → reportingState + → output.writer + → diagnostics.writer +``` + +**Potential consequences:** completed jobs can retain large output buffers, +compression state, or other writer-owned object graphs for the lifetime of an +otherwise small closed handle. Even keeping only Output retains the diagnostic +destination. Across N retained handles, retained destination memory can grow +with the sum of those objects' sizes. This is conditional retention, not an +unreachable-heap leak: dropping the handles releases the references. File and +connection closure remains the caller's responsibility; the observation does +not establish a CommandKit-owned descriptor leak. + +**Potential remediation:** after final presentation is captured and the shared +state is closed, clear its direct destination writer references under the +existing mutex, or replace them with a lightweight discard destination. Keep +the shared closed state so all copied handles continue to return +ErrReporterClosed. Do not call Close or Flush on borrowed writers. Preserve +returned presentation observations and error identity; errors may legitimately +retain their own referenced data independently. Apply consistent reference +release to any private closure helper that remains relevant. + +This improves resource lifetime without adding a public garbage collection +guarantee. Future regression coverage should check that closure +releases unnecessary direct references while preserving caller ownership, +late-operation errors, and presentation evidence. + +## Other reviewed costs and limits + +Whole-report JSON normalization, YAML adaptation, aggregate report preparation, +and pending deferred messages require memory proportional to their selected +content and representations. Rendering still runs in quiet mode to retain +rendering failures. These are selected behaviors; this audit does not infer +arbitrary report-size limits, queue truncation, or provider cleanup ownership. +The [future efficiency record](../future.md#possible-further-efficiency-work) +preserves buffering/reuse proposals, and the +[efficiency audit](02-efficiency.md) separately records avoidable allocation and +repeated-work opportunities. Those findings remain separate assignments. + +No additional file-descriptor, network-connection, timer, goroutine, or child +process cleanup defect was established in CommandKit-owned code. The +[concurrency audit](04-concurrency.md) records the existing queue-acceptance and +cancellation defects; they do not establish additional resource leaks here.