Record resource management audit findings

docs/roadmap/resources.md

@@ -0,0 +1,215 @@
+# Resource Management Audit
+
+Reviewed 2026-11-04 against commit `fb87fab`.
+
+## Scope And Evidence
+
+Reviewed file and response-body ownership, bounded reads, HTTP client/transport
+lifetimes, timeout contexts and retry timers, backend admission and permits,
+prepared-handle cleanup, retained snapshots, JSON conversion, rendering buffers,
+schema resources, and repair feedback. The architecture uses immutable
+engine-lifetime configuration, invocation-local execution/validation resources,
+and caller-owned public results. Production code launches no PromptKit-owned
+goroutines or background workers requiring a shutdown protocol.
+
+Evidence consists of source/call-graph review, relevant existing tests, and a
+small temporary offline diagnostic outside the repository. The diagnostic used
+a controlled HTTP transport, counted body reads/closure, inspected the existing
+prepared-handle owner reference, and compared string backing addresses. No
+benchmark, heap-size measurement, deliberate exhaustion, live network call,
+production change, or new repository test was needed.
+
+Focused existing tests passed for content-file closure, HTTP error-body limits
+and closure, independent attempt contexts/body closure, read cancellation,
+prepared discard/ownership, diagnostic formatting, synchronous schema
+cancellation, and repeated-value JSON work bounds. These tests establish the
+existing protected behavior; they do not cover all findings below.
+
+## 1. Authentication Diagnostics Read An Unbounded Upstream Body
+
+**Priority:** P2. Unbounded remote-response consumption; a finite limit would be
+a new policy decision, not enforcement of an existing documented byte limit.
+
+**Code:** [runOpenRouterKey](../../internal/usecase/smoke_authentication.go:66),
+[recognizedOpenRouterKeyResponse](../../internal/usecase/smoke_authentication.go:139),
+and [diagnostic HTTP execution](../../internal/llm/http_evidence.go:129).
+
+**Circumstances:** An authentication endpoint returns HTTP 200 with a large or
+continuously growing body. Recognition calls unrestricted `io.ReadAll` and then
+unmarshals the whole document into raw-message maps. Neither declared length nor
+streamed bytes are bounded on this path. Generation's 16 MiB success-body limit
+and 64 KiB error-body limit do not apply to diagnostic GETs.
+
+**Consequences / evidence:** The offline diagnostic supplied a valid small
+identifying `data` object plus 4 MiB of irrelevant padding. The probe read all
+4,194,375 bytes, closed the body, and reported credential acceptance. Increasing
+the padding has no corresponding byte-budget check. A fast misconfigured or
+unexpected endpoint can consume substantial memory before its timeout; concurrent
+authentication probes multiply consumption because HTTP-only diagnostics bypass
+generation admission. A timeout bounds elapsed time, not bytes allocated within
+that time. There is no demonstrated descriptor leak: the body is closed.
+
+**Potential remediation:** Select a finite body budget appropriate to this small
+diagnostic protocol, enforce it for both declared and streamed/underreported
+lengths, and close without unbounded draining on overflow. Preserve single-JSON
+framing and the distinction between unknown acceptance and observed rejection;
+an oversized response must not be reported as credential rejection. Decide the
+budget and overflow report classification explicitly, then update the owning
+contract and tests. This need not impose new limits on application prompts,
+artifacts, or generated output.
+
+## 2. Convenience Source Conversion Expands Shared Graphs Before Bounded Loading
+
+**Priority:** P2. Potentially exponential allocation before upstream resource
+checks can run.
+
+**Code:** [providerMapForMapping](../../provider_values.go:19),
+[providerValueForMapping](../../provider_values.go:31),
+[ProfileSource](../../profiles.go:33), and
+[BackendSource](../../backends.go:100).
+
+**Circumstances:** A caller builds a small acyclic Go value graph containing
+repeated map/slice references. Each level can point twice to the same child:
+
+```go
+value := map[string]any{"value": true}
+for range depth {
+    value = map[string]any{"left": value, "right": value}
+}
+```
+
+The source converter tracks only active ancestors to identify cycles. Once one
+branch finishes, its visit is removed, so the sibling independently converts
+the same entire child. It allocates a fresh expanded tree before calling
+`configkit.Mapping`. For d levels, O(d) distinct input maps become
+Theta(2^d) output maps. The depth guard does not constrain width or total work;
+this expansion can become impractical well below depth 100.
+
+**Consequences:** Programmatic source creation can exhaust memory before
+`NewEngine` or ConfigKit's deferred diagnostics are reached. This is not a claim
+that ConfigKit lacks bounds: the selected ConfigKit v1.1.0's mapping builder
+checks node, byte, and depth budgets during its own snapshot traversal
+(`internal/engine/value.go`, `valueBuilder.build`). It is called only after
+PromptKit has already expanded the graph. PromptKit's later `jsonvalue` node
+budget is also too late to protect this conversion.
+
+**Potential remediation:** Memoize completed structural conversions so repeated
+input containers reuse already-converted containers until ConfigKit snapshots
+them with its existing bounds. Keep active-ancestor tracking distinct from the
+completed memo so cycles still become deferred source errors. Account correctly
+for container identity, including distinct slice views. Alternatively, seek
+upstream support for the additional Go value forms so this conversion can be
+removed. Preserve immediate source ownership and deferred failure behavior;
+do not introduce a PromptKit raw-source validation pass or duplicate ConfigKit's
+configuration limits to fix this pre-handoff allocation problem.
+
+## 3. Inert Prepared Handles Retain Their Entire Execution Owner
+
+**Priority:** P3. Avoidable retained-memory dependency after use/discard.
+
+**Code:** [PreparedExecution.owner](../../internal/usecase/prepared_execution.go:24),
+[Discard](../../internal/usecase/prepared_execution.go:96),
+[claim](../../internal/usecase/prepared_execution.go:172), and
+[payload clearing](../../internal/usecase/prepared_execution.go:188).
+
+**Circumstances:** A consumer retains a completed or discarded handle for its
+supported `Details` access, while replacing or dropping the originating engine.
+Payload clearing releases the per-execution plan, credential, observer, and
+execution snapshot, but the handle's `owner *Runner` remains unchanged.
+
+**Consequences / evidence:** The reference path is
+`handle -> Runner -> captured repositories/clients/validator/capacity manager`.
+Thus a small inert handle keeps the originating engine's complete runtime
+snapshot and collaborators reachable, not just its deliberately retained details.
+The offline diagnostic confirmed the same nonnil runner pointer after both
+discard and successful execution. Retaining handles from successive engine
+reconstructions can retain successive large snapshots. Keeping details itself
+is required behavior, and this has no additional owner-retention cost while the
+application still retains the same engine anyway.
+
+**Potential remediation:** Clear the owner reference when the handle becomes
+inert, checking ownership under the same mutex to avoid racing the current
+unlocked owner read. Another option is a small standalone identity token shared
+by a runner and its handles, which establishes ownership without retaining the
+runner graph. Such a token must have reliable distinct identity; do not rely on
+distinct zero-sized allocations comparing unequal. Preserve related namespace
+views, foreign-owner rejection, single claim, discard/run races, and details
+availability. No engine `Close` method or automatic closing of caller-owned
+collaborators is implied.
+
+## 4. Repair Feedback's Output Bound Does Not Bound Its Temporary Serialization
+
+**Priority:** P3. Avoidable memory amplification on validation failures.
+
+**Code:** [formatRepairDiagnostics](../../internal/usecase/repairer.go:119).
+
+**Circumstances:** A failed schema validation produces a large diagnostic or many
+diagnostics and repair is enabled. The formatter normalizes every string, marshals
+the complete array, then discovers that it exceeds 64 KiB. It subsequently
+marshals individual full entries again before truncating one of them.
+
+**Consequences:** Although the returned feedback is bounded correctly, temporary
+storage scales with the complete diagnostic text, including text omitted from
+the request. JSON escaping can amplify its size, and normalization adds another
+copy for invalid UTF-8. Large validation messages and concurrent repairs can
+therefore create large additional buffers despite producing tiny feedback.
+The complete diagnostics already exist for the validation result; these extra
+full encodings do not help preserve them. Existing formatting tests check output
+bounds/data preservation, not scratch-space use.
+
+**Potential remediation:** Encode feedback incrementally with bounded buffering
+and lookahead sufficient to distinguish a complete array from an omitted suffix.
+Stop encoding omitted text rather than allocating its full JSON representation.
+Keep the existing 64 KiB result bound, ordered entries, rune-safe truncation,
+valid UTF-8, JSON escaping, and in-bound omission notice. Leave the original
+validation diagnostics and full previous candidate unchanged. This is an
+implementation opportunity, not a claim that the existing contract promises a
+64 KiB peak heap footprint or permission to add a diagnostic-input limit.
+
+## 5. Short Normalized Session IDs Can Pin Large Input Strings
+
+**Priority:** P3. Avoidable backing-buffer retention in retained metadata.
+
+**Code:** [NormalizeSessionID](../../internal/domain/session.go:10),
+[preparation](../../internal/usecase/runner.go:312), and
+[prepared metadata construction](../../internal/usecase/runner.go:491).
+
+**Circumstances:** A direct or rendered session ID contains substantial leading
+or trailing whitespace around a short valid identifier. `strings.TrimSpace`
+returns a substring; normalization validates that substring and returns it
+without detaching its backing storage. Public conversion preserves the string.
+
+**Consequences / evidence:** An offline public-API probe prepared a request with
+1 MiB of spaces followed by `s`. The returned session ID had length one but its
+backing address still pointed inside the large original allocation. Retaining a
+prepared value, handle details, or run result can therefore retain that buffer
+after the original request/rendering buffer would otherwise be collectible.
+The 256-code-point semantic limit does not prevent this retention. This does not
+require changing which padded identifiers are accepted.
+
+**Potential remediation:** Detach accepted normalized session metadata with a
+small owned string, for example `strings.Clone` after validation. Preserve empty
+omission, Unicode trimming/counting, hashes, and wire content. Review other
+short canonical metadata returned by trimming for the same pattern when making
+the change; do not indiscriminately clone large prompt/message payloads or add a
+new raw-session-size restriction.
+
+## Cleanup Checks And Non-Findings
+
+No additional descriptor, timer, connection-ownership, or goroutine leak was
+established. Artifact/schema handles use deferred closure; prompt content handles
+close after reading; generation response bodies close on success and decoding,
+status, and size failures. Diagnostic bodies close, and their timeout cancellation
+is attached to body closure. Retry timers stop on exit. Admission/active permits
+release on errors, cancellation, and panic unwinding. Prepared payloads otherwise
+clear as intended.
+
+Caller-selected artifact/schema/prompt sizes, intentionally unlimited backends,
+and caller-retained ready handles are not classified as bugs merely because
+their total memory can grow. Synchronous dependency calls may delay cancellation
+by design, rather than being detached into leaking goroutines. Borrowed HTTP
+transports/filesystems/clients remain subject to their ownership contracts; this
+audit proposes no blanket shutdown or disposal API.
+
+Previously recorded repeated-copying issues remain in
+[the efficiency audit](audit/02-efficiency.md); they are not duplicated here.