Skip to content

runner: time-boxed exit + end-of-run summary (P5a) - #18

Merged
ianp94 merged 2 commits into
mainfrom
feat/runner-duration-summary
Jul 20, 2026
Merged

runner: time-boxed exit + end-of-run summary (P5a)#18
ianp94 merged 2 commits into
mainfrom
feat/runner-duration-summary

Conversation

@ianp94

@ianp94 ianp94 commented Jul 20, 2026

Copy link
Copy Markdown
Owner

First slice of operator P5a (DD-025): the two additive runner changes the campaign needs, so the harness itself is ready before the operator drives it.

  • -Dclosurejvm.run.duration (10m/30s/500ms/bare-seconds) — stops the coverage-guided loop and exits cleanly at the deadline. Enforced inside the runner, not via Job activeDeadlineSeconds: a SIGKILL would report Failed and skip the summary write (both broken). When set without an iteration count, the deadline governs.
  • -Dclosurejvm.summary.out=<path> — on shutdown, writes the StatusReporter snapshot JSON (coverage %, findings, crashes, invariants, iterations) for the operator to read into campaign status (DD-025 §7a, decoupled from the dashboard). Written via a shutdown hook so it lands on normal and deadline exits.

parseDurationMillis unit-tested (suffixes / bare seconds / whitespace+case / fractional). USAGE flags documented. Full Java suite green.

Next P5a slices: the closurejvm/runner image, the ClosureJVMCampaign CRD + reconciler (+ coverage-classes initContainer), and the e2e.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JuGqspSB9zpA5AyvHJBiDK

…mpaign)

The two additive runner changes DD-025 §7a/§7.2 specified, so an operator
campaign can bound a coverage-guided run and read its result:

- -Dclosurejvm.run.duration (10m/30s/500ms/bare-seconds): stop the loop
  and exit CLEANLY at the deadline. A Job activeDeadlineSeconds SIGKILL
  would report Failed AND skip the summary write, so the bound is enforced
  inside the runner instead. When set without an iteration arg, the
  deadline governs.
- -Dclosurejvm.summary.out=<path>: on shutdown, write the StatusReporter
  snapshot JSON (coverage %, findings, crashes, invariants, iterations)
  to <path> for a supervisor to read. Via a shutdown hook so it lands on
  both a normal and a deadline-triggered exit.

parseDurationMillis unit-tested (suffixes, bare seconds, whitespace/case,
fractional). USAGE flags documented. Full suite green.

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

ianp94 commented Jul 20, 2026

Copy link
Copy Markdown
Owner Author

@claude please review — first slice of operator P5a (DD-025): the two additive runner changes (-Dclosurejvm.run.duration clean-exit time-box + -Dclosurejvm.summary.out end-of-run summary).

Worth checking:

  1. Duration semantics — the deadline break at loop top exits cleanly (falls through to renderFinal + the summary shutdown hook); when duration is set without an iterations arg, iterations is forced to MAX so the deadline governs. Any off-by-one or case where the loop over-runs / the summary doesn't get written?
  2. Summary via shutdown hook — writes StatusReporter.snapshotJson(); does that fire reliably on a normal exit, and does it have the numbers a campaign needs (coverage %, findings, crashes, invariants)?
  3. parseDurationMillis — the suffix handling (ms/s/m/h, bare=seconds) and the tests.
  4. Anything that'd bite when the operator runs this in a Job (P5a continues).

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

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


Review — runner: time-boxed exit + end-of-run summary (P5a)

  • Gather context (diff, CLAUDE.md, related files)
  • Review duration semantics (loop-top break, MAX iterations forcing)
  • Review shutdown-hook summary write
  • Review parseDurationMillis + tests
  • Consider Job/operator runtime implications
  • Post review feedback

Solid, well-scoped additive slice — clean-exit time-box and shutdown-hook summary are both the right shape for DD-025 §7a. One finding I'd fix before this feeds the operator Job, plus a couple of minor notes.

1. Summary will silently report all-zeros unless -Dclosurejvm.status=true is also set (the main thing worth fixing)

writeSummary (runner/coverage/CoverageGuidedRun.java:230-238) writes StatusReporter.snapshotJson() unconditionally. But every counter that feeds that JSON is gated behind StatusReporter.ENABLED, which is Boolean.getBoolean("closurejvm.status") set once at class-init (runner/util/StatusReporter.java:19):

  • recordIteration (agent/Agent.java:234StatusReporter.java:81-99) — no-ops if disabled, so iterations stays 0
  • recordCrash (StatusReporter.java:101) — no-ops, so crashes stays 0
  • recordSaved (StatusReporter.java:110-120) — no-ops, so corpus/findCrash/findInvariant stay 0
  • recordCoverage (StatusReporter.java:128-139) — no-ops, so coveredEdges/totalEdges (and pct) stay 0

snapshotJson() itself (StatusReporter.java:215-237) doesn't check ENABLED — it always returns valid JSON, just full of zeros when disabled. So if an operator campaign sets only -Dclosurejvm.summary.out=<path> (as the CAMPAIGN-DESIGN.md:143-149,166 sketch implies) without also passing -Dclosurejvm.status=true, the driver Job exits cleanly, writes summary.json, and the operator reads a "successful" run with coveragePct: 0, findings: 0, crashes: 0, iterations: 0 — indistinguishable from a real all-clean run. That's a worse failure mode than an error, since it's silently wrong rather than loudly broken.

