Skip to content

Bound OpenCode history reads and repeated worker failures - #25292

Merged
nwparker merged 4 commits into
mainfrom
orca/opencode-scan-owned-budget
Oct 4, 2026
Merged

nwparker merged 4 commits into
mainfrom
orca/opencode-scan-owned-budget

Conversation

@nwparker

@nwparker nwparker commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​489 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​483
Prod 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​177 $\color{#cf222e}{\Huge{\mathbf{−}}}$​27 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​150

ELI5

A busy or repeatedly crashing OpenCode history reader can delay an AI Vault scan. Each scan now gets 45 seconds for its outstanding OpenCode work and stops replacing failed workers after three failures.

What Changed

Reuse the existing cancellation flow and shared worker queue. The allowance includes queue waiting and WSL preparation, and pauses between completed OpenCode operations. Reaching it records an OpenCode issue while retaining sessions already read and allowing other agents to finish.

Only failures of work that actually started count against that scan. Its own success resets its count; unrelated readers and later healthy scans keep working. Caller cancellation and worker retirement retain their existing behavior.

Why

The limit belongs to the scan requesting the work. A global timer or failure counter would also interrupt unrelated readers.

Linked Issue

Continues #10708 and credits @AmethystLiang for the original contribution. Its broader unavailable-reader, cooldown, WSL, and complete-history concerns remain open.

Visual Proof

N/A — background history-reader behavior. Verification retains installed-CLI responses, scan results, worker events, and cleanup evidence.

Testing

  • Installed OpenCode 2.0.16 read four completed sessions from a fresh private profile before the scanner exercises.
  • Automated tests added: 87 tests across 12 files passed, with all three typechecks and owned lint, format, and quality gates.
  • The integration with main at a5b8b7e2 passes all three required typechecks. All 200 scanner source inputs, nine dependencies, and two controlled harness inputs match the runtime-verified compilation closure.
Control Before After
Ordinary history read Four sessions, no issues Four sessions, no issues
Waiting behind two held reads Returned after 50.049 seconds Scoped issue after 45.003 seconds
Failures across 32 candidates Created 12 workers Stopped after three faulted workers
Later independent scan Existing behavior retained Four sessions, no issues

Queued and active cancellation still propagate. The two unrelated readers returned four candidates each after the AFTER scan deadline. Earlier outside-reader fixtures used invalid filenames; their zero-candidate results are retained separately.

The 25-second holds, 28 added sessions, and forced exits are test amplification on real installed-session data. The final suite passed once with zero retries; all seven child processes and eight worker threads closed, and the private listener was confirmed closed. This is macOS evidence, with no live Windows, WSL, SSH, natural-crash, or whole-Vault wallclock claim.

Review

Five production files and five tests change. The installed verification remains pinned to source db6ca590; integration 5d3e7c99 preserves all ten files and all unrelated main changes. Source and evidence were independently reviewed. Follow-up 8be861f5 derives the deadline message from the existing budget; the emitted 45-second message is unchanged, and all six scope tests, lint, and format pass.

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer implementation was copied.

Notes

Original #10708 stays open for its remaining scope. Earlier harness failures are preserved separately.

Checklist

  • Small, focused change with the user-visible behavior, mechanism, and reuse choice explained.
  • Before/after evidence provided, with N/A for visual proof explained.
  • Self-reviewed for correctness, security, and performance.
  • Cross-platform, SSH execution boundaries, and cancellation considered.
  • Relevant tests, typechecks, lint, and format pass. CI will cover the full build.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 63dfdfc6-1b8c-4e6f-a853-1d3471094c80
📥 Commits

Reviewing files that changed from the base of the PR and between 5d3e7c9 and 8be861f.

📒 Files selected for processing (1)
  • src/main/ai-vault/session-scanner-opencode-sqlite-scan-scope.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/ai-vault/session-scanner-opencode-sqlite-scan-scope.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The changes add a 45-second active-work budget to OpenCode SQLite scans and route worker-client and WSL requests through that scope. The scope excludes zcode and native-chat requests. The worker queue tracks crash-loop failures by request owner and refuses calls for an owner that reaches the failure limit. Tests cover budget expiration, cancellation, retries, and owner-specific failure handling.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 8be86

This change adds a 45-second work budget for OpenCode scans and per-owner worker failure limits. No merge-blocking issue was found in the supplied review context.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 5d3e7

The change improves containment of slow or repeatedly failing history reads. No new access or privilege expansion was identified in the reviewed paths. Partial results, cancellation, and recovery remain the main risks, and coverage is limited.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — If a history producer can supply pathological database contents, the relevant reviewed outcome is delayed, failed, or omitted history within the application's existing native or WSL reader paths. The changes inspected add containment rather than database access authority. Broader caller reachability and maximum deployment exposure remain outside the established evidence.

Trust Boundaries and Controls

  • observed — Requests store their owner before queueing, and responses must match the active request ID. Failure accounting uses the active request's owner rather than attributing a worker fault to every queued caller. Retired worker listeners are removed before termination, limiting stale-event attribution.

Resilience and Maintainability Implications

  • observed — A successful owned request resets only that owner's failure count. At the configured failure threshold, the queue refuses that owner and drains only its queued calls, preserving peer owners for subsequent dispatch. Cancellation and disposal settle affected requests and clear their listeners and timers.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 10 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main changes: limiting OpenCode history reads and repeated worker failures.
Description check ✅ Passed The description covers the required sections, explains the change and rationale, links the work to issue #10708, and gives detailed testing evidence. Visual proof is marked N/A with a reason. The AI D…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — the mechanism holds up under the tests I ran; two minor consistency nits below.

Reviewed changes

  • Per-scan OpenCode work budget (session-scanner-opencode-sqlite-scan-scope.ts): an AsyncLocalStorage-scoped owner gives each scanAiVaultSessions call a 45s clock that is spent only while OpenCode SQLite work is outstanding (paused between legs, and counting queue wait and WSL preparation), aborting the scan's OpenCode signal when exceeded.
  • Per-owner worker failure budget (worker-thread-request-queue.ts): worker deaths are now tracked per WorkerThreadRequestOwner, so a crash loop drains and refuses only the failing scan's queued calls while unrelated readers and later scans keep working.
  • Call-site wiring (session-scanner.ts, ...-worker-client.ts, ...-worker-spawn.ts): the scope is entered at the top of the scan; OpenCode list/parse/capture register with it, while zcode/native-chat reads are explicitly left unscoped.
  • Tests: five files cover budget pausing/resume, owner isolation, WSL preparation, deadline-incomplete capture, and cancellation.

ℹ️ Nitpicks

  • session-scanner-opencode-sqlite-worker-client.ts:30-34 — the MAX_CONSECUTIVE_DEATHS comment still describes the pre-change reset rules ("after draining", "when a fresh scan burst starts from idle"). With per-owner budgets an owner's cap is enforced by its own counter and a crash loop now sets refused instead of resetting; the "per-scan, not process-wide" intent still holds, but the mechanism it names is stale.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Derived the OpenCode deadline message from its budget (session-scanner-opencode-sqlite-scan-scope.ts): the deadline error now builds `${OPENCODE_SQLITE_SCAN_BUDGET_MS / 1000}s` instead of a hardcoded 45s, so the message cannot drift from the budget constant. The emitted text is unchanged (the constant is still 45 000), and the existing stringContaining('45s work budget') assertions still pass.

This addresses the inline nit from the prior review, whose thread is now resolved. The delta since 5d3e7c9 is this single message change; the rest of the PR is unchanged and was covered by the prior review.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit f0b5b85 into main Oct 4, 2026
33 checks passed
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