Skip to content

Invoke-OverlappingPoolTests: distinguish "no overlap" from "a run timed out" in the failure message #148

Open

Description

@github-actions

What

In tests/Invoke-OverlappingPoolTests.ps1, the overlap-detection logic only counts a run toward overlap when EndIsExact is $true. When a run's ExitTime throws (e.g. the process was killed by Kill() on the timeout path, or HasExited is $false), EndIsExact stays $false and that run is silently excluded from the overlap check. The script then fails with a message telling the operator to "lower -StaggerSeconds", even when the real cause was a run timing out.

The failure message must distinguish between:

  1. No overlap — all runs produced exact end times, but the intervals genuinely don't overlap. The existing "lower -StaggerSeconds" guidance is correct here.
  2. A run timed out — one or more runs failed to produce an exact end time, so overlap could not be evaluated for them. The message should name the timed-out run(s) and point at the timeout, not at -StaggerSeconds.

Why

The current message is actively misleading. An operator who sees "lower -StaggerSeconds" will tune the stagger and re-run, when the actual problem is a run that exceeded its timeout (or was killed). This wastes CI cycles and obscures the real failure. The review flagged this as acceptable-but-misleading; the fix is small and removes a real diagnostic trap.

How

In tests/Invoke-OverlappingPoolTests.ps1:

  1. Track which runs failed to produce an exact end time. When ExitTime throws or HasExited is $false, record the run (index/name and the reason) in a collection alongside setting EndIsExact = $false.
  2. After the runs complete, branch the failure message:
    • If the timed-out collection is non-empty, emit a message that names the affected run(s) and states that overlap could not be evaluated because the run did not exit cleanly / within its timeout. Do not suggest lowering -StaggerSeconds.
    • If the timed-out collection is empty and no overlap was detected, keep the existing "lower -StaggerSeconds" message.
    • If both conditions hold (some runs timed out and the remaining exact runs don't overlap), report both facts rather than picking one.
  3. Keep the existing behavior of failing when no run reported an exact end — that case is already handled, but make sure its message is consistent with the new wording.

Prefer a small helper that formats the failure message from the collected state, so the branching is testable and the message text lives in one place.

Files Affected

  • tests/Invoke-OverlappingPoolTests.ps1

Constraints

  • Must not change the pass/fail semantics: the script still fails when runs don't overlap, and still fails when no run reported an exact end.
  • Must not change the CLI surface of Invoke-OverlappingPoolTests.ps1 (no new required parameters).
  • Out of scope: the other follow-ups from the same review (Register-SlotPidFile read order, Stop-Slot log PID, Start-Slot stranger-PID test, Get-SlotProcess permission-error documentation).
  • Do not weaken the existing check that a single timed-out run among otherwise-exact runs is still surfaced — it should be reported, not swallowed.

LLM tier

Sonnet — small, localized change, but it requires judgment about message wording and how to combine the two failure conditions without regressing the existing pass/fail behavior.

Success looks like

  • A run that times out produces a failure message that names the timed-out run and does not mention -StaggerSeconds.
  • A run where all end times are exact but intervals don't overlap still produces the existing "lower -StaggerSeconds" guidance.
  • The case where no run reported an exact end still fails, with a message consistent with the new wording.
  • The case where some runs timed out and the remaining exact runs don't overlap reports both facts.
  • No change to the script's parameters or exit-code semantics.

Failure looks like

  • The timeout case still emits "lower -StaggerSeconds".
  • The no-overlap case loses its existing guidance.
  • A timed-out run is silently dropped from the report (the original bug, just reworded).
  • The script starts passing when it previously failed, or vice versa, for any of the three cases above.

Follow-up from AI review of PR #124.

Activity

  1. added
    ai-suggestedSuggested by AI code review
    sonnetSuitable for Claude Sonnet (moderate reasoning)
    on Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ai-suggestedSuggested by AI code reviewsonnetSuitable for Claude Sonnet (moderate reasoning)

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions