From 8fe8b67b29bbd96e321a09d797b3df5a21cfd0bf Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 7 Nov 2026 12:58:48 +0000 Subject: [PATCH] Record the concurrency audit --- docs/roadmap/audit/04-concurrency.md | 178 +++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 docs/roadmap/audit/04-concurrency.md diff --git a/docs/roadmap/audit/04-concurrency.md b/docs/roadmap/audit/04-concurrency.md new file mode 100644 index 0000000..82c493f --- /dev/null +++ b/docs/roadmap/audit/04-concurrency.md @@ -0,0 +1,178 @@ +# Concurrency audit + +Status: three relevant P2 findings: one new cancellation defect and two +reconfirmed findings from the functional correctness audit. Reviewed source: +`2ea26afcc5ae691152e80d2fa5ee4d572d390a95`. + +## Architecture, contracts, and scope + +The [internal overview](../../internal/overview.md) inventories the implemented +owners. Core captures declarations and routing metadata, builds fresh backend +parse state per invocation, prepares typed inputs, and coordinates invocation +reporting and outcomes. ckconfig owns configuration scope/comparison coordination; +ckprompt owns PromptKit request/inspection coordination and sequential aggregate +smoke tests. Applications own loaders, builders, runtime dependencies, contexts, +and executable lifecycle. Upstream libraries own their domain operations. + +The [reporting contract](../../consumers/guide.md#invocation-reporting) and +[Reporter GoDoc](../../../reporter.go) explicitly permit concurrent operations on +one invocation's reporter. One reporting-state mutex protects visibility, +deferred messages, destination observations, and writes. Managed output writes +acquire it; rendering callbacks use a coordinated writer while the rendering +operation holds it. Rendering callbacks must not reenter that reporter, and +their supplied writer is valid only while the callback runs. Concurrent message +submission order is unspecified. Reporter handles close before Execute returns; +caller-owned writers are not closed. + +Concurrent Execute/Register access to one App is not established by the current +contracts; [concurrent dispatch remains separate future work](../future.md#other-future-proposals). +The audit did not infer that guarantee from independent invocation state or +declaration reuse. Application callback objects and factory results retain their +own ownership and synchronization responsibilities under the +[architecture policy](../../policy/architecture.md#handler-inputs-and-invocation-state). + +Static review covered locks and state access, reporter acceptance/finalization, +callback and writer lifetimes, declaration/snapshot ownership, integration input +and shared-state lifetimes, cancellation checkpoints, comparison/aggregate +sequencing, and maintained examples and release launchers. CommandKit's runtime +code starts no goroutines and owns no channels or worker pools. Integration +callbacks run synchronously; aggregate scopes, comparison sides, and smoke kinds +are coordinated sequentially. Release launchers use exec to delegate to ReleaseKit. +Application/upstream workers and subprocess internals remain with their owners. + +## Validation evidence + +Codebase-memory-mcp symbol/source searches and call tracing were supplemented +with repository-wide searches for synchronization, goroutines, channels, and +cancellation. The indexed Go source matches this revision. Validation used Go +1.27.0, cached dependencies, and no live services or credentials: + +- The aligned current-source workspace suite passed with race detection and + fresh test execution: + `GOTOOLCHAIN=local go test -race -count=1 -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`. + This includes all three modules and their maintained examples. The optional + interactive shell checks remain opt-in and were not enabled in this pass. +- A temporary external consumer run with `go run -race` reproduced N1 through + the public ckprompt/core APIs. It canceled the context from the progress + destination's writer without reentering the reporter. +- Existing temporary external probes using local replacements for all three + CommandKit modules reproduced N2 and N3 in three consecutive runs each with + `go test -race -count=3`. Their functional assertions failed as described + below. No data-race report was emitted. + +No production files or repository tests were changed. These failures concern +ordering and cancellation accounting; memory-race detection alone does not +establish that an accepted queue item is delivered or a stopped aggregate fails. + +## N1 — P2: Cancellation can stop aggregate smoke testing with a success outcome + +**Code:** [smoke_command.go](../../../integrations/ckprompt/smoke_command.go), +runSmokeAggregate lines 159–176 and final outcome selection at 183–187; +queuePromptDiagnostics in [command.go](../../../integrations/ckprompt/command.go), +lines 586–603; core's Progress delivery in [reporter.go](../../../reporter.go). + +**Circumstances:** a smoke callback returns a report without error. The first +post-callback context check sees no cancellation. Cancellation becomes visible +while diagnostics are presented, or asynchronously before the later checkpoint +at line 175. That checkpoint stops the loop, but does not retain its error. +Progress diagnostics are delivered immediately, so this window can be exercised +deterministically with a valid progress writer that cancels the context. + +**Observed behavior:** a temporary consumer invoked +`promptkit test all --progress --format=json`. Its first callback returned a +completed reachability report and a progress diagnostic. The diagnostic writer +canceled the context and accepted all bytes. Only one callback ran; the JSON +report contained one successful entry with no error. Execute nevertheless +returned Kind success, status 0, and nil Cause. Rendering and writing errors +were nil. The other three requested kinds were never attempted. +A second probe returned a provider error from that callback: status was 1, but +errors.Is(outcome.Cause, context.Canceled) was still false after the same stop. + +**Expected behavior and consequences:** the +[aggregate smoke contract](../../cli.md#promptkit-smoke-testing) requires all four +callbacks to return without error for status 0 and preserves cancellation causes +when cancellation stops the sequence. A partial aggregate stopped by cancellation +must not report successful completion. Automation can otherwise accept an +incomplete aggregate, and callers cannot detect the cancellation through +errors.Is on the returned cause. If an operation error already exists, the same +window can omit cancellation from its joined causes. + +**Potential remediation:** when the later checkpoint uses cancellation to omit +remaining kinds, retain that error in the aggregate cause before leaving the +loop. Deduplicate an already retained cancellation as appropriate. Preserve +completed reports, prior operation errors/explanations, and attempted-kind order; +do not invent a report for an unattempted kind. Moving only the earlier check +leaves another interval in which cancellation can be observed and discarded. +This does not require treating every cancellation after completed report delivery +as a new failure. Coverage should distinguish cancellation in the provider, +during immediate progress presentation, and between aggregate operations. + +## N2 — P2: Deferred-message acceptance remains open after the final batch detaches + +This is the same issue as +[C2 in the correctness audit](01-correctness.md#c2--p2-finalization-can-discard-a-successfully-queued-deferred-message), +reconfirmed here rather than a separate remediation item. + +**Code:** [reporter.go](../../../reporter.go), DeferredReporter.enqueue lines +254–279 and Reporter.finalize lines 282–325. + +**Circumstances:** finalization detaches state.deferred and unlocks between +deliveries. Another goroutine can acquire the mutex during that flush, append a +visible message to the now-empty queue, and receive nil from its submission. +The reporter has not yet set closed. The finalizer drains only the detached +batch, then clears the new queue when closing. + +**Consequences and evidence:** the accepted message silently disappears, while +Execute can return success and mark diagnostic delivery complete. The probe +used one invocation, a briefly pausing diagnostic destination, and a goroutine +submitting AtEnd().Warn during the flush. All three race-enabled runs accepted +and lost the message. There was no unsynchronized memory access or callback +reentry; the defect is the queue's acceptance/finalization ordering. + +**Potential remediation:** atomically close deferred acceptance when detaching +the final batch, rejecting later submissions explicitly, or drain every accepted +item before closure. Keep finalizer-owned delivery possible after acceptance +closes; setting the reporter's existing closed flag too early would also reject +its internal delivery. Preserve message visibility, ordering, and error evidence. +Goroutines created by an application remain application-owned; this finding +concerns an operation that already returned successful acceptance. + +## N3 — P2: Buffered configuration-show cancellation becomes a presentation failure + +This is the same issue as +[C4 in the correctness audit](01-correctness.md#c4--p2-show-preparation-labels-ordinary-cancellation-as-a-rendering-error), +reconfirmed here rather than a separate remediation item. + +**Code:** [show.go](../../../integrations/ckconfig/show.go), runConfigShow lines +69–95, interacting with core Reporter.RenderOutput in +[reporter.go](../../../reporter.go). + +**Circumstances:** cancellation becomes visible between buffered effective +document conversions. The checkpoint inside RenderOutput returns ctx.Err as +the rendering callback's error. Core records it as Output.RenderError before +ckconfig returns cancellation as the operation cause. + +**Consequences and evidence:** normal cancellation is reported as both an +operation failure and an incomplete output/rendering failure. A two-scope probe +acquired both views; the first native writer emitted valid safe YAML, canceled +the context, and returned nil. The second writer was skipped and no report +bytes were delivered. All three race-enabled runs retained context.Canceled as +Cause but also incorrectly recorded it as Output.RenderError, with no writer +error. This can mislead callers distinguishing interrupted preparation from +encoder or output failures. + +**Potential remediation:** retain preparation cancellation separately from the +rendering callback's error, following renderPreparedConfigReport in +[operations.go](../../../integrations/ckconfig/operations.go). Preserve real +native-writing, conversion, and destination errors independently, and keep an +incomplete aggregate report from being delivered. Retain the selected distinction +between cancellation before delivery and cancellation after delivery has started. + +## Assessment + +N1 is new. N2 and N3 remain open in the correctness audit and should be tracked +as the same underlying issues. No data race, supported-path deadlock, +goroutine leak, or channel-lifecycle defect was established in the reviewed +CommandKit code. The passing race suite supports the exercised synchronization +paths; the three findings demonstrate why lifecycle and cancellation reasoning +are also necessary.