Skip to content

Stop a retried trial inheriting the failed attempt's classification - #1059

Open
charlesyhuang wants to merge 2 commits into
stagingfrom
fix/stale-analysis-on-trial-retry
Open

Stop a retried trial inheriting the failed attempt's classification#1059
charlesyhuang wants to merge 2 commits into
stagingfrom
fix/stale-analysis-on-trial-retry

Conversation

@charlesyhuang

@charlesyhuang charlesyhuang commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

A retried trial can end up permanently labelled HARNESS_ERROR while its
recorded reward is 1.0. Three independent defects produce that, and all three
are fixed here.

What goes wrong

Harbor mints a fresh task-<slug>__<id> job directory per attempt, and every
attempt re-uploads its wrapper into the same S3 prefix
(tasks/<task>/trials/<trial>/) without clearing it. So a retried trial's
artifact tree accumulates one job directory per attempt, side by side. Only the
last attempt overwrites the job-level result.json.

1. A re-attempt kept the previous attempt's QA verdict.
_prepare_trial_run clears every result field before an attempt runs — reward,
result, tokens, cost, trial_s3_key, phase_timing, has_trajectory — but
left analysis/analysis_status alone. QA is terminal-sticky
(_trial_needs_classification skips SUCCESS/FAILED, and the analysis
handler skips them again), so once an attempt was classified, no later pass
ever revisited the trial. An attempt killed by infrastructure would be
classified HARNESS_ERROR, the trial would then be retried and pass, and the
HARNESS_ERROR would ride along forever, describing artifacts that had already
been deleted from the row.

2. The classifier picked an arbitrary attempt's result.json.
It scanned trial_dir.iterdir() for the first task-* directory and stopped.
iterdir yields in filesystem order, so with several attempt directories
present the classifier analysed whichever one readdir happened to return first
— often a stale, failed attempt — even when the analysis ran after the
successful one.

3. A task that already closed out was never reopened.
maybe_start_qa_stage only fires from PENDING/RUNNING. When QA ran while an
attempt was failing, the task went COMPLETED — and stayed there while the
trial kept re-running underneath it. Even with the analysis cleared, no QA pass
would ever have been enqueued again. oraclecomp-evaldist-semantics-7d392cfa
shows this directly: finished_at = 08:28:37, with its trials finishing at
12:30 and 13:06.

The fix

The stale analysis is dropped during settlement, in _run_post_trial_hooks,
under the task and trial locks that function already holds — so the clear is
atomic with the maybe_start_qa_stage re-enqueue on the next line, and the
trial is terminal, meaning its artifacts are complete. Doing it at attempt
start instead would leave the trial unclassified for the length of an
attempt (hours), racing an in-flight QA job that snapshots its work list up
front. Any label such a job wrote from a mid-flight snapshot is discarded the
same way and re-derived from the artifacts the attempt actually produced.

Only an analysis that demonstrably predates the current attempt is dropped
(analysis_finished_at < started_at): a probe writes its own classification
during settlement, and that must survive.

The same block reopens a task that had already closed out, the way
append_trials reopens a finished task when live trials appear, so the QA
pass actually gets enqueued.

The classifier now consults the job-level result.json to identify the current
attempt. Its stats.evals[*].reward_stats / exception_stats name that
attempt's directory — the only signal in the tree that distinguishes it from
the stale siblings. When nothing is named (older layouts, partial uploads) the
scan falls back to sorted order, so the choice is at least deterministic
instead of readdir-dependent.

Nothing about upload or retention changes; stale attempt directories are still
uploaded and kept, they are just no longer mistaken for the current one.

Evidence

Four FVSmith runs (comppoly-fri,
pairwise-sum-triple-free-density-bound-wr, provenance-whole-default-target,
vcvio) were wedged at QA_PROBE, each showing 3/3 HARNESS_ERROR on sweeps
whose pass@1 was 1.0. Sample trials:

  • oraclecomp-evaldist-semantics-7d392cfa-79reward=1.0, 52/52 cases
    passed, attempts=5. Its analysis finished at 08:28 while the successful
    attempt ran 12:20–13:06, and reports an EnvironmentStartTimeoutError from
    an earlier attempt. (Defect 1.)
  • sum-free-triple-extremal-bound-47a9bc99-3reward=1.0, attempts=5,
    analysis ran at 14:25, after the 14:19 finish. The job-level result.json
    attributes reward 1.0 to task-sum-free-triple-extremal-bo__gnM73kh, but the
    analysis describes an idle-timeout crash with verifier_result: null from
    one of the four sibling attempt directories. (Defect 2.)

Trials already carrying a stale label are not rewritten by this change; they
need oddish backfill-analysis --task <id> --force.

Tests

tests/test_retry_clears_stale_analysis.py — three cases against a real
Postgres: the stale label is dropped and the closed task reopens; a probe's own
classification survives settlement; a first attempt is left alone.

tests/analyze/test_current_attempt_result.py — 8 cases, including an
adversarial iterdir order so the regression is pinned rather than dependent
on the host filesystem.

Each was watched fail against the pre-fix code.

Full suite run against a local Postgres, before and after: no new failures, no
new errors. The pre-existing failures are environmental (quota/cost tests that
need schema this setup does not create) and identical in both runs.


Note

Medium Risk
Changes QA gating and classification inputs for retried trials; incorrect clearing or dir selection could mis-label tasks or skip re-QA, but behavior is covered by new integration and unit tests.

Overview
Fixes retried trials staying labeled HARNESS_ERROR while reward=1.0, from three bugs addressed together.

Settlement (_run_post_trial_hooks): On retries (attempts > 1, non-probe), clears stored trial analysis so terminal-sticky QA can re-classify the current attempt. Clears at settlement (not attempt start) to avoid racing in-flight QA jobs. Reopens COMPLETED tasks (and verdict fields when run_analysis) so maybe_start_qa_stage can run again.

Classifier: Replaces “first task-* dir from iterdir” with _current_attempt_result, which reads job-level result.json reward_stats / exception_stats to pick the current attempt’s nested result.json, with sorted fallback when nothing is named.

Adds Postgres integration tests for stale-analysis clearing and unit tests for attempt-dir selection.

Reviewed by Cursor Bugbot for commit 4b77753. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
oddish-app Ready Ready Preview Aug 6, 2026 11:09pm

Comment thread oddish/src/oddish/workers/queue/trial_handler.py Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: medium. Left a non-blocking comment — Cursor Bugbot completed as skipped and reported an unresolved medium-severity finding (retry race with in-flight QA), so this is not approved. Human reviewers are already assigned; no additional reviewers requested.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Oddish preview

Commit: 4b77753d72a64d06bd36d3f9365578c86401e98a

Surface Link Target
Frontend https://pr-1059.oddish.app Vercel preview for 4b77753
Backend oddish-pr-1059 oddish-pr-1059
Database project msqcgymhlmjzhksduhwd project msqcgymhlmjzhksduhwd

Vercel deployment URL: https://oddish-m144mld41.oddish.app

Plan:

  • Frontend deploy: true
  • Backend deploy: true
  • Migrations: true

This comment is updated by the PR Preview workflow.

@charlesyhuang
charlesyhuang force-pushed the fix/stale-analysis-on-trial-retry branch from 1e1bffa to b5dde0f Compare August 3, 2026 23:57
@charlesyhuang

Copy link
Copy Markdown
Collaborator Author

Reworked for the Bugbot finding: the clear now happens during settlement (_run_post_trial_hooks), under the task and trial locks it already holds, so it is atomic with the maybe_start_qa_stage re-enqueue and the trial is terminal. Clearing at attempt start — the previous approach — left the trial unclassified for the length of an attempt and raced an in-flight QA job's up-front snapshot, exactly as flagged.

A label such a job wrote mid-attempt carries a newer stamp than the attempt's own start, so the guard keys on is_probe rather than timestamps; tests/test_retry_clears_stale_analysis.py::test_a_label_written_mid_attempt_is_dropped_too pins that, and fails against a timestamp-based guard.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b5dde0f. Configure here.

Comment thread oddish/src/oddish/workers/queue/trial_handler.py
…ication

A trial that fails an attempt, gets classified HARNESS_ERROR, then passes on
a later attempt kept the HARNESS_ERROR forever -- describing artifacts the
row no longer held. Three defects compound to produce that:

- Nothing invalidated the analysis when a trial re-ran. `_prepare_trial_run`
  clears every result field before an attempt (reward, result, tokens,
  trial_s3_key, ...) but the classification derived from them survived, and
  QA is terminal-sticky, so it could never be revisited.
- The task itself was never reopened. `maybe_start_qa_stage` only fires from
  PENDING/RUNNING, so a task that reached COMPLETED while an attempt was
  failing stayed closed through the attempt that passed, and no QA pass was
  ever enqueued again.
- The classifier picked the first `task-*` job dir from an unsorted
  `iterdir()`. Every attempt re-uploads into the same S3 prefix without
  clearing it, so a retried trial's tree holds one job dir per attempt and
  the classifier analysed an arbitrary one.

The analysis is now cleared during settlement, in `_run_post_trial_hooks`,
under the task and trial locks it already holds -- atomic with the
`maybe_start_qa_stage` re-enqueue on the next line, and with the trial
terminal so its artifacts are complete. Doing it at attempt start would
instead leave the trial unclassified for the length of an attempt, racing an
in-flight QA job that snapshots its work list up front. The same block
reopens a COMPLETED/FAILED task the way `append_trials` already does.

The classifier now resolves the current attempt from the job-level
`result.json`, whose `reward_stats`/`exception_stats` name that attempt's
directory, and falls back to sorted order so the choice is deterministic
rather than readdir-dependent.

Verified against four FVSmith runs wedged at QA_PROBE showing 3/3
HARNESS_ERROR on sweeps whose pass@1 was 1.0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@charlesyhuang
charlesyhuang force-pushed the fix/stale-analysis-on-trial-retry branch from b5dde0f to 58365e8 Compare August 4, 2026 00:05
@dot-agi
dot-agi changed the base branch from main to staging August 4, 2026 03:35
@dot-agi
dot-agi force-pushed the staging branch 2 times, most recently from bb5d394 to 9e715c4 Compare August 4, 2026 04:48
@dot-agi
dot-agi self-requested a review as a code owner August 4, 2026 23: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