Skip to content

feat: load as a first-class citizen (DD-033) - #77

Merged
ianp94 merged 14 commits into
mainfrom
feat/load-first-class
Jul 21, 2026
Merged

feat: load as a first-class citizen (DD-033)#77
ianp94 merged 14 commits into
mainfrom
feat/load-first-class

Conversation

@basquin-bot

@basquin-bot basquin-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

What & why

Load campaigns were second-class everywhere they surfaced: the dashboard is hard-coded to explore (a load campaign showed iterations=0/crashes=0/"no coverage source" — empty explore scaffolding), and the CLI status table showed <none> coverage / 0 findings for load. The real load numbers only reached status.load via kubectl. LOAD-MODE-DESIGN.md promised a dashboard push that was never built.

DD-033 makes load first-class in the dashboard and the CLI.

How — one push path, made mode-aware

  • StatusReporter gains a mode + a load block; snapshotJson() emits "mode":"explore"|"load" and, in load, "load":{throughputRps, latencyMs:{p50,p90,p99,max}, heapDriftKb, threadDrift, serverErrors, requests}. Explore JSON only gains a "mode" field — additive.
  • LoadRun feeds it live: a snapshotter thread computes throughput/percentiles from the live histogram + drift, so the existing DashboardClient loop carries it and the dashboard updates during the soak. No second pusher.
  • Dashboard renders a mode-aware load card (throughput/p50-p90-p99/drift/5xx) + shows mode in the campaign list.
  • CLI status — a MODE column + mode-aware metrics (rps · p99 for load, cov% · finds for explore, pending for a load campaign with no results yet).

Correctness

  • Explore is behaviorally unchanged (JSON additive-only; the explore #sub elapsed/rate line stays independent of the /clusters fetch — a regression found and fixed in review; CLI explore row identical).
  • One drift poller, no hot-path lock. The snapshotter is the sole periodic /__basquin/drift poller; the terminal drift reuses its last poll; the per-request worker loop gains zero synchronization (DD-029 lock-free load preserved).
  • Live converges to terminal — same post-warmup window, same percentile()/histogram, and the final snapshot equals the terminal number. Live is approximate (torn reads, by design); status.load is the authoritative terminal record (the dashboard shows the last live snapshot, converged).

OTel-shaped, export deferred

Per the design discussion, metrics are typed for a future OTLP export (request latency = histogram basquin.load.request.duration, unit ms, the MAX_MS+2 bucket boundaries as the contract; throughput/5xx = counters; drift/coverage = gauges; campaign.id = resource attribute, mode = metric attribute) so the exporter is a thin adapter. No OTel dependency added — the OTLP export is its own roadmap DD, tied to clustered-runners histogram merge.

Verification

Unit: StatusReporterLoadTest, LoadSnapshotTest (live math == terminal), CampaignRow{Explore,Load,LoadPending}. Full suite 124/124; Go cmd/basquin + controller green. The in-cluster e2e asserts the load campaign's dashboard received mode=load + a non-empty load block (CI runs it).

Spec: docs/superpowers/specs/2026-07-21-load-first-class-design.md · Plan: docs/superpowers/plans/2026-07-21-load-first-class.md

🤖 Generated with Claude Code

basquin-bot Bot and others added 13 commits July 21, 2026 18:03
Make the one dashboard push path mode-aware (StatusReporter gains mode + a
load block; LoadRun feeds it live via a snapshotter so the dashboard updates
during the soak), add a load UI card + mode to the campaign list, and polish
the CLI status table (MODE column + mode-aware metrics from status.Load).
Metrics are typed for a future OTLP export (latency=histogram, throughput/5xx
=counter, drift/coverage=gauge, mode=attribute) so the exporter is a thin
adapter — no OTel dependency added; the OTLP export is captured as its own
roadmap DD tied to clustered-runners histogram merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
…Tel naming)

- Live snapshotter: torn reads acceptable / terminal authoritative (no hot-path
  lock); live throughput uses the post-warmup window so it converges to terminal;
  one drift poller (snapshotter-owned, terminal reuses it).
