Retry failed test group cases - #77
Merged
Merged
Conversation
A test group can now retry failed cases: once, twice, or by re-running the whole group once. Retries run as extra passes after the entire group has finished, and each attempt is its own TestRun row so a failed attempt keeps its events, screenshots, and error trace. The budget is per case and counts only attempts that reached PASS/FAIL, so a case cancelled by stop-on-failure still gets its full allowance once it finally runs. Under STOP, planning stops once an unresolved case has exhausted its budget: cases behind a permanently broken one sit at executed = 0 forever, so without that rule the round loop would never terminate. A retry round replays the group's normal execution and failure rules over the cases the plan selected, which is what makes a STOP group resume correctly — it re-runs the failed case and then continues into the cases cancelled behind it rather than leaving them cancelled. A whole-group retry clears the captured login baselines so its login flows re-run; a failed-case retry keeps them, since login flows that passed are not in the plan but their dependent cases still need them. RunSession.retryPending solves the central problem: without it the rollup settles the session the moment round 0's last member goes terminal, emitting the terminal event, posting the Slack summary, and releasing the hasActiveSession edit lock before any retry starts. It holds a would-be-FAIL session RUNNING through that gap; PASS and CANCELLED still settle at once, since an all-pass group has nothing to retry and a stopped one must not retry. Every path that ends execution releases the hold or a fully-terminal session would stay RUNNING forever and block the group. The stranded-session reaper also had to learn about sessions with no active member at all, which is the shape of a group stranded between rounds by a crash. Schema, types, planning rules, session lifecycle, and the round loop land together because a required TestGroupSummary.retryPolicy couples them — splitting further would leave commits that do not compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A retried group has one member row per attempt, so counting memberRuns directly double-counts a case and reports failures it has already recovered from. The Slack group summary and the run-session API now reduce to the latest attempt per case; the API also returns each case's attempt number and its earlier attempts so they stay inspectable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four choices: no retry, retry failed cases once or twice, or retry the whole group once. The run table keeps one row per case, showing the final attempt with an attempt badge and links to the earlier ones. Raises the locale line ceiling by 7 for the six selector strings plus the attempt badge, none of which are reducible. That script asks for the locale modules to be split rather than bumped again, so its note now says the split is overdue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_run_session returns retryPolicy, retryPending, and each member's attempt, so an agent reading a retried group does not mistake repeated cases for duplicates or treat a mid-retry FAIL as final. Tool names and input schemas are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records the three ways this feature is easy to break: counting attempt rows instead of reducing to the latest attempt, ending a session's execution without releasing the retry hold, and adding an eligibility rule without a termination argument. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The retry hold only guarded the FAIL branch of the rollup, but WHOLE_GROUP_ONCE re-runs every case regardless of status. An all-passing round 0 therefore settled PASS the moment its last member finished: terminal event emitted, Slack summary posted, group edit lock released — and only then did the retry pass start, flipping the session back to RUNNING and settling it a second time. Move the check ahead of the outcome branches so retryPending suppresses any settle while it is set. Every path that ends execution already releases the hold first (the orchestrator's finally, the stop button, the stranded-session reaper), so a stopped or crashed session still settles on its real outcome instead of waiting here.
The ceiling was tested before the round was planned, but it equals the number of rounds a healthy run may legitimately use: 1 for WHOLE_GROUP_ONCE, and retries * caseCount for the failed-case policies. So every whole-group retry, and every single-case FAILED_ONCE group, logged "hit the safety ceiling" on a perfectly normal run — a warning whose whole purpose is to report a bug. Plan the round first and return on an empty plan, so the ceiling is only consulted when the loop still wants to run and reaching it really does mean the plan is not converging.
A retried case has one TestRun row per attempt, so the two memberCount readings that came straight from _count.memberRuns grew as a group retried: a 5-case group reported 7 members on the group's session list and through list_run_sessions once two cases were retried. Add countSessionCases next to resolveLatestAttempts — the two together now cover the whole invariant, one for statuses and one for totals — and route both call sites through it. Also swap the run-session route's per-row linear scan over the final attempts for a Set lookup.
npm audit reported 16 advisories that were not present when this branch was cut. Every one lands on a package the overrides already pinned, with a vulnerable range that now ends exactly at the pinned version — these are fresh disclosures against versions previously considered patched, not a regression from this branch (the lockfile is unchanged from main). Raise each override past its new range, and the matching CVE floors so a later resolution cannot drop back under them. Two nested override entries shadowed their flat keys and had to move in lockstep, or the vulnerable copies stayed nested: @midscene/core@1.9.2 > js-yaml 4.3.0 -> 4.3.1 monaco-editor > dompurify 3.4.12 -> 3.4.13 Also declare js-yaml in apps/web. Five first-party modules import it — including two API routes — but no manifest listed it; it resolved only because a transitive copy happened to hoist to the root. Re-resolving left the remaining copies nested and broke those imports, so the undeclared dependency had to become a real one. npm audit is clean and npm run verify passes end to end, including the lockfile-floor gate that the audit failure previously short-circuited.
WHOLE_GROUP_ONCE returned every case unconditionally on the first retry round, so a group that came back fully green still burned a second complete pass — double the wall-clock and double the AI-action spend for a run that had nothing wrong with it. Require an unresolved case before any policy retries, which is what the failed-case policies already did. The trigger is now uniform across policies and only the scope differs: WHOLE_GROUP_ONCE re-runs every case including the passing ones, because a sequential group's later cases may depend on state its earlier ones build. This also removes the reason the rollup held a would-be-PASS session at RUNNING, so that hold goes back to covering only FAIL and CANCELLED. A green group settles promptly again, and a crash straight after round 0 no longer defers a correct PASS until the reaper's stale window elapses.
A mutation sweep over the retry code found the suite blind wherever state was involved. Twelve deliberate defects, 502 tests: seven survived, including every invariant the design notes call load-bearing — the rollup settling despite retryPending, rolling up every attempt instead of the latest, releaseSessionRetryHold doing nothing, and the stop button skipping it. All five defects the suite did catch were in pure functions. The cause was one gap. run-session-service.ts had no test file, and the orchestrator harness mocks out recomputeRunSessionStatus and releaseSessionRetryHold, so the composition of the hold, the latest-attempt reduction and the rollup was never executed by anything — which is exactly where both bugs found in review had lived. Add three suites that drive the real functions: - run-session-service: the recompute seam against an in-memory RunSession + TestRun pair, covering the hold, retried-case rollup, single terminal emission, and failActiveSessionMembers releasing the hold before it settles. - cancel-run: the stop button releases the hold, and does so *before* the rollup — the ordering is what lets a group stopped between rounds settle now instead of waiting on the reaper. - test-group-retry-runner: the loop driven directly through its injected runRound, reaching the no-progress backstop a real group cannot produce, and pinning how an attempt row is derived from its predecessors.
The prisma fake asserted nothing about how it was queried. It ignored `orderBy` and hardcoded the descending attempt sort that createRetryAttempts happens to want, so flipping that sort to ascending — which makes the second retry round recreate an attempt number that already exists — still passed. It also never looked at `runSessionId`, so dropping the session scope from a query passed too. Honour both instead of simulating them: apply the caller's own orderBy, and filter on runSessionId. Seed every group with a decoy attempt of the same case in another session at a far higher attempt number, so a query that loses its scope numbers the next attempt from the decoy and fails loudly rather than being coincidentally right. Also make an over-run of a declared outcome script an error rather than a silent PASS. That immediately exposed one test whose comment claimed a case "exhausts after attempt 3" while attempt 3 was in fact defaulting to PASS, so it asserted the case recovered when it meant to assert the case stayed failed with its sibling's budget untouched.
The rollup guarded the transition into a terminal status atomically, but the transition out of it was a bare update. Retry rounds are the first thing that inserts new QUEUED members into a session whose members were all terminal moments earlier, which makes that reachable: a stop landing in the gap between rounds settles the session, then createRetryAttempts inserts its rows, and the next recompute reopens a session that had already reported its result. The effect was worse than a flicker. Because the rollup reduces to the latest attempt per case, the reopened session read as QUEUED — as if it had never run — with completedAt cleared, and settling again emitted a second terminal event whose status overwrote the first. Guard the write the same way as the terminal one, so "a settled session stays settled" holds at the write rather than by luck of ordering.
Two members lists on this page still described a retried group wrongly. The login-flow prefix list, which tells a queued test which login flows it is waiting on, was not reduced to the latest attempt. A whole-group retry re-runs its login flows, so the same flow appeared twice and its superseded failure was reported as the current reason the test had not started. Reduce it like every other members list. The copy-log listed session members as final attempts only, and marked "<- this run" on an exact runId match. Opening an earlier attempt — which the session page links to directly — therefore produced a bundle in which the run being viewed was absent and its case was reported as whatever the retry became, so a copied diagnostic for a failed attempt asserted the case had passed. It now lists each superseded attempt, marks the viewed run wherever it appears, and says outright when the run being viewed was superseded and how that case ended. Also cover the session GET, which this series reshaped without tests.
Attempt numbers are assigned in application code as max(attempt) + 1 per case, with nothing stopping a retry from reusing one. A duplicate would fail quietly rather than loudly: the case's executed count would double, cutting its retry budget below what the policy promises, and the latest-attempt reduction would tie-break on whatever order the rows came back in. Add the unique index so such an insert is rejected instead. Folded into this branch's existing migration rather than added as a second one, keeping the change series at a single migration. Standalone runs are unaffected: their runSessionId is null, which Postgres treats as distinct, so two runs of one case still coexist. Verified on a scratch database — attempts 1 and 2 of a case insert fine, a second attempt 2 is rejected by name, and two null-session runs of the same case both succeed. `migrate diff` reports no drift from schema.prisma, and the replay/idempotency/rollback verifier passes.
stop_all_runs settled each active row with a per-run cancel, which never reaches the session's in-process driver: the status watcher only aborts that member's own controller, never the session's. A group with a retry policy therefore kept going after the stop, and because a cancelled attempt spends no retry budget, every case it had just cancelled came back eligible with a full allowance — so stopping a retrying group could spend more AI actions than it saved. Route runs that belong to a session through cancelActiveRunSession, which aborts the driver, releases the retry hold, and rolls the session up. Standalone runs keep the per-run path. cancelActiveRunSession takes an optional reason so MCP stops stay attributable rather than being recorded as a UI stop. stop_all_queues deliberately keeps cancelling row by row: draining the queue must not kill a running test. The next commit stops its cancelled cases from being retried.
Retry planning treated every CANCELLED attempt as unresolved, which is right for a case the group skipped behind a failure but wrong for one a person stopped: since a cancelled attempt spends no budget, stopping a run handed that case its whole allowance back. Reachable from the UI stop button and from all three MCP tools that cancel runs — stop_all_runs, stop_all_queues, and the cancel update_test_case performs to apply an edit, which could then re-run the case against the edited config. The cancellation reason already carries the distinction, so classify on it: USER_SINGLE, USER_GROUP, MCP and MCP_FOR_UPDATE resolve the case, while EARLIER_CASE_FAILED and LOGIN_FLOW_FAILED stay retryable so stop-on-failure still resumes. An unrecognized reason stays retryable too, so an unmapped string can never quietly weaken that.
get_run_session returned every attempt row, and in the gap between retry rounds all of them are terminal while the session is not — so the obvious reading of "all members settled" said a retrying group had finished. Return one entry per case at its latest attempt instead, with superseded ones and their reason codes under previousAttempts, matching what the web session route already returns. That also makes members.length agree with list_run_sessions' memberCount again. get_test_run now reports the run's own attempt, so an agent handed a runId from a failed group can tell a superseded attempt from the current one. The tool description no longer asks the caller to reduce attempts itself; it says to judge completion by the session status. Manifest snapshot updated deliberately — tool names and input schemas are unchanged.
Stopping was session-wide from the UI and the single-run HTTP cancel — that route already escalates any member to cancelActiveRunSession — but two MCP paths still settled rows one at a time, so a stop left the group running: - stop_all_queues cancelled queued members individually. A queued member cannot be drained out of a live session, because the session's driver decides what runs next and a retry policy re-queues what was cancelled. - update_test_case with cancel_and_save cancelled only this case's run, so its group carried on — and with a retry policy would re-run this very case against the edit being saved. Route both through the same session-aware helper, so all five stop paths now behave identically: a run that belongs to a session stops the session, and standalone runs are still cancelled individually. Stopping a queued member therefore also stops that group's running member. That is the intent, but the count would otherwise be quietly larger than what the caller asked for, so the helper reports the extra members in sessionMembersAlsoCancelled and all three tools surface it.
Stopping is session-wide, so a caller naming one queued case can end a whole test group including the case running right now. That is the intent but it is not visible in the request, so stop_all_runs and stop_all_queues now refuse until the caller answers. When any matched run belongs to a session they return SESSION_STOP_CONFIRMATION_REQUIRED, naming the affected test groups and reporting how many members would settle versus how many were asked for, and cancel nothing. The caller re-calls with activeSessionResolution: stop_sessions to end those groups too, or only_standalone to leave them running and stop just the runs outside a session, which come back listed in sessionsLeftRunning. Runs outside a session need no confirmation, so stopping ad-hoc runs stays one call. The shape mirrors update_test_case's ACTIVE_RUN_CONFIRMATION_REQUIRED so an agent meets one confirmation pattern rather than two. update_test_case does not re-confirm: choosing cancel_and_save is already the answer. Manifest snapshot updated deliberately — this adds an optional input to two tools, and an existing caller now meets the confirmation instead of silently stopping more than it named. Also bring the agent skills and docs in line: - skytest gained the stop-confirmation rule and a retry section; it had no mention of retries at all, so an agent watching a group could read the gap between rounds as completion. - skytest-fix now checks a run is still the current attempt before diagnosing it, so a flake a retry already absorbed is not "fixed", and knows not to stop runs to investigate. - review skill pointed at apps/web/src/i18n/messages.ts, which has not existed since the locales were split into modules.
The team does not read documentation in this repo; the actual readers are coding agents, which CLAUDE.md routes to specific files. Keep that set and drop the rest. Removed the six operator self-hosting runbooks, five maintainer docs that were human process rather than agent reference (AI provider troubleshooting, CLI Homebrew release, dependency lifecycle policy, performance observability, repository hygiene), and the docs index, whose job CLAUDE.md already does. Ten maintainer docs remain: the runtime invariants, contracts and diagnostics an agent needs to change this code safely. Nothing was deleted while still referenced. Inbound links were rewritten in CLAUDE.md, both READMEs, apps/macos-runner/README.md, three remaining maintainer docs, and three development skills — setup pointers now name the make targets and the surviving maintainer docs instead. All 27 relative markdown links across tracked files resolve, and no tracked file mentions docs/operators or docs/README any more. Note for whoever self-hosts from this repo: the operator setup path is now only `make bootstrap` / `make dev` plus infra/README.md. If external self-hosters need those runbooks, restore them from this commit rather than rewriting them.
Both stop tools searched for runs in an active status. The retry hold makes
a session live with every member terminal, so in the gap between rounds
there is no active run to find: stop_all_runs reported
requestedActiveRuns: 0, cancelled nothing, showed no confirmation, and the
retry loop went on to create the next round. An explicit stop was a silent
no-op precisely while a group was running.
This is the same class as the settled-session resurrection fixed earlier —
the hold created a state ("live with nothing active") that surrounding code
had no reason to expect. Before retries, no active runs did imply no live
session, because the rollup could not hold an all-terminal session open.
Stop now starts from live sessions as well as active runs:
findLiveSessionIdsForStop returns the project's non-terminal sessions, the
confirmation gate describes them like any other affected session, and
cancelRunsForStop stops them even when no requested run belongs to them.
Session ids are deduped, so a session reached both ways is cancelled once.
Recorded as a footgun in the maintenance guide: anything that stops, sweeps
or reaps must consider non-terminal RunSession rows, not just active runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a user-selectable retry option to the Test Group config page. Retries run as extra passes after the whole group has finished; each attempt is its own
TestRunrow, so a failed attempt keeps its events, screenshots, and error trace.Every policy shares one trigger: a retry round only starts if some case did not pass. A group that came back fully green never retries, whatever its policy. The policies differ only in scope — the failed-case ones re-run just the unresolved cases, while whole-group re-runs every case including the passing ones, since a sequential group's later cases may depend on state its earlier ones build.
Interaction with sequential runs and stop-on-failure
These were the two behaviours most at risk, and they fall out of one rule: a case is retry-eligible when its latest attempt is
FAILorCANCELLED.Because a retry round replays the group's normal execution and failure rules over the planned cases, a stop-on-failure group resumes correctly rather than leaving its tail cancelled:
The budget is per case, not per group
A
CANCELLEDattempt never executed, so it spends no budget. A case skipped by stop-on-failure therefore keeps its full allowance when it first actually runs during a retry round:This makes the round count open-ended, so termination needs an explicit rule: under stop-on-failure, planning stops once an unresolved case has exhausted its budget. Cases behind a permanently broken one sit at
executed = 0forever, so without it the loop would replan them indefinitely. Two backstops sit behind that — a no-progress guard, and aretriesPerCase × caseCountceiling that is provably unreachable and only exists so a bug settles the session instead of wedging it.The main hazard this had to solve
Without a hold, the rollup settles the session the moment round 0's last member goes terminal — emitting the terminal event, posting the Slack summary, and releasing the
hasActiveSessionedit lock, all before any retry starts.RunSession.retryPendingholds a would-be-FAILor would-be-CANCELLEDsession atRUNNINGacross that gap, because either outcome can still be retried.PASSsettles immediately — no policy retries a green group. Every path that ends execution releases the hold — the orchestrator'sfinally, the stop button, and the stranded-session reaper — or a fully-terminal session would stayRUNNINGforever and permanently block the group.Two consequences worth reviewing:
FAILafter its retry passed. Same reduction applied to the Slack group summary and the run-session API, which previously countedmemberRunsdirectly.reapStrandedRunSessionshad to learn about sessions with no active member at all — the exact shape of a group stranded between rounds by a crash.Testing
TestRuntable rather than generic mocks, so a broken round loop cannot pass — including the per-case-budget and non-termination cases above.prisma migrate diff --exit-codereports no drift fromschema.prisma;db:migrate:test(replay + idempotency + rollback simulation) passes.sessionPosition = 0, the session settlingPASSafter a recovered retry despite failed rows remaining, and the reaper's awaiting-retry branch.Notes for the reviewer
check-config-i18n-guardrails.mjs) for the six selector strings plus the attempt badge, none reducible. That script asks for the locale modules to be split rather than bumped again; its note now says the split is overdue. Happy to do the split first if preferred.npm run verifynow passes end to end, audit included. 16 advisories appeared after this branch was cut, all against packages the overrides already pinned at what were then the patched versions; they are bumped past the new ranges here, with the CVE floors raised to match. This also surfacedjs-yamlbeing imported by five first-party modules while declared in no manifest — it resolved only via a hoisted transitive copy — so it is now a declaredapps/webdependency.🤖 Generated with Claude Code