From 6b1323d1fe8457b7e57957590eaa807973c01084 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 4 Nov 2026 14:02:50 +0000 Subject: [PATCH] Record resource management audit findings --- docs/roadmap/resources.md | 215 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 docs/roadmap/resources.md diff --git a/docs/roadmap/resources.md b/docs/roadmap/resources.md new file mode 100644 index 0000000..eb5cc71 --- /dev/null +++ b/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.