load(PR1): persist the interesting corpus as a campaign ConfigMap (DD-026)

[ianp94]

Producer half of load/soak mode (DD-026). An explore run's interesting inputs (the replay corpus) are now persisted so they can be replayed under load (PR 2), rendered on the dashboard (the corpus-view you asked for), and re-run for reproducibility.

Transport: reuse the termination-message channel, not a sidecar

DD-026 §3 flagged the sidecar + a new driver-pod identity as the costly path. Instead — using the reviewer's own "reuse the termination-message" option + the top-N cap — the driver splices a byte-capped (~3 KB, ≈top-N) replayCorpus array into the summary it already writes to /dev/termination-log; the operator (which now holds configmaps: create;update) reads it back and materializes a campaign-owned <campaign>-corpus-out ConfigMap, setting status.corpusConfigMap. The driver stays credential-less — no new pod SA. The cap is also the right load-replay semantics (hammer the best states); a bigger transport (sidecar/PVC) is a documented future extension.

Changes

• runner (CoverageGuidedRun): publishes its live corpus; writeSummary splices in replayCorpus; replayCorpusJson (dedup + JSON-escape + byte budget) with unit tests.
• operator: driverSummary.ReplayCorpus; emitCorpusConfigMap (create-or-update, owner-ref'd); status.corpusConfigMap; configmaps create;update;patch RBAC (regenerated role + synced the chart's CRD + rbac.yaml). envtest spec asserts the emitted ConfigMap (content + owner ref).
• e2e: a completed campaign emits status.corpusConfigMap with route entries, owned by the campaign.

Scope

