Skip to content

fix(compile): isolate account locks and stop stalled review loops - #6133

Open
fujiajie666 wants to merge 7 commits into
mainfrom
fix/compile-execution-bounds
Open

fujiajie666 wants to merge 7 commits into
mainfrom
fix/compile-execution-bounds

Conversation

@fujiajie666

Copy link
Copy Markdown
Collaborator

Description

Compile tasks from different accounts can block each other when they use the same target URI. Forced thinking, timeout-triggered pipeline fallback, and repeated reader calls can also cause excessive model usage.

This PR isolates target locks by account, stops reader loops that make no progress, and makes Compile respect the configured model settings and retry policy.

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Fixes #6081

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 not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • Key target locks by effective account and target URI, preserving serialization for tasks writing to the same account and target.
  • Detect repeated reader calls with unchanged results and bound repeated reader failures, while allowing reads that continue making progress.
  • Keep stage review failures advisory, preserve the existing plan, and propagate cancellation.
  • Inherit model thinking, reasoning effort, and request timeout settings instead of imposing separate Compile overrides.
  • Propagate direct-generation model failures instead of automatically falling back to the pipeline; retain fallback for oversized inputs and invalid or truncated output.
  • Remove unbounded adapter retry loops, respect the configured retry budget, and prevent stream replay after output becomes visible.
  • Update English and Chinese configuration documentation and add regression coverage.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

50 targeted tests passed on Python 3.12:

  • bot/tests/test_compile_execution.py
  • bot/tests/test_bot_vlm_credentials.py
  • bot/tests/test_bot_vlm_streaming.py
  • tests/unit/test_vikingbot_vlm_adapter_retry.py

Ruff lint, Ruff format checks, and git diff --check passed for the changed files.

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

N/A.

Additional Notes

N/A

@ZaynJarvis ZaynJarvis 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.

Thanks for picking up #6081. The lock fix is right and I verified it live, but two of the boundedness goals aren't met yet and the failure semantics regressed, so requesting changes.

Verified (isolated instance from this head, vlm.thinking:false, timeout:90)

  • Lock key (account_id, canonical target) (service.py:669-673): two accounts compiling to the same target now run concurrently (15.5s / 11.4s; on main the second sat queued until 50.5s). Same account + same target still serializes correctly. Lock is released on failure and on cancel at every stage.
  • Review stage: the "same read ~130 times" pattern from #6081 is now stopped by the no-progress check (8 rounds, review rejected as advisory, task finished in 95s vs 732s before).

Blocking

  1. Read rounds are still unbounded, and the wall-clock budgets were removed. seen_reads (pipeline_io.py:482-501) only catches byte-identical (name, args, result). Reads that change offset/limit slightly count as progress forever: a repro ran 300 rounds with varying offsets and 200 rounds with limit decreasing by 1 without being stopped. At the same time pipeline_call/plan/agent_seconds were deleted (models.py:40-43), so a single call is now bounded only by (1+max_retries) × vlm.timeout (≈40 min with defaults) and plan/review have no total budget. Please add a per-call read-round cap (e.g. 8–16) and keep a stage-level wall-clock budget as a backstop.
  2. Compile-stage failures no longer fall back to the pipeline. entry.py:59-63 now re-raises every ModelCallError except INPUT_TOO_LARGE. On main, a single-call timeout or a stalled read loop returned None and fell back to the multi-step pipeline; on this head the whole task fails. A single legitimate re-read also hard-fails the task instead of degrading. Suggest restoring the fallback for timeout/no-progress (or making it configurable), and answering a duplicate read with an "unchanged" tool message, failing only after N consecutive repeats.
  3. 4 existing tests break: bot/tests/test_bot_provider_thinking.py::test_vlm_adapter_volcengine_stream_respects_optional_max_tokens (4 cases) fail because the fake stream is an async generator without __aenter__; they pass on main. CI doesn't run bot/tests, so this wasn't caught.

Non-blocking

  • 429 handling went from unbounded exponential backoff (5s→120s) to 3 retries / ~4–5s total (vlm_adapter.py:296-306). Under per-minute TPM limits that fails compile and bot chat much sooner; consider a longer budget for rate limits or honoring Retry-After.
  • With an explicit bot model (AgentsConfig.thinking defaults to True), map/route/reduce now all run with thinking on — previously explicitly False. Your own test asserts this; please confirm it's intended (cost/latency).
  • volcengine_vlm.py:250-251 makes the async path send the configured reasoning_effort server-wide, but sync get_completion wasn't changed — inconsistent, and broader than compile.
  • Unrelated docs change in sessions.md (dropped POST).
  • connection["account_id"] → .get(...) defensively.
  • Optional from the issue: surface "waiting for target lock" in task status.