- OTel contract: basquin.load.latency -> basquin.load.request.duration; unit in
  the UCUM field not the name; the 1ms x 30000 bucket layout is the explicit
  histogram boundaries (breaking to change); campaign.id = resource attribute,
  mode = metric attribute.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
5 TDD tasks: StatusReporter mode+load block; LoadRun live snapshotter (one
drift poller, post-warmup window, torn-reads/terminal-authoritative); dashboard
load card + list mode; mode-aware CLI status; e2e + docs + OTel roadmap. Metrics
OTel-typed (request.duration/units/buckets/attributes) with the export deferred.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
…e drift poller (DD-033)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
…ove test into runner.coverage (DD-033)

Reverts the public widening from the prior commit; instead moves LoadSnapshotTest
into package runner.coverage (test/runner/coverage/LoadSnapshotTest.java), matching
the repo's existing convention for tests that need a package-private helper
(LoadRunTest, CostRankedReplayTest, CostCorpusPheromoneTest).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
… wrap (DD-033)

Hoist the explore elapsed/rate #sub update back out of the mode else-branch so
it fires immediately after the status fetch, not after the /clusters await +
generation guard — matching pre-load-card behavior and no longer stalling
behind a slow /api/analyze. Also fix a numeric-fallback nit (||'0' -> ||0) on
the throughput card.
`basquin status` showed a fixed CAMPAIGN/TARGET/PHASE/COVERAGE/FINDINGS/
DASHBOARD table, so load campaigns rendered <none> coverage and 0 findings
even though cp.Status.Load (throughput/latency percentiles/drift) is
populated for them. Add a MODE column and a mode-aware METRICS column:
explore shows coverage%/findings, load shows throughput rps and p99
latency from cp.Status.Load.

Extracted campaignRow(cp) as a unit-testable helper so mode-aware
rendering is covered directly (TestCampaignRowExplore/Load), independent
of the fake-client-backed printStatus tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
Review fix on the mode-aware status table (Task 4): a load campaign
still Running with no Status.Load published yet fell through to the
explore-shaped "<cov> · 0 finds", misleadingly implying a fuzz search
found nothing. campaignRow now special-cases mode=="load" && Load==nil
to render "pending" (nil-guard ordering preserved, no dereference).

Add TestCampaignRowLoadPending to isolate that path — the two prior
tests (load-with-results, explore) don't exercise it, so a regression
flipping the guard would otherwise pass silently.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
…bucket count, mark mode-aware-dashboard roadmap item delivered

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
…tus.load is authoritative

Final-review precision: the JVM can exit before the daemon push loop ships an
explicit terminal push, so the dashboard's last value is the last live snapshot
(converged to terminal) and status.load is the authoritative terminal record.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
@ianp94

ianp94 commented Jul 21, 2026

Copy link
Copy Markdown
Owner

@claude please review — load as a first-class citizen (DD-033): mode-aware dashboard (live during the soak, one push path) + CLI status. Explore is behaviorally unchanged; status.load stays the authoritative terminal record.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @ianp94's task in 3m 40s —— View job


Review: DD-033 — load as a first-class citizen

Todo list

  • Gather context (PR description, changed files)
  • Read repo conventions
  • Review runner/util/StatusReporter.java
  • Review runner/coverage/LoadRun.java (snapshotter thread, drift/percentile math)
  • Review runner/util/DashboardServer.java
  • Review resources/dashboard.html
  • Review operator/cmd/basquin/status.go + tests
  • Review new tests (StatusReporterLoadTest, LoadSnapshotTest, status_test.go)
  • Review docs (DESIGN-DECISIONS.md, spec/plan, TODO.md)
  • Check e2e script change
  • Post final review

CI is green (CI, Operator CI passed; Operator e2e (kind) still running as of this review).


Summary