No load behavior yet (that's PR 2); every completed explore campaign emits its corpus. Emission is non-fatal — a ConfigMap write failure logs but doesn't fail the (already Completed) run.

Validation

runner ReplayCorpusJsonTest green; operator envtest green (adds the emission spec). In-cluster e2e running (adds 3 corpus-emission checks).

@claude please review — focus on: (1) the termination-message splice (replayCorpusJson byte cap vs the ~4 KiB limit including the metrics JSON — is 3 KB safe headroom? malformed-JSON risk from the string splice?); (2) emitCorpusConfigMap create-or-update + owner ref + the non-fatal error handling; (3) the lastCorpus static publish (thread-safety at shutdown-hook time); (4) RBAC scope (configmaps create;update;patch — is patch needed?).

🤖 Generated with Claude Code

[ianp94]

@claude please review this load-mode PR 1 (corpus producer). Focus: (1) the termination-message splice — replayCorpusJson byte cap (3 KB) vs the ~4 KiB termination-message limit INCLUDING the metrics JSON (safe headroom?), and malformed-JSON risk from the string splice into snapshotJson; (2) emitCorpusConfigMap create-or-update + owner ref + non-fatal error handling; (3) the lastCorpus static publish + shutdown-hook thread-safety; (4) RBAC scope (configmaps create;update;patch). runner + envtest green; in-cluster e2e running.

[claude]

Claude finished @ianp94's task in 4m 59s —— View job

---

Review: load(PR1) — corpus producer (DD-026)

Todo list

• Read repo structure and relevant design doc (LOAD-MODE-DESIGN.md)
• Review replayCorpusJson splice (byte cap vs termination-message limit, malformed-JSON risk)
• Review emitCorpusConfigMap (create-or-update, owner ref, non-fatal error handling)
• Review lastCorpus static publish + shutdown-hook thread-safety
• Review RBAC scope (configmaps create;update;patch)
• Post final review

No repo-root CLAUDE.md was found, so no repo-specific review conventions to reconcile against. I did not re-run the Java/Go test suites (Bash tool required approval that wasn't available in this session); I traced the logic statically against ReplayCorpusJsonTest.java and the envtest spec instead.

---

1. Termination-message splice (CoverageGuidedRun.java:242-299)

Byte-cap headroom — REPLAY_CORPUS_MAX_BYTES defaults to 3000. StatusReporter.snapshotJson()'s format string (util/StatusReporter.java:222-236) is roughly 350-400 literal bytes plus digits/floats for ~19 numeric fields — realistically 450-650 bytes even for long soak runs. So 3000 + ~500 + the ,"replayCorpus": splice overhead lands around 3550-3700 bytes, leaving only ~400-500 bytes of headroom under the kubelet's hard 4096-byte termination-message cap (kubecontainer.MaxContainerTerminationMessageLength). That's workable today, but the 3000 constant is fixed and independent of the actual metrics size — nothing ties the two together. If snapshotJson() ever grows a field, the combined write can silently exceed 4096, and kubelet truncation would produce a JSON payload that fails to Unmarshal in readDriverSummary (Go), silently losing both the corpus and the coverage/findings status for that run (caught, but silent). Recommend either computing the corpus budget as 4096 - snap.length() - margin at write time, or adding a test that exercises writeSummary end-to-end (real snapshotJson() + a full corpus) and asserts the merged size stays under 4096 — ReplayCorpusJsonTest only exercises replayCorpusJson in isolation, not the combined splice.

Malformed-JSON risk from the string splice (writeSummary, line ~266) — the splice logic (snap.substring(0, snap.length()-1) + ",\"replayCorpus\":" + corpusJson + "}") is safe given snap is always a well-formed object ending in } (guaranteed by snapshotJson's fixed format string) and corpusJson is always a well-formed [...] array (guaranteed by replayCorpusJson's own construction, tested by ReplayCorpusJsonTest). No injection risk from corpus content either — jsonString() escapes ", \, and control chars correctly (CoverageGuidedRun.java:301-321).

One real gap in jsonString/replayCorpusJson's size accounting: enc.length() (CoverageGuidedRun.java:287) counts Java UTF-16 chars, not UTF-8 bytes. For today's JPetStore routes (all-ASCII) this is a distinction without a difference, but the k8s termination-message cap is a true byte limit — if a future grammar or mutated input ever contains multi-byte UTF-8 (e.g. non-ASCII query values), the actual on-disk byte count written by Files.write(..., merged.getBytes(UTF_8)) could exceed the computed "byte" budget without replayCorpusJson knowing it. Low risk given current seed/grammar data, but worth a enc.getBytes(StandardCharsets.UTF_8).length fix if this is meant to be a general-purpose cap (as the design doc implies for a broader replay-corpus notion).

2. emitCorpusConfigMap create-or-update + owner ref + non-fatal handling (closurejvmcampaign_controller.go:263-290, 203-217)

The create-or-update + owner-ref logic itself looks correct: Get → IsNotFound → Create, else copy Data/OwnerReferences onto the fetched object and Update (preserves ResourceVersion, avoids conflicts). The envtest spec (closurejvmcampaign_controller_test.go:223-260) covers this path, including the owner reference assertion.

The more substantive issue is when this runs: emitCorpusConfigMap is only ever called during the single reconcile that flips campaign.Status.Phase to CampaignCompleted (line 204, before the emit call at line 213). The reconciler's top-of-function guard (lines 78-81) short-circuits all future reconciles once the phase is terminal. So "non-fatal" here really means "non-fatal and unrecoverable" — a transient API-server error on that one attempt (the kind l.Error(...) logs at line 214) permanently forfeits status.corpusConfigMap for that campaign, with no retry path, since the campaign never re-enters this code again. Contrast with ensureDashboard, which runs on every non-terminal reconcile and therefore self-heals on retry. If corpus emission is meant to be best-effort-but-eventually-consistent, consider requeuing once (stay Running for one more reconcile) when emitCorpusConfigMap fails, rather than completing regardless.

3. lastCorpus static publish + shutdown-hook thread-safety (CoverageGuidedRun.java:121-123, 144, 180, 218, 242-243, 260-299)

This is the one I'd block on. lastCorpus is volatile, which only guarantees visibility of the reference — it does nothing for the ArrayList it points to, which is mutated by the main loop (corpus.add(...) at lines 180 and 218) with no synchronization.

The shutdown hook (registered at line 121-122) is not guaranteed to run only after the main thread has returned. JVM shutdown hooks run concurrently with any still-live non-daemon threads once shutdown begins — and shutdown can begin from an external SIGTERM (e.g. kubectl delete pod, or a rolling eviction, before the campaign's own time-box deadline is hit), not just from main() returning normally. In that scenario, the hook thread calls writeSummary → replayCorpusJson(lastCorpus, ...) → new LinkedHashSet<>(corpus) (line 282, a full iteration) while the main thread may still be inside the for-loop calling corpus.add(input). ArrayList gives no thread-safety guarantee for concurrent read+write; this can throw ConcurrentModificationException or, more subtly, observe an array reference mid-resize.

Compounding this: writeSummary's try { ... } catch (Exception ignored) (lines 261-272) wraps both StatusReporter.snapshotJson() and replayCorpusJson(lastCorpus, ...) in one block. A race-triggered exception from the corpus read aborts the entire summary write — so the SIGTERM race doesn't just risk losing the new replay-corpus feature, it risks losing the pre-existing DD-025 metrics summary too, which the campaign controller depends on for status.coveragePct/status.findings.

Suggested fix: guard both the add() call sites and the corpus snapshot in replayCorpusJson/writeSummary with the same lock (e.g. synchronized (corpus) { ... } around each mutation and around the defensive copy taken before iterating), and split the try/catch so a corpus-specific failure can't take down the metrics snapshot with it.

Fix this →

4. RBAC scope — configmaps create;update;patch (operator/config/rbac/role.yaml, deploy/helm/closurejvm-operator/templates/rbac.yaml)

patch does not appear to be needed for this PR. emitCorpusConfigMap only ever calls r.Get → r.Create (not-found path) or r.Update (existing path) — no r.Patch call anywhere in the diff. controller-runtime's typed Update issues a full-resource PUT, not a PATCH. Unless there's a near-term plan to move to server-side-apply (which does use PATCH), I'd drop patch from both role.yaml and rbac.yaml to keep the grant matched to what the code actually exercises — create/update alongside the pre-existing get;list;watch is sufficient.

Fix this →

---

Other notes

• ReplayCorpusJsonTest (dedup, escaping, byte-cap truncation) is well-targeted and passes the cases that matter for the pure function — the gaps above are about what's not yet covered (combined-size assertion, concurrent access).
• The Helm chart's RBAC is a namespaced Role and the kubebuilder-generated one is a ClusterRole for local/dev tooling — scope itself (namespace-bound in the chart) looks right, this is purely about the verb list.
• docs/LOAD-MODE-DESIGN.md and both CRD copies (operator/config/crd/bases/... and deploy/helm/.../crds/...) are in sync, and status.corpusConfigMap is properly +optional.

Summary

Solid, well-scoped PR1 — the credential-less transport choice and top-N cap are sound, and the byte-cap/dedup/escaping logic is unit-tested. The one issue I'd want addressed before merge is #3: the unsynchronized corpus list read by a shutdown hook that can run concurrently with the main loop on an external SIGTERM, which (via the broad catch in writeSummary) can silently drop the entire end-of-run summary, not just the new corpus field. #1's headroom is fine today but not test-enforced against snapshotJson's actual size. #2 and #4 are smaller: a non-retryable emission failure and an RBAC verb (patch) that isn't exercised by the current code.

[ianp94]

Great review — the shutdown-race catch (and that it could take down the metrics summary too, not just the new field) was the important one. All four fixed in `45deb1e`:

#3 (race, was blocking): synchronized both corpus.add() sites and take a defensive copy under the same monitor in writeSummary, so the shutdown hook can't CME against the main loop on an external SIGTERM. Also split the try/catch — snapshotJson() and the corpus build are now independent, so a corpus-side failure falls through to "[]" and the DD-025 metrics still get written.

#1 (byte budget): the corpus budget is now computed as min(cap, TERMINATION_MSG_BUDGET − actualMetricsBytes − overhead) at write time, so it's tied to the real snapshotJson() size — a future metrics field can't silently push the merged write past 4096. Switched the accounting to UTF-8 bytes (getBytes(UTF_8).length), not UTF-16 chars. Added combinedSummaryStaysUnderTheTerminationCap — writes a 500-entry corpus through the real snapshotJson() and asserts the file is < 4096 and still one well-formed object carrying both exploration and replayCorpus.

#2 (non-retryable): emit now runs before the terminal-phase flip; on failure the campaign stays Running and requeues (the create-or-update is idempotent), so a transient API error retries instead of permanently forfeiting status.corpusConfigMap.

#4 (RBAC): dropped patch from the configmaps grant in both role.yaml and the chart — emitCorpusConfigMap only Creates/Updates.

runner + envtest green; the producer happy path was already validated in-cluster (emitted a 24-route corpus ConfigMap) and these are edge-hardenings that don't touch it. Merging.