Tests: the 4 test files in this PR pass (50 cases). bot/tests overall: 438 passed, 5 failed — 4 above introduced here, 1 (test_bot_model_timeout) also fails on main.

@fujiajie666
fujiajie666 force-pushed the fix/compile-execution-bounds branch from 4f46a44 to e14174c Compare October 16, 2026 08:53
@fujiajie666

Copy link
Copy Markdown
Collaborator Author

@ZaynJarvis Updated the branch and rebased onto main. Reader repetition is now based on the tool and content, excluding range metadata. Duplicate reads get an "unchanged" message and stop after three consecutive rounds without new content. Calls and execution assignments use the configured timeout; plan/review get 2x. Timeout/no-progress fallback and the four streaming tests are fixed.

Map/route/reduce thinking, sync reasoning_effort, and account ID validation are fixed too. The unrelated sessions docs diff is gone.

The optional lock-wait status is out of scope here. 429 handling now uses the shared retry policy; longer backoff and Retry-After support need a fix in that shared policy and are outside this PR.

Docs CI currently fails on missing interrupt/steer API references introduced by #6115 on main. The docs build and all other docs checks pass locally.

Could you take another look?

@ZaynJarvis ZaynJarvis 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.

Thanks for the update. Most of the last round is fixed at e14174c8: content-keyed repeat detection, "Unchanged" + 3-round stop, fallback on timeout/no-progress, thinking off for map/route/reduce, sync reasoning_effort, account-scoped locks, and the streaming tests all check out. 245 related tests pass locally, and Docs CI only fails because of the steer/interrupt refs from #6115 (main fails the same check).

Remaining issues, all reproducible against e14174c8:

1. Execution assignments get only 1× the call timeout (blocking)
pipeline_io.py:363 caps an execution assignment at 1× the configured timeout, and the agent job runs inside that same budget (pipeline_io.py:416). Plan/review get 2×.

  • Repro: timeout=1, each model call takes 0.6s, one read then emit. map fails at 1.00s with assignment wall-clock budget exhausted. review passes the same script because it has 2×.
  • Impact: with the default 600s, a 70-round agent transform now gets 600s instead of 1200s on main. At 60–90s timeouts, map/reduce/agent jobs time out almost every time, and reduce.py:318-323 raises instead of degrading, so the whole compile fails.
  • Suggest a separate assignment budget, or at least 2× for execution assignments too, plus a test that fills a real multi-round assignment.

2. No per-call cap on reader rounds
Any read with different content counts as progress, so a model that walks offsets never stops before the wall clock.

  • Repro: offset +1 each round → review ran 405 rounds in 4s. limit −1 each round → map ran 282 rounds in 2s.
  • Suggest a hard cap of 8–16 reader rounds per call, raising ModelCallLimitError so the fallback applies.

3. Reader failures raise plain ModelCallError, not a limit error
pipeline_io.py:520-523: the 4th reader failure raises ModelCallError, which entry.py:61 does not treat as fallback-eligible, so the compile job fails outright. The count is cumulative, not consecutive. Repeated reader failure is a no-progress case and should raise ModelCallLimitError.

4. Repeat key ignores the path (minor)
Two different paths with identical content (e.g. two empty files) are treated as "Unchanged" and stop after 3 rounds. Include the normalized path in the key.

Repro scripts (pure unit-level, mocked provider) are available if useful.

@fujiajie666

Copy link
Copy Markdown
Collaborator Author

@ZaynJarvis Fixed the timeout budgets and reader-failure handling. Calls and execution assignments now use max(600, timeout) seconds; plan/review use max(1200, 2 * timeout). Successful reads reset that reader's failure counter, and exhausted retries raise ModelCallLimitError so direct compile can fall back to the pipeline.

We consider identical-content reads across different resources and changing-content read loops to be low-probability cases, so we're leaving both out of this PR. The wall-clock budget still bounds the latter.

Could you take another look?

@fujiajie666
fujiajie666 force-pushed the fix/compile-execution-bounds branch from 4115e1e to 6a30bd3 Compare October 17, 2026 03:24

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

Compile: target lock ignores account, and a review loop with no call limit can keep one task running 10+ minutes and exhaust model TPM

2 participants