Skip to content

fix(opencode): separate per-file and overall timeouts - #717

Open
Linxiushen wants to merge 1 commit into
alibaba:mainfrom
Linxiushen:fix/opencode-configured-timeout
Open

fix(opencode): separate per-file and overall timeouts#717
Linxiushen wants to merge 1 commit into
alibaba:mainfrom
Linxiushen:fix/opencode-configured-timeout

Conversation

@Linxiushen

@Linxiushen Linxiushen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

timeoutMinutes is a per-file OCR setting, while the plugin watchdog covers the complete multi-file process. Treating them as one value can terminate valid work, especially across multiple files.

This change keeps the two policies independent:

  • timeoutMinutes is passed only to OCR's per-file --timeout flag;
  • a new optional overallTimeoutMinutes controls the enclosing process watchdog;
  • review runs have no implicit process-wide deadline when that option is omitted, while OpenCode cancellation still terminates the process tree;
  • health probes retain their explicit 30/60-second limits;
  • the branch is rebased on current main, including the newer process-group termination behavior.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

Tested on Windows with Node.js 24:

  • tsc -p tsconfig.json --noEmit
  • node --test test/*.test.mjs (21 passed, 1 platform-specific skip)
  • git diff --check

Checklist

  • My code follows the project's existing TypeScript style
  • I have performed a self-review of my code
  • I have added tests that prove the timeout policies are independent
  • New and existing plugin unit tests pass locally with my changes
  • I have signed the CLA

Related Issues

Addresses the confirmed H1 finding in #702.

AI Assistance

AI-assisted tooling was used during implementation. I reviewed the final diff and ran the typecheck and complete plugin test suite locally.

@CLAassistant

CLAassistant commented Aug 4, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s).

@ethan-parkk

Copy link
Copy Markdown
Contributor

timeoutMinutes is documented as a per-file timeout, but runOcr.timeoutMs limits the entire review process.
Setting them equal can terminate valid multi-file reviews, and it shortens the previous 15- minute watchdog when timeoutMinutes is smaller.
Could we keep these limits separate or define an explicit overall timeout policy?

@Linxiushen
Linxiushen force-pushed the fix/opencode-configured-timeout branch from 6f2bbbd to 5b33909 Compare August 5, 2026 14:59
@Linxiushen Linxiushen changed the title fix(opencode): honor configured review timeout fix(opencode): separate per-file and overall timeouts Aug 5, 2026
@Linxiushen

Copy link
Copy Markdown
Contributor Author

Good catch. I rebased onto current main and changed the policy in 5b33909 rather than coupling the two values.

timeoutMinutes now remains strictly per-file and is only forwarded as ocr --timeout. A separate optional overallTimeoutMinutes controls the enclosing process watchdog; when it is omitted, review execution has no implicit wall-clock deadline, so valid multi-file runs are not cut off. OpenCode cancellation still terminates the process tree, and health probes keep explicit short deadlines.

The regression exercises timeoutMinutes: 30 with overallTimeoutMinutes: 45 and observes a 45-minute watchdog, while a separate test verifies that omitting the overall option schedules no process-wide timer. Typecheck passes; plugin suite: 21 passed, 1 platform-specific skip.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Nice fix — separating per-file and overall timeouts is the right call. One suggestion:

Consider a generous default overall timeout instead of no timeout.

The original 15-minute default was clearly too short for multi-file reviews, but swinging to "no implicit timeout" removes the safety net entirely. If the abort signal never fires (e.g., OpenCode itself crashes or the host process dies), the OCR process will hang indefinitely.

A default of ~60 minutes would be a reasonable middle ground:

  • Long enough that no legitimate review gets killed
  • Short enough to catch genuinely stuck processes
  • Users can still override via overallTimeoutMinutes

Something like:

const defaultOverallMs = 60 * 60 * 1000
const options: RunOptions = {
  cwd,
  signal: context.abort,
  timeoutMs: input.overallTimeoutMinutes !== undefined
    ? input.overallTimeoutMinutes * 60 * 1000
    : defaultOverallMs,
}

Defense in depth — the safety net should exist but stay out of the way.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consider a generous default overall timeout instead of no timeout. @Linxiushen

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.

4 participants