Skip to content

Finder timeout is a recipe constant calibrated per-model — a model change silently truncates productive finders (p95 censored at the cap, complete:false verdicts) #93

Description

@mabry1985

Summary

The panel's per-finder timeout is a constant in workflows/code-review-structural.yaml, and the recipe's own comment explains it was calibrated against a specific model's latency distribution:

900s is deliberately conservative — just above both slow finders' p95 (crossfile 841s, correctness 774s) — so it rescues the exhaustion/hang tail while rarely touching a productive run; tune down as the eval harness's labeled set grows.

That reasoning is sound, and the number was right for the lane it was measured on. The problem is that the number is baked into the recipe while the model is operator-configurable, so changing model.name silently invalidates the calibration — and the failure is quiet, because a timed-out finder degrades to an empty Gap rather than erroring.

What we measured

Moving this deployment from protolabs/cloud to protolabs/smart (same panel, same repos, same day) shifted the distribution right:

step durations on protolabs/smart, n=17 reviews

  find_crossfile         p50  454s   p95  900s   max  900s   ← CENSORED at the cap
  find_correctness       p50  508s   p95  746s   max  746s
  find_conventions       p50  338s   p95  640s   max  640s
  find_removed_behavior  p50  301s   p95  553s   max  553s

steps cut off at the 900s cap: 2

find_crossfile's p95 is not 900s — it is at least 900s and unknowable, because the cap censors it. Two finder steps were cut mid-work, and the review carrying one of them posted complete: false with a FAIL built on four angles instead of five.

That last part is the reason this is worth filing rather than just tuning locally. The reviewer's own persona doc says:

The 80% bar: … a starved or partial review is never a verdict.

A finder cut off at its budget produces exactly a starved review, and the panel posts a verdict anyway. The degrade-to-Gap behaviour is correct and graceful — it is the right thing to do when a finder genuinely hangs. It is the wrong thing to do when the finder was working fine and simply needed more wall-clock than a constant calibrated for a different model.

Suggested fix

Make the finder budget resolvable from config, with the recipe constant as the default. Something like a pr_reviewer.finder_timeout_s knob that the dispatcher substitutes into the recipe's step timeouts at prepare time, or more generally a workflow-engine-level step_timeout_default that a recipe's own timeout can still override. Either shape means an operator who changes the model can re-calibrate the budget without shadowing the recipe.

Two smaller things worth pairing with it:

  1. Emit a distinguishable telemetry signal when a step is cut by its budget — today the review carries complete: false and the step's duration sits suspiciously at exactly the cap, but nothing says "this was truncated". Grouping on "step duration ≈ timeout" is how I found this, which is not a diagnostic anyone should have to invent. A step_timeout event (or a truncated_steps list beside degraded_steps) would make it greppable.
  2. Consider whether a truncated finder should block the verdict rather than degrade to a Gap. Arguably a panel that lost an angle to the clock should reaffirm-and-retry or escalate rather than post, given the 80% bar. That is a judgement call for the maintainers, not an obvious bug — but the current behaviour means the quietest possible failure produces a posted, promotable verdict.

Workaround in use

Dropping a copy of the recipe into the workflows plugin's writable dir (/sandbox/workflows/) shadows the bundled one, since later dirs win on a name clash. That works and survives container rolls — but it freezes the entire recipe, so any future improvement to the bundled version is silently hidden. Two notes for anyone else doing this:

  • The registry cache only rebuilds when the directory set changes, not when a file inside a directory changes — so a recipe edit needs a process restart. A POST /api/config reload is not sufficient.
  • Record the bundled file's hash in the override so the drift is at least detectable after a plugin bump.

A config knob would make the workaround unnecessary, which is the ask.

Activity

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions