Skip to content

fix(ollama): yield during dense stream processing#86633

Open
udaymanish6 wants to merge 1 commit into
openclaw:mainfrom
udaymanish6:fix/86599-local-provider-event-loop
Open

fix(ollama): yield during dense stream processing#86633
udaymanish6 wants to merge 1 commit into
openclaw:mainfrom
udaymanish6:fix/86599-local-provider-event-loop

Conversation

@udaymanish6
Copy link
Copy Markdown
Contributor

@udaymanish6 udaymanish6 commented May 25, 2026

Summary

  • add cooperative yields while processing dense Ollama NDJSON stream chunks
  • preserve abort checks inside the stream loop
  • add a regression test covering event-loop yielding under a dense chunk burst

Real behavior proof

Behavior or issue addressed: Ollama stream chunk processing was monopolizing the event loop on dense native streams.
Real environment tested: local OpenClaw checkout in /Users/miya/Desktop/openclaw-issue-86599.
Exact steps or command run after this patch: pnpm test extensions/ollama/src/stream.test.ts
Evidence after fix:

Test Files  1 passed (1)
Tests  9 passed (9)
Start at  17:03:09
Duration  2.02s

Observed result after fix: the dense-stream regression test passed after the cooperative-yield change.
What was not tested: I did not reproduce the Windows beta gateway loop on a live Windows beta machine in this environment.

Verification

  • pnpm test extensions/ollama/src/stream.test.ts

@openclaw-barnacle openclaw-barnacle Bot added extensions: ollama size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 25, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 25, 2026

Codex review: needs real behavior proof before merge. Reviewed May 27, 2026, 8:09 AM ET / 12:09 UTC.

Summary
The PR adds a cooperative scheduler to the Ollama native stream loop and a Vitest regression test that asserts timers fire during dense mocked NDJSON bursts.

PR surface: Source +41, Tests +41. Total +82 across 2 files.

Reproducibility: no. not for the live end-to-end issue on this PR branch. Source inspection and maintainer probes support the dense Ollama stream path, but no live OpenClaw/Ollama or Windows gateway reproduction after the patch is supplied.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Add redacted real behavior proof from a patched run against local Ollama or the Windows gateway scenario, showing improved responsiveness after the change.
  • Clarify in the PR body that this is a native-Ollama mitigation and that the broader local-provider/full-partial stream work remains tracked separately.
  • Get the failing GitHub check-runs green or explain any unrelated failure with maintainer-visible proof.

Proof guidance:
Needs real behavior proof before merge: The PR body shows only a mocked Vitest run; before merge, add redacted live Ollama or Windows/local-provider output, logs, terminal screenshot, copied live output, linked artifact, or recording after this patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • The contributor proof is still only a mocked Vitest run; live Ollama, Windows, gateway responsiveness, redacted logs, terminal output, or a recording is still needed before merge.
  • The related gateway-starvation report spans llama.cpp/OpenAI-compatible routing and repeated full-partial stream work, so this PR should be scoped as native-Ollama mitigation rather than the complete fix.
  • The reviewed head currently has failing GitHub check-runs with generic annotations, so maintainers need normal CI log review or a rerun before landing.

Maintainer options:

  1. Decide the mitigation before merge
    Land this as a narrow native-Ollama mitigation only after redacted real local-provider proof, while keeping the broader local-provider starvation work tracked separately.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
The remaining action is maintainer review of real behavior proof, CI state, and scope against the broader linked starvation issue, not an automated code repair.

Security
Cleared: The diff only changes Ollama stream scheduling and a mocked regression test; it adds no dependency, workflow, secret, install, or package-resolution surface.

Review details

Best possible solution:

Land this as a narrow native-Ollama mitigation only after redacted real local-provider proof, while keeping the broader local-provider starvation work tracked separately.

Do we have a high-confidence way to reproduce the issue?

No, not for the live end-to-end issue on this PR branch. Source inspection and maintainer probes support the dense Ollama stream path, but no live OpenClaw/Ollama or Windows gateway reproduction after the patch is supplied.