Solid, well-documented change. The "make the one push path mode-aware" approach is the right call — no second pusher, additive JSON, and the DD-029 lock-free load-hot-path guarantee is genuinely preserved (only the snapshotter thread and the final terminal call touch StatusReporter.recordLoad; the per-request worker loop only touches the existing AtomicLongArray/AtomicLong counters). computeLoadSnapshot being shared between the live snapshotter and the terminal push is a nice way to guarantee live-converges-to-terminal, and LoadSnapshotTest exercises the bucket math directly. Docs (DESIGN-DECISIONS.md, TODO.md, spec/plan) are thorough and match the code.

One real finding, plus a couple of minor nits:

1. c.mode is rendered into the fleet view without HTML-escaping (resources/dashboard.html)

In tickFleet():

'<div class="s" style="margin-top:8px">iters '+c.iterations+' · crashes '+c.crashes+
  ' · cov '+(c.coveragePct>=0?c.coveragePct+'%':'—')+' · '+(c.mode||'explore')+'</div>'+

Every other field here (c.id, c.kind, c.classification, c.campaignNames elsewhere in the file) goes through the file's own esc() helper before hitting innerHTML; c.mode doesn't. On the server side, DashboardServer.strField() (the new helper backing this field) pulls the raw regex capture out of whatever body was POSTed to /ingest/status and embeds it unescaped into the /api/campaigns JSON — it only guards against breaking out of the JSON string ([^"]*), not against </>.

In normal operation this is harmless: mode is only ever set by StatusReporter.setMode("load"|"explore") with hardcoded literals. But /ingest/status is a shared-token-authenticated endpoint, not something that validates the JSON shape of the body (schema-agnostic scrape by design, per DD-013), so this is one write path away from stored XSS in the fleet view if that assumption ever slips (e.g. a modified/compromised driver). Given the file already treats every other reflected field as untrusted, this one should match: wrap with esc() at the render site (resources/dashboard.html, the tickFleet() template).

Fix this →

2. StatusReporterLoadTest relies on unreset static state (test/StatusReporterLoadTest.java)

StatusReporter's new loadRecorded/loadThroughputRps/etc. fields are static and never reset. recordLoadEmitsModeLoadAndAWellFormedLoadBlock resets mode back to "explore" at the end but leaves loadRecorded=true — so defaultModeIsExploreAndCarriesNoLoadBlock's assertion that there's no "load": block only holds if it happens to run before the other test in the same JVM. JUnit4's default method ordering isn't declaration order (it's a deterministic-but-unspecified hash order), so this is a latent order dependency rather than a guaranteed pass. Every other static-state test file in this repo (LoadModeTest, LoadModeControlTest, CostCorpusTest, RequestBoundaryTest) resets via @After; this one doesn't follow that convention. Low risk today (only 2 tests, likely stable ordering in practice), but worth a @After reset for consistency and to stop it from becoming a flaky test later if more cases are added to this file.

Nits / confirmed non-issues (for the record, since they looked suspicious at first read)

  • "throughputRps":"%.1f" being emitted as a quoted JSON string (not a number) in both StatusReporter.loadBlockJson() and LoadRun's terminal JSON looked off at first, but it's intentional and pre-existing: LoadStatus.ThroughputRps in the CRD (operator/api/v1alpha1/basquincampaign_types.go:171) is itself typed string ("formatted, e.g. 713.4"), and the dashboard JS does string concatenation either way. Consistent, not a bug.
  • The explore-JSON mutation in StatusReporter.snapshotJson() (strip trailing }, append ,"mode":... + load block, re-close) is string surgery rather than a JSON builder, but it matches the file's existing String.format-based JSON assembly style — not a new anti-pattern introduced by this PR.
  • The #sub timing-independence regression mentioned in the PR description (commit 3eb96a8) is correctly fixed in the current diff — the explore branch still sets #sub before the /clusters fetch, matching pre-PR behavior exactly.

Nothing here blocks merging — #1 is worth fixing before this ships to be consistent with the rest of the file's escaping discipline, #2 is a cheap test-hygiene follow-up.
· branch