Two ways to close this:

  • Minimal: when summaryOut is set and !StatusReporter.isEnabled(), log a loud stderr warning (or fail fast) in main() so a misconfigured Job surfaces immediately instead of producing a plausible-looking zeroed summary.
  • Better for P5a: have the eventual closurejvm/runner Job template always pass -Dclosurejvm.status=true alongside -Dclosurejvm.summary.out, and call that dependency out explicitly in docs/USAGE.md's new summary-out row (currently doesn't mention it needs status=true to have real numbers).

Fix this →

2. Duration semantics — correct, but "clean exit" is best-effort, not precise

The deadline check at the loop top (CoverageGuidedRun.java:148) is checked once per iteration before any work for that iteration starts, so it can't infinite-loop or skip the summary — confirmed correct, and break falls through to renderFinal() + the shutdown hook exactly as described.

One thing worth documenting rather than fixing: because the check only happens between iterations, a run can overrun the deadline by up to one iteration's/sequence's wall time — bounded by the HTTP readTimeout (10s, CoverageGuidedRun.java:299) per request, and a multi-step sequence (runSequence) can chain several of those before the next check. For the short durations shown in the tests/USAGE example (500ms), real elapsed time could exceed the budget by an order of magnitude. Not a bug, but worth a one-line caveat in docs/USAGE.md ("best-effort — a slow in-flight request can push the actual exit past the deadline") so operators don't treat it as a hard bound when picking activeDeadlineSeconds backstop values.

iterations forced to Integer.MAX_VALUE only when args.length == 0 (i.e., duration set without an explicit count) is correct — and if both are somehow set, the loop naturally stops at whichever bound is hit first, which is reasonable dual-bound behavior even though the CRD is meant to validate duration XOR iterations upstream.

3. parseDurationMillis — logic and tests look right

Suffix ordering handles "500ms" before the "s"/"m" single-char checks correctly (checked first), bare-number-as-seconds and fractional parsing ("1.5s" → 1500) are both correct, and whitespace/case tolerance is real (.trim().toLowerCase() up front). The four tests cover the meaningful cases well. Not blocking, but the parser has no test for malformed input (e.g., "ms" with no digits, negative values) — Double.parseDouble will throw NumberFormatException in that case, which propagates out of main() before the summary shutdown hook gets registered (duration is parsed at CoverageGuidedRun.java:102-107, summary hook registered after at :110-114), so a bad duration string also means no summary file at all. Low priority since P5a's CRD is meant to CEL-validate this shape before it reaches the runner, but worth keeping in mind if the runner is ever invoked directly with a bad value.

4. Job-runtime notes (for the next P5a slices)

  • Shutdown hooks (ClosureJVM-Summary, StatusReporter's own ClosureJVM-Status-Final, TriageSink's flush hook) run concurrently on separate threads with no ordering guarantee — fine here since snapshotJson() is synchronized and self-consistent, just noting it was checked.
  • All background threads (StatusReporter render loop, DashboardClient push loop, TriageSink consumer) are daemon threads, so a normal deadline-triggered break really does let the JVM exit on its own without needing System.exit — good, this is what makes the "SIGKILL skips the summary" problem this PR solves actually solvable.
  • writeSummary's catch (Exception ignored) won't catch Error (e.g., disk-full-ish IOError), but since shutdown hooks run on their own thread and don't block JVM exit either way, this doesn't risk hanging the run — just a note that it was considered, not a request to change it.

Nothing here blocks merging the additive change itself — the duration/exit mechanics and the parser are solid. I'd fix or at least explicitly flag #1 before wiring the operator Job in the next P5a slice, since it undermines the summary's entire purpose (coveragePct/findings/crashes for campaign status) without any visible error.
· branch feat/runner-duration-summary

…eview)

Real silent-failure gap: StatusReporter's counters are all no-ops unless
-Dclosurejvm.status=true, but snapshotJson() always returns valid JSON —
so summary.out without status would write a plausible all-zeros summary
(coverage 0, findings 0), which an operator reads as a clean run rather
than a misconfiguration. Warn loudly on that combination. (The campaign
reconciler already passes status=true; this catches hand-runs.)

Also documented in USAGE: summary.out requires status=true, and the
duration deadline is best-effort (checked between iterations, so a slow
in-flight request can overrun it).

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

ianp94 commented Jul 20, 2026

Copy link
Copy Markdown
Owner Author

Fixed the silent-failure gap (#1) in 9b0b7c1: the runner now warns loudly when -Dclosurejvm.summary.out is set without -Dclosurejvm.status — so a hand-run misconfiguration surfaces instead of writing a plausible all-zeros summary. (The campaign reconciler in #19 already passes status=true, so the operator path is correct; this is the defense for direct invocation.) Also documented in USAGE that summary.out requires status=true, and that the duration deadline is best-effort (checked between iterations — a slow in-flight request can overrun it), per your #2. Left the malformed-duration note (#3) as-is since the CRD CEL-validates the shape upstream. Merging.

@ianp94
ianp94 merged commit bf9f83f into main Jul 20, 2026
4 checks passed
@ianp94
ianp94 deleted the feat/runner-duration-summary branch July 20, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant