feat(load): <nonce> generator for non-idempotent write replay + Location-classified 3xx counters (DD-038)

[basquin-bot]

DD-038 — <nonce> generator + Location-classified 3xx counters

Two defects surfaced while validating DD-037's correlated JSPWiki writes end-to-end. Both made a failing write path look like a working one.

1. A fixed-corpus write replay is idempotent, so it stops being a write. JSPWiki's DefaultPageManager.saveText returns before writing when oldText.equals(proposedText) — and still 302s "success". So after the first replay the page never changes and the save costs nothing, while the load driver counts full-price 2xx/3xx traffic. This is a general CMS pattern, not a JSPWiki quirk.

2. A rejected write was invisible. fire auto-followed redirects, so a 302 → /Wiki.jsp?page=SessionExpired (or PageModified) was counted as an ordinary request: not 4xx, not 5xx, and capture succeeded so not a captureMiss. A run could reject 100% of its saves and report clean.

What this adds

<nonce> is a grammar generator primitive — same <…> namespace as <int>/<string>, so the author names their own rule and it reserves nothing in the ${{}} correlation namespace:

$rev = <nonce>
POST /Edit.jsp page=${page}&…&_editedtext=${wikitext} ${rev}&ok=Save

It expands to the fire-time marker ${{@nonce}} (@ can't be a Capture name → collision-proof), which LoadRun.substitute fills per fire with RUN_SALT-counter, RUN_SALT = millis + pid. Note a bare millis-seeded AtomicLong does not work: the seed advances in wall-clock ms while the counter advances by number-of-saves, so a later run's seed lands inside a prior run's emitted range and re-emits still-saved values — reintroducing the no-op. Salt + counter is uniqueness by construction, within a run and across parallel JVMs.

Substitution is also widened from body-only to the full request line (path+body), null-safe — generators are usually used in a URL query.

Load mode stops following redirects. fireR returns FireResult(code, location) (the int fire overloads stay as test-facing wrappers) and a pure-static normalizeLocation classifies the target into bounded redirects / redirectTargets counters in the terminal summary. Self-redirects fold to one "self" key so JSPWiki's success 302s can't evict the real rejects from the bounded map; keys are charset-restricted and the map hard-bounded, because the operator drops the whole summary on invalid/oversized JSON. Explore mode still follows redirects — it wants coverage, not redirect metrics.

No-follow is also a session-carry fix, not a regression: with follow on, a 302 was re-issued as a cookieless GET whose anonymous Set-Cookie overwrote the jar's valid JSESSIONID. captureSessionCookie runs on the direct 3xx before any follow, so the real session is kept.

⚠️ Benchmark baselines

For any corpus whose steps redirect (JSPWiki edit_save, jpetstore Stripes POSTs), the driver no longer pays the follow hop — post-DD-038 p50/p90/p99 and throughputRps are not comparable to earlier numbers. Re-baseline before comparing.

Verification

215 tests green (211 before). Nonce uniqueness + never-masks-an-unbound-${{ref}}; a path-only marker substituting without NPE; normalizeLocation self-fold (composed from a body-leading page=, as the worker does), frompage= rejection, absolute URLs, 64-char truncation; the admitKey overflow cap (including: a key already present in a full map is still admitted, so counts don't fragment); fireR location on 302, null on 200, and a 302-carrying-Set-Cookie still populating the jar; summaryJson valid + bounded JSON with a populated redirectTargets. Grammar validation shows the marker survives expansion and fills uniquely per fire.

Backward compatible: a v1/v2 corpus line with no ${{/no capture parses, formats, and fires byte-for-byte as before. The DD-036 invariant is intact — findings are still labeled with the raw recipe step, never a substituted body.

Process

Spec and plan were fable-reviewed before any code (16 issues caught pre-implementation, incl. the nonce cross-run collision). Built via SDD — fresh implementer + reviewer per task — then a whole-branch Opus review: 8/8 binding checklist items PASS, no Criticals; its 4 Important + 4 cheap Minor findings are fixed in b2d4abf.

Docs: DD-038 in docs/DESIGN-DECISIONS.md, docs/LOAD-MODE-DESIGN.md §12, runner/CHANGELOG.md.

@claude please review.

[basquin-bot]

@claude please review this PR.

Focus areas:

1. Nonce uniqueness under concurrency. LoadRun.RUN_SALT (millis + pid) plus a shared AtomicLong must not collide within a run, across sequential runs of the same corpus, or across parallel JVMs. The whole feature exists because JSPWiki's saveText no-ops an identical body — a repeated token silently reintroduces that.
2. The DD-036 token-leak invariant. A captured CSRF/session token must never reach disk. Findings and crash records must still be labeled with the raw recipe step, never a substituted line — check both CoverageGuidedRun.runSequence and the single-step request(base, String).
3. The summary must stay parseable. The operator parses the terminal JSON with json.Unmarshal and drops the entire summary if it's malformed or oversized. redirectTargets keys come from a fuzzed Location header — verify safeKey + the admitKey cap make that impossible to break.
4. Null-safety of the widened substitution. Path is never null, body can be. A path-only ${{@nonce}} must substitute and fire, not NPE and not skip; an unbound ${{csrf}} must still skip and count a captureMiss. There are three fire sites — they should be identical.
5. Backward compatibility. A v1/v2 corpus line with no ${{ and no capture suffix should parse, format, and fire byte-for-byte as before.

Please flag anything that would make a broken run look like a clean one — that's the failure mode this whole change is about.

[claude]

Claude finished @basquin-bot[bot]'s task in 4m 7s —— View job

---

Review: DD-038 <nonce> generator + Location-classified 3xx counters

• Gather context (diff, changed files)
• Review nonce uniqueness under concurrency (RUN_SALT + AtomicLong)
• Review DD-036 token-leak invariant (raw recipe step in findings/crash records)
• Review summary JSON parseability (safeKey + admitKey cap)
• Review null-safety of widened substitution (3 fire sites)
• Review backward compatibility (v1/v2 corpus lines)
• Post final review

Went through LoadRun.java, CoverageGuidedRun.java, RequestLine.java, RequestGrammar.java, jspwiki.grammar, and all 5 test files against origin/main. Note: I could not execute the Gradle test suite in this sandboxed review environment (shell commands here require interactive approval I don't have), so the below is verified by reading, not by re-running 215 tests green.

1. Nonce uniqueness — solid within a run and across sequential runs, weaker across parallel JVMs than claimed

NONCE (AtomicLong) is a single static field shared by every worker thread and by all three call sites (LoadRun's worker loop, CoverageGuidedRun.runSequence, CoverageGuidedRun.request), so within one process every getAndIncrement() is distinct — good.

RUN_SALT = currentTimeMillis() + "x" + ProcessHandle.current().pid() is meant to cover the cross-process case, and the doc's stated justification is "two driver JVMs starting in the same millisecond still differ [because pid differs]" (docs/DESIGN-DECISIONS.md DD-038, LoadRun.java:511-515). That holds on a bare host, but this project is explicitly Kubernetes-native (docs/LOAD-MODE-DESIGN.md: "driver Job", "driver pod"). Under a default Linux PID namespace, the main process of every container starts renumbered from a small integer — very often PID 1 — independent of the host's real PID. Two driver Jobs/pods launched close together (a parallel Job with parallelism>1, or two campaigns kicked off by the same controller/cron tick) can easily collide on both the pid and the millisecond component, especially since Kubernetes scheduling tends to batch-start pods.

If that happens, both processes emit an identical RUN_SALT-counter stream from counter 0 — reintroducing the exact oldText.equals(proposedText) no-op DD-038 exists to prevent, but now across campaigns instead of within one. This only bites if two driver pods hit the same target concurrently with overlapping counter values, so it's a narrower window than the original bug, but it's exactly the "broken run looks clean" failure mode the reviewer asked to flag, and the current doc's reasoning doesn't hold in the deployment model this repo targets.

Suggest folding in something host/pod-unique instead of (or in addition to) pid — System.getenv("HOSTNAME") is already used elsewhere in this codebase as the pod identity (DD-013: "in Kubernetes, HOSTNAME is the pod's name by default"), which would make the salt collision-resistant across pods, not just across same-host processes.

Fix this →

2. DD-036 token-leak invariant — verified correct

All three fire sites keep the recorded label raw:

• CoverageGuidedRun.runSequence (CoverageGuidedRun.java:518) passes the original step string as label, even though it fires the substituted r.
• CoverageGuidedRun.request(base, String) (CoverageGuidedRun.java:574) passes the original step, not the substituted r.
• The private request(base, r, bindings, label) only ever uses label for X-Basquin-Invariant-* saves and the 5xx exception message (CoverageGuidedRun.java:633, 673) — never for firing.

LoadRun's worker loop doesn't persist findings from load mode at all, so there's no equivalent leak surface there. Good — matches the invariant exactly as before this PR.

3. Summary JSON parseability — correct, one low-severity edge case

safeKey restricts to [A-Za-z0-9._-] (LoadRun.java:502-507), so a fuzzed Location can't inject a quote/control char into the JSON — confirmed by LoadDriftUnavailableTest.redirectTargetsSerializeAsValidJson, which explicitly fuzzes a"b<c through normalizeLocation and checks for balanced quotes/braces. admitKey's size() < CAP check has a documented, intentional benign race (multiple threads can pass the check for distinct new keys before any commits land), but the terminal JSON always takes .limit(REDIRECT_TARGETS_CAP) off the map regardless of any live-map overshoot (LoadRun.java:232-237), so the emitted summary is bounded to 12 keys no matter what — the parseability guarantee holds even in the race's worst case.

Minor: "self" and "other" are reserved bucket names (self-redirect fold, overflow) that aren't distinguished from a genuine page/route literally named self or other — if an app under test happens to have such a route, its counts would silently merge into the reserved bucket. Very low real-world likelihood, not worth blocking on, but worth a one-line doc note if it's ever hit in practice.

4. Null-safety of widened substitution — verified identical across all 3 sites

LoadRun.java:138-144 (worker loop), CoverageGuidedRun.java:500-508 (runSequence), and CoverageGuidedRun.java:566-569 (request) all use the same shape:

String p = substitute(step.path(), bindings);           // path never null
String b = (step.body() == null) ? null : substitute(step.body(), bindings);  // body guarded
if (p == null || (step.body() != null && b == null)) { ...skip... }

RequestLineV3Test.needsSubstitutionDetectsAMarkerInThePath and LoadCorrelationTest.pathOnlyNonceSubstitutesEndToEndWithoutNpe exercise the path-only/null-body case end-to-end through a real HttpServer, confirming no NPE and that the literal marker never reaches the wire. LoadCorrelationTest.nonceDoesNotMaskAnUnboundRealRef confirms ${{@nonce}} filling doesn't hide a genuinely unbound ${{csrf}} in the same string (substitute still returns null). All correct.

5. Backward compatibility — verified

RequestLine.needsSubstitution() only trips on "${{" in path or body; a v1/v2 line with neither never enters the substitution branch at any of the 3 sites and takes the pre-existing code path unchanged. The only production-code diff in RequestLine.java is the one-line widening to also check path (git diff confirms this is the entire change to that file).

Other observations

• fire(base, step, jar) / fire(base, step, jar, bindings) are correctly reduced to test-facing wrappers around fireR; no production caller was missed (confirmed via grep — the worker loop calls fireR directly).
• The PR's own DD-038 record is honest about a residual gap: the worker's full extract-page → classify → admit → increment path is only unit-tested in its parts, not end-to-end through a live redirect under load. Not a blocker, just noting it's disclosed rather than hidden.
• normalizeLocation's (^|[?&])page= handling (PAGE_PARAM, precompiled per the review-commit fix) correctly checks both path and body for the firing request's own page= (LoadRun.java:172-173), which is what makes the self-fold work for JSPWiki's body-leading page=Main&action=save shape — good catch already applied in b2d4abf.

Summary

The core mechanism (nonce generation, full-line null-safe substitution, no-follow + Location classification, bounded/charset-safe summary) is correctly implemented and well-tested for the scenarios in scope. The one substantive finding is the RUN_SALT pid-collision risk under Kubernetes' per-container PID namespaces (#1 above) — worth addressing given this is exactly the deployment model the driver targets and exactly the failure mode ("broken run looks clean") this PR is about. Everything else is low-severity or already handled well.
· Branch: feat/nonce-payload-dd038

[basquin-bot]

Thanks @claude — finding #1 is real and I've fixed it in d52ff7e.

You're right that the record's justification didn't hold in this repo's deployment model. The driver runs as a Kubernetes Job, so inside the container's own PID namespace the main process is renumbered from a small integer (commonly 1) — millis+pid collides outright for two pods started in the same millisecond, and both then emit an identical <salt>-<counter> stream. That revives the saveText() no-op across campaigns while the run still looks clean, which is exactly the failure class this PR exists to close.

RUN_SALT is now millis + pod HOSTNAME + pid (DD-013 already establishes HOSTNAME as the pod identity; runner/util/DashboardClient.java:53 uses it the same way). pid stays the fallback for a local run where HOSTNAME is unset. The construction is extracted as a pure, package-private buildRunSalt(hostname, millis, pid) so the collision-resistance is testable without spawning pods, and the hostname goes through safeKey so the salt stays URL-safe when spliced verbatim into an encoded body.

New test LoadCorrelationTest.runSaltSeparatesTwoPodsThatCollideOnMillisAndPid pins: two pods sharing millis and namespaced PID 1 still get distinct salts; a blank/unset HOSTNAME falls back to pid rather than an empty slot; and an exotic hostname can't introduce an unsafe character. The DD-038 record's rationale is rewritten to state the PID-namespace reason rather than the (wrong) "different pid" one.

Your minor on the reserved "self"/"other" bucket names is now recorded as an accepted caveat in the DD-038 record — a target route literally named self/other merges into the bucket. These are operator-facing counters, not an oracle, so I don't think it's worth disambiguating.

Confirming the two non-findings you verified by reading: the suite is green at 216 tests locally (./gradlew test), and CI is green on this branch.

[claude]

Claude finished @basquin-bot[bot]'s task in 1m 31s —— View job

---

Verified: d52ff7e correctly fixes finding #1 (RUN_SALT pod-collision risk)

• Gather context (diff of d52ff7e)
• Review buildRunSalt implementation in LoadRun.java
• Review new test runSaltSeparatesTwoPodsThatCollideOnMillisAndPid
• Review DD-038 doc rationale rewrite + self/other caveat
• Post final verdict

I could not execute ./gradlew test in this sandboxed environment (shell commands here require an interactive approval I don't have), so this is verified by reading the diff, not by re-running the suite.

buildRunSalt (runner/coverage/LoadRun.java:518-525) — matches what the commit message claims:

static String buildRunSalt(String hostname, long millis, long pid) {
    String node = (hostname == null || hostname.isBlank()) ? Long.toString(pid) : safeKey(hostname);
    return Long.toString(millis) + "x" + node + "x" + Long.toString(pid);
}

• Pure and package-private, as claimed — no static state read, deterministic on inputs, testable without a real pod.
• hostname.isBlank() catches both null and ""/whitespace-only, falling back to pid — confirmed against the test's buildRunSalt("", …) == buildRunSalt(null, …) assertion.
• Hostname is run through safeKey (charset-restricted to [A-Za-z0-9._-], LoadRun.java:502-507), so the salt stays URL-safe even for an adversarial/exotic HOSTNAME. I checked the one subtlety in safeKey: it returns the sentinel "?" only when its input is empty — but buildRunSalt never calls safeKey on an empty string (the blank check short-circuits to the pid branch first), so that non-URL-safe sentinel can never leak into RUN_SALT. Good.
• Real Kubernetes pod names are already DNS-1123-compliant (lowercase alphanumeric + -), so safeKey is effectively identity for the real deployment target — no realistic collision introduced by the charset restriction itself.
• This resolves the actual bug: two pods sharing both millis and namespaced PID 1 now differ by node (the pod name), so the token streams no longer collide.

New test (LoadCorrelationTest.java:192-208) exercises exactly the three claims in the fix:

1. Same millis + same PID 1, different HOSTNAME → distinct salts (the actual pod-collision scenario).
2. Blank/null HOSTNAME → falls back to pid, and blank/null are equivalent (not an empty slot).
3. An exotic hostname ("pod/../x&y=1") can't inject an unsafe character into the salt.

All imports (assertEquals/assertNotEquals/assertTrue) the new assertions need are already present in the file's existing import static block, so this should compile cleanly.

Docs (docs/DESIGN-DECISIONS.md) — Decision 1's rationale is rewritten to state the actual PID-namespace mechanism instead of the superseded "different pid" claim, the alternatives-considered entry for the salt format is updated to <millis>x<host>x<pid>-<counter>, and the "self"/"other" reserved-bucket caveat from my prior review is now recorded as an accepted tradeoff. All three read accurately against the code.

No further issues found. This closes out finding #1 from the earlier review; the PR looks good from my side.
· Branch: feat/nonce-payload-dd038

[ianp94]

Approving. I gated this feature's spec earlier, so I checked the implementation against both my spec-review points and the Claude App's finding — all landed in code, several with refinements beyond what was asked.

My spec concern #1 (cross-run nonce collision) is fixed the right way. The spec's original single-AtomicLong-seeded-with-millis scheme couldn't be cross-run-unique under load (counter advances by save-count, not milliseconds). This adopts the per-run salt I recommended — RUN_SALT = <millis>x<node>x<pid> then -<counter> — and buildRunSalt is pure/package-private so the collision-resistance is unit-testable without pods. Good testability discipline.

The Claude App's k8s finding is real and correctly fixed. millis+pid alone collides across pods (PID namespaces renumber from 1, and pods can start in the same millisecond); folding in HOSTNAME (the unique pod name, pid fallback for local runs) closes it, with buildRunSalt made testable specifically for that regression. Verified the fold is present and the token stays URL-safe (safeKey-restricted node, unreserved separators).

My spec concern #2 (cookie-through-redirect under no-follow) is addressed with the exact test I asked for. captureSessionCookie(c, jar) still runs on the direct response in fireR, and a302SetCookieStillPopulatesTheJar pins that a 302 with Set-Cookie still populates the jar — session continuity survives the no-follow flip.

My spec concern #3 (LoadFireTest audit) is handled cleanly. Rather than flipping the existing fire's default (which Java can't overload on return type anyway), they added a distinct fireR returning FireResult(code, location); the int fire(...) delegates via .code(), so existing LoadFireTest assertions are untouched and the redirect tests are additive.

Component 2 exceeded the guidance. The redirectTargets cap uses the best-effort admitKey pattern I recommended (benign race, ≤12 + "other"), AND the summary emits a deterministic top-12-by-count. Beyond that, they caught a robustness issue I only gestured at: because summaryJson builds JSON via raw String.format, a fuzzed/reflected Location could emit an invalid JSON key and make the operator's json.Unmarshal silently drop the entire summary — so normalizeLocation restricts output to [A-Za-z0-9._-]. That's the right defensive call at the 4 KB termination-message boundary.

Design improvement over the spec: moving nonce from a reserved ${{nonce}} name to a <nonce> grammar generator (author writes $rev = <nonce>) reserves nothing in the author-controlled ${{}} namespace, and the ${{@nonce}} fire-time marker (with @ outside Capture.NAME_PATTERN) is collision-proof by construction. The generator→marker split correctly separates grammar-expansion-time from fire-time, which is necessary — a baked value would repeat across fires of one expanded sequence, defeating the whole point. Verified @nonce fills without masking a genuine unbound ${{ref}} (the else-branch still returns null → step skips) and is exempt from lintCorrelationOrdering.

All CI green including the in-cluster e2e; 216 tests; summaryJson's new params updated at both LoadDriftUnavailableTest call sites. Clean close of the DD-038 pair.

(reviewed at d52ff7e)