…e load block on mode

1. resources/dashboard.html: wrap c.mode with esc() in tickFleet (every other
   reflected field is escaped; /ingest/status is schema-agnostic, so an untrusted
   mode value is one write-path from stored XSS in the fleet view).
2. StatusReporter.loadBlockJson: gate on "load".equals(mode) as well as
   loadRecorded — the block now appears iff mode==load (explicit invariant, no
   stale block on an explore snapshot), and StatusReporterLoadTest gets an @after
   mode reset (matching the repo's static-state test convention), removing the
   latent method-ordering dependency.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXSdVrJQy6DvELDbtptG5p
@basquin-bot

basquin-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Both findings addressed in 2fb7f70 (verified, not rebutted):

  1. Fleet c.mode unescaped — wrapped with the existing esc() in tickFleet(), matching every other reflected field. You're right that /ingest/status is schema-agnostic (DD-013), so an untrusted mode was one write-path from stored XSS in the fleet view.
  2. StatusReporterLoadTest static-state ordering — went one better than an @After alone: loadBlockJson now gates on "load".equals(mode) as well as loadRecorded, so the block appears iff mode==load (explicit invariant, no stale block on an explore snapshot) — which also makes the test order-independent — plus an @After mode reset for convention. Full suite 124/124.

Nits #1–3 confirmed as intentional/non-issues, thanks for recording them.

@basquin-bot basquin-bot Bot added the ready-for-approver Claude App feedback addressed, CI green; approver agent should review label Jul 21, 2026

@ianp94 ianp94 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Both Claude App findings verified as genuinely addressed in code, not just replied to:

  1. Fleet XSS: tickFleet now renders esc(c.mode||'explore') — checked the actual diff hunk, consistent with every other reflected field.
  2. Static-state ordering: loadBlockJson() gates on "load".equals(mode) && loadRecorded with the invariant documented in-code (a load block appears iff mode==load), plus the conventional @After reset — the stronger fix, as claimed.

Review highlights (practices + efficiency lenses):

  • The snapshotter honors every design commitment: one drift poller (terminal reuses lastDrift, single-poll fallback only if the snapshotter never ran), the live window anchored at measureFromNanos exactly like the terminal computation so live converges to terminal rather than diverging, torn-reads documented on the shared computeLoadSnapshot (and shared code means the "live matches terminal" test is testing the real thing), daemon thread interrupted+joined cleanly, terminal push before mode revert.
  • OTel table commitments all recorded: basquin.load.request.duration (not "latency"), UCUM unit field never in the name, the 1ms×30000+overflow bucket layout as a hard exporter contract, campaign.id as resource attribute vs mode as metric attribute.
  • CLI: the pending state for a load campaign with no Status.Load yet is exactly right — falling through to explore-shaped "0 finds" would have implied a fuzz search found nothing. Go tests cover both modes and the negative case (explore row must not show rps).
  • e2e closes the loop for real: queries the load campaign's own dashboard (token-authed) for mode:load + a non-empty load block before campaign GC. Green in CI.
  • strField defaulting to "explore" keeps pre-DD-033 payloads rendering — good back-compat thinking in a schema-agnostic scrape.

One observation, no change requested: throughputRps is a JSON string ("%.1f") in both the load block and status.load. That's consistency with the existing DD-029 CRD contract (ThroughputRps is a string field), so changing it here would be a schema break for no gain — but the future OTLP exporter must read the registry doubles, never parse this JSON, which the DD record already mandates. Just flagging so it stays deliberate.

(reviewed at 2fb7f70)

@ianp94 ianp94 added approved-awaiting-merge Approver agent approved; waiting for human merge and removed ready-for-approver Claude App feedback addressed, CI green; approver agent should review labels Jul 21, 2026
@ianp94
ianp94 merged commit 6d2219f into main Jul 21, 2026
11 checks passed
@ianp94
ianp94 deleted the feat/load-first-class branch July 21, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved-awaiting-merge Approver agent approved; waiting for human merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant