Record the resource management audit
This commit is contained in:
132
docs/roadmap/audit/05-resources.md
Normal file
132
docs/roadmap/audit/05-resources.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user