Is this the best way to solve the issue?

Unclear but plausible. Cooperative yielding is a narrow maintainable mitigation for dense native Ollama chunks, but the discussion shows the broader root cause also includes repeated full-partial stream work across sibling paths.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 7efbaf7dba13.

Label changes

Label justifications:

  • P1: The PR targets a recent local-provider event-loop starvation path that can make gateway operations and model calls effectively unusable for affected users.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body shows only a mocked Vitest run; before merge, add redacted live Ollama or Windows/local-provider output, logs, terminal screenshot, copied live output, linked artifact, or recording after this patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +41, Tests +41. Total +82 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 41 0 +41
Tests 1 41 0 +41
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 82 0 +82

What I checked:

  • Repository policy read: Root AGENTS.md and the scoped extensions guide were read; the applicable guidance is to review plugin runtime changes against the bundled plugin boundary and require real behavior proof for user-visible bug-fix PRs. (AGENTS.md:1, 7efbaf7dba13)
  • Current main stream loop: Current main parses dense Ollama NDJSON records and processes each yielded chunk without a cooperative macrotask yield in the loop body. (extensions/ollama/src/stream.ts:1290, 7efbaf7dba13)
  • PR implementation: The PR adds event-count/time-threshold scheduling, abort checks, and an awaited scheduler call after non-final Ollama stream chunks. (extensions/ollama/src/stream.ts:53, e40e6a473100)
  • Mock-only proof: The added regression test uses a mocked fetchWithSsrFGuard and an in-memory Response body, so the PR body's test output proves the unit path but not a live Ollama, gateway, or Windows run. (extensions/ollama/src/stream.test.ts:3, e40e6a473100)
  • Maintainer analysis in discussion: A maintainer comment reports focused probes showing dense Ollama-style bursts benefit from yielding, while also explaining that repeated full-partial stream work remains a broader root cause across sibling paths.
  • Sibling stream precedent: The OpenAI-compatible stream path already has a cooperative scheduler and awaits it inside stream loops, supporting the design direction while leaving sibling repeated-work cleanup separate. (src/agents/openai-transport-stream.ts:142, 7efbaf7dba13)

Likely related people:

  • steipete: GitHub history shows several recent Ollama stream and native-provider fixes by this contributor, including usage estimation, native thinking, Kimi garbled-output handling, and a broad coercion refactor touching provider paths. (role: recent area contributor; confidence: high; commits: 5d5c37775e1d, 732e5805e3f8, 1b13f53047ee; files: extensions/ollama/src/stream.ts, extensions/ollama/src/stream.test.ts)
  • vincentkoc: The latest GitHub history for the Ollama stream file includes a recent native Ollama greedy top_p fix, and local blame in the shallow checkout attributes the current stream-loop lines to a recent imported state under this name. (role: recent area contributor; confidence: medium; commits: dfadc7b704d2, 6c42fea2d8; files: extensions/ollama/src/stream.ts)
  • osolmaz: The PR discussion includes this maintainer's detailed analysis of the dense-stream, diagnostic serialization, subscriber, and sibling-provider surfaces, and the related gateway-starvation issue is assigned to this person. (role: root-cause analyst and related issue owner; confidence: medium; files: extensions/ollama/src/stream.ts, src/agents/openai-transport-stream.ts, src/agents/pi-embedded-runner/run/attempt.model-diagnostic-events.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. labels May 25, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 25, 2026

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@osolmaz
Copy link
Copy Markdown
Member

osolmaz commented May 28, 2026

Related: #86599.

This PR addresses one contributor from that issue: native Ollama can deliver dense stream chunks, and the Ollama parser can process many of them before the gateway gets a chance to handle other work.

I would scope this as a related but partial mitigation. It may improve the native-Ollama dense-stream path, but #86599 is broader unless we also prove the gateway stays responsive during the original repro and address the repeated full-partial stream work discussed there.

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

Labels

extensions: ollama P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants