From 5925066d35795b2dc1d31d069a1fb23024a62ab8 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 16:37:05 +0000 Subject: [PATCH] Record resource management audit findings --- docs/roadmap/resources.md | 189 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 189 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..e1ee559 --- /dev/null +++ b/docs/roadmap/resources.md @@ -0,0 +1,189 @@ +# Resource Management Audit + +Reviewed revision: `0cca633a67abd3d0fdcb7db6e83260dcaf1eaac8`. +Audit date: 2026-10-25. + +## Scope And Architecture + +The audit covered input snapshots and readers, discovery, filesystem acquisition, +imports and selected overlays, typed projection and cloning, composition and +inheritance, callback teardown, retained inspection history, rendering/diff, and +secrets bootstrap. Knowledge-graph discovery and caller tracing were supplemented +by source inspection, existing tests, and small standalone diagnostics outside +the repository. No benchmarks or large-memory stress tests were conducted. + +The [implemented architecture](../internal/overview.md) has a mostly clear +resource-ownership model: + +- The public facade delegates to a synchronous engine lifecycle, with private + document and YAML packages. Loading creates no library-owned background + goroutines, timers, network connections, or shutdown service. +- Ordinary file reads close owned handles, including read/stat failures. Host + import and overlay acquisition own their root handles; caller-supplied readers + and filesystems remain borrowed. +- Published inspection retains immutable documents, history, indexes, and passive + resource contexts. A later explicit resource open transfers ownership of that + returned file to the caller; publication does not normally retain acquisition + handles. +- Retained history and detached query results are intentional ownership choices, + not leaks by themselves. The findings below concern enforcing existing limits + and closing owned resources, rather than removing that functionality. + +Three new findings warrant remediation. All remain open. + +## 1. Typed Containers Allocate Before Their Node Budget Is Enforced + +Priority: medium. Category: peak memory and failure handling. + +**Relevant code.** `projectSequence` and `projectMap` in +[projection.go](../../internal/engine/projection.go), `valueOperation.charge` in +[value_operation.go](../../internal/engine/value_operation.go), and the defaults +projection in [decode.go](../../internal/engine/decode.go). + +**Circumstances.** Sequence projection reserves `value.Len()` document pointers +before visiting any children. Map projection similarly reserves the complete +entry array and obtains all map keys before charging individual entries. The +existing node limit therefore rejects an oversized typed value only after a +potentially much larger allocation has already occurred. Defaults and values +returned by application resolution can reach these paths. + +**Evidence.** A standalone public-API diagnostic loaded `NoSource()` with defaults +containing an `Items []struct{}` field initialized with +`make([]struct{}, int(^uint(0)>>1))`. Zero-sized elements make the input slice +inexpensive, but projection immediately panicked with +`runtime error: makeslice: cap out of range`. It did not return the existing +`schema.projection_limit` diagnostic. No enormous backing array was successfully +allocated by this diagnostic. + +**Consequence.** Less extreme oversized containers can cause excessive temporary +memory allocation before rejection; sufficiently large lengths panic or exhaust +memory instead of reaching the intended limit failure. This is a library-created +allocation, distinct from memory an application has already allocated itself. + +**Potential remediation.** Before bulk reservations, compare the known minimum +node cost of the container against the operation's remaining existing allowance, +using overflow-safe arithmetic. Alternatively, bound initial capacity and grow +only as successfully charged elements are accepted. Review the analogous map +reservation/key collection. Preserve current node/depth semantics, inline-field +behavior, and useful diagnostic paths; do not introduce an unrelated container +length cap or promise a universal heap-size bound. + +## 2. Retained-History Accounting Omits Published Replay Trees + +Priority: medium. Category: retained memory and lifecycle integration. + +**Relevant code.** The history check in `executeLoadRequest` in +[load.go](../../internal/engine/load.go), `historyNodesWithin` in +[history.go](../../internal/engine/history.go), `preparePostDecodeOverrideContext` +in [override_context.go](../../internal/engine/override_context.go), and +`resolveLoadInput` / `publishLoadResult` in +[load_result.go](../../internal/engine/load_result.go). + +**Expected behavior.** The +[retained-history contract](../config.md#historical-value-explanation) limits retained +history to 4,000,000 distinct document nodes, counting shared nodes once and +reporting `explain.limit_exceeded` when exceeded. + +**Actual behavior.** The load check visits the original decoded baseline and +pre-CLI final tree, the resolved final tree, application trees, resolution deltas, +and the acquisition inheritance scope. Publication retains a different root set: +it includes a separately cloned post-CLI decoded tree as inspection input, plus +independent `structuralInput` and `preCLI` replay snapshots. The replay snapshots +are cloned again during publication and included in its immutable lookup. They +are not passed to the budget check. Inheritance operation copies are also made +after that check. Counting the earlier root set does not bound the distinct +nodes actually kept alive by the result. + +**Evidence.** A successful public load of `items: [1, 2]`, decoded into +`struct { Items []int }` with a no-op resolver, was inspected by a read-only +standalone reflection diagnostic. Its baseline/input/final/application roots +contained 14 distinct nodes; including the two published replay roots yielded +22. The checkpoint uses a different decoded-final instance of the same size in +this example. Source inspection establishes that its count is also 14. + +For this fixture generalized to N scalar items, the checkpoint counts `3N + 8` +nodes, while the published roots retain `5N + 12`. Thus 800,000 items would pass +the history checkpoint with 2,400,008 nodes yet retain 4,000,012. Such a source +also fits the existing per-source byte and node allowances. This larger case is +a structural derivation, not an executed allocation test. + +**Consequence.** Successful loads can exceed the established retained-history +limit, with additional retained lookup/index overhead. The quota currently gives +an incomplete account of the memory-owning snapshot graph. + +**Potential remediation.** Apply one pointer-aware accounting operation to the +complete set of document roots actually retained at publication, including replay +snapshots and copied inheritance history. Preserve count-once semantics for +shared nodes. Where practical, charge or reserve these copies before creating +them so failure does not first incur avoidable peak allocation. Reusing an +already-owned immutable snapshot can be considered where equivalent, but this +finding does not require the broader metadata-copy redesign deferred in +[future experiments](future.md#3-performance-experiments), weaker detachment, a +smaller limit, or removal of historical inspection. + +## 3. An Imported Version-Check Panic Leaves The Host Root Open + +Priority: medium. Category: file-descriptor cleanup during unwinding. + +**Relevant code.** `acquireImportedInput` / `acquireImportedWithAccess` and imported +version processing in +[import_acquisition.go](../../internal/engine/import_acquisition.go), together with +`openHostAccess` / `filesystemAccess.close` in +[filesystem_access.go](../../internal/engine/filesystem_access.go). Compare the +deferred fallback cleanup in +[selected_overlay.go](../../internal/engine/selected_overlay.go) and deferred file +cleanup in [source_file.go](../../internal/engine/source_file.go). + +**Circumstances.** Host import acquisition opens an `os.Root`, then closes it +explicitly on ordinary failure paths and after import traversal. It has no +deferred fallback closure. A user version-check callback runs for imported +sources while that root is open. If it panics and the application recovers outside +`Load`, unwinding bypasses all those explicit root closes. The imported file's +own deferred close still runs; the outstanding resource is the directory/root +handle. + +**Evidence.** A standalone Linux diagnostic created a temporary main file and one +import, both with a version, enabled source-version checks, and panicked only on +the imported source's check. With automatic GC disabled only in that disposable +diagnostic process, the number of `/proc/self/fd` entries targeting the temporary +root went from zero to one after external recovery. The non-panicking control +ran both checks and returned to zero. One outstanding root was sufficient to +demonstrate the issue; no descriptor-exhaustion workload was run. + +**Consequence.** Library-owned root cleanup depends on eventual runtime cleanup +instead of load-scope unwinding. Repeated recovered callback failures can retain +descriptors long enough to exhaust them. This does not imply that ConfigKit must +recover application panics or successfully return a configuration after one. + +**Potential remediation.** Install a deferred fallback close at the owning +acquisition boundary, while preserving normal explicit close timing and close +error diagnostics, as the selected-overlay path already does. Avoid double-close +confusion and leave borrowed native filesystems untouched. Preserve panic +propagation. This is separate from +[correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding): +deactivating a callback context alone does not close the root, and closing the +root alone does not deactivate that context. + +## Validation And Boundaries + +The following passed with Go 1.26.10 on Linux amd64, using cached dependencies +with automatic toolchain/network acquisition disabled: + +```sh +go test ./internal/engine -run 'Closes|Close|Budget|Limits|Limit|SecretsDir' -count=1 +go test ./... -count=1 +``` + +The three standalone diagnostics described above were run outside the repository; +they added no production code or repository tests. The history diagnostic only +read private fields to count pointers; it did not mutate inspection state. No +native macOS or Windows checks were performed. + +No additional network, timer, goroutine, buffer-pool, or ordinary-return handle +leak was established. Borrowed blocking readers/callbacks do not create a new +cancellation guarantee. Previously reported recursive-schema stack exhaustion +and callback-state retention remain owned by the +[correctness audit](correctness.md), rather than duplicated here. Existing +algorithmic and maintainability findings remain owned by their respective +audits. This report adds no API, release gate, new resource quota, or background +management requirement.