Skip to content

Conversation

@chrarnoldus
Copy link
Contributor

@chrarnoldus chrarnoldus commented Jul 23, 2025

Kilo Code PR: Kilo-Org/kilocode#1447

Description

Roo Code often underreports the total cost of a task, because the cost of some requests are missing. For example:

CleanShot 2025-07-23 at 14 03 33 CleanShot 2025-07-23 at 14 03 51

The first $0.00677 request is missing its cost in the UI, while requests 2-4 are not. Also the total cost (should be $0.0816) is off.

This is because a request is abandoned before the usage block has been processed when an LLM tries to use multiple tools (only one is allowed in the rules):

if (this.didAlreadyUseTool) {

(this code is inherited from Cline)

Test Procedure

Run tasks with a variety of providers and models and verify all requests have a cost reported. Kimi K2 seems to be especially affected. Sonnet 4 sometimes, Gemini 2.5 Pro not so much.

I did test this with OpenRouter and costs are reported more reliably than before. Also tested with Anthropic and Ollama for regressions.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

In this screenshot all costs are reported and the tally is correct ($0.0591):

CleanShot 2025-07-23 at 16 49 46 CleanShot 2025-07-23 at 17 01 30

Additional Notes

I'll add some comments to the code.

Get in Touch

Christiaan in shared Slack


Important

Fixes underreporting of token usage and cost in Task.ts by processing all request chunks and updating usage data even if requests are interrupted.

  • Behavior:
    • Fixes underreporting of token usage and cost in Task class in Task.ts by ensuring all request chunks are processed.
    • Adds drainStreamInBackgroundToFindAllUsage() to process remaining chunks and update usage data even if a request is interrupted.
  • Functions:
    • Modifies recursivelyMakeClineRequests() to handle incomplete requests by processing all chunks in the background.
    • Updates updateApiReqMsg() to include all token and cost data.
  • Misc:
    • Removes redundant TelemetryService call for capturing LLM completion in the main loop, now handled in the background task.

This description was created by Ellipsis for 60f0a03. You can customize this summary. It will automatically update as commits are pushed.

@chrarnoldus chrarnoldus requested review from cte, jr and mrubens as code owners July 23, 2025 15:07
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jul 23, 2025
// PREV: We need to let the request finish for openrouter to
// get generation details.
// UPDATE: It's better UX to interrupt the request at the
// cost of the API cost not being retrieved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment calls out OpenRouter specifically, but I think if seen usage being misreported with other providers as well.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 23, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jul 24, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 24, 2025
- Add error handling in drainStreamInBackgroundToFindAllUsage with try-catch
- Add 30-second timeout to prevent potential memory leaks from hanging streams
- Include model ID in console warning for better debugging
- Fix race condition by moving updateApiReqMsg() call inside background task
- Ensure saveClineMessages() is called after usage data is updated
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 25, 2025
- Handle unhandled promise rejection by adding .catch() to background task
- Fix race condition by using local variables in background task before atomically updating shared state
- Eliminate duplicate telemetry capture logic with captureUsageData helper function
- Make usage collection timeout configurable via CLINE_USAGE_COLLECTION_TIMEOUT env var (default: 30s)
- Improve code organization and readability
- Include model ID in all warning messages for better debugging
…ce condition fixes

- Add proper error handling for background promise with .catch()
- Fix race condition by using local variables in background task
- Make timeout configurable via DEFAULT_USAGE_COLLECTION_TIMEOUT_MS constant
- Extract captureUsageData helper for better code organization
- Ensure model ID is included in warning messages
- Update comments to reflect the actual implementation
@daniel-lxs
Copy link
Member

I've pushed some improvements to address the review comments:

  • Added proper error handling for the background promise
  • Fixed the race condition by using local variables in the background task
  • Made the timeout configurable via a constant in global-settings.ts
  • Improved code organization

One concern I noticed: with this implementation, when a user cancels a request, the background stream continues running to collect usage data. This differs from the original behavior where streams would be immediately terminated.

Should we keep this new behavior for better usage tracking, or would it be better to add a check to stop the background task immediately when this.abort is true? The OpenRouter docs mention stream cancellation best practices here: https://openrouter.ai/docs/api-reference/streaming#stream-cancellation

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Jul 27, 2025
@chrarnoldus
Copy link
Contributor Author

chrarnoldus commented Jul 27, 2025

Should we keep this new behavior for better usage tracking, or would it be better to add a check to stop the background task immediately when this.abort is true?

If it is decided to abort the stream I do feel there should be a visual indication that the cost figure is no longer reliable. Same goes for the timeout. (I can make a PR for this.)

@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Review] in Roo Code Roadmap Jul 27, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Review] to PR [Needs Prelim Review] in Roo Code Roadmap Jul 27, 2025
…dToFindAllUsage

- Merged if (usageFound) and else if blocks that both called captureUsageData with identical parameters into a single condition
- Eliminates code duplication and improves maintainability
@daniel-lxs
Copy link
Member

daniel-lxs commented Jul 28, 2025

@chrarnoldus
Thank you for the review! I believe we should keep the old functionality of aborting the stream for now and in a future PR we can add a visual indicator, the main concern is that this would cause the API costs to some users just to report the correct cost data.

Let me know what you think!

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jul 28, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Review] to PR [Changes Requested] in Roo Code Roadmap Jul 28, 2025
@chrarnoldus
Copy link
Contributor Author

Ok, it's just that not every provider supports cancellation so costs could rack up anyway.

@chrarnoldus
Copy link
Contributor Author

@daniel-lxs can you consider taking this if we make the timeout very small? Then it shouldn't impact cost much. I can make a PR for the visual indicator afterwards.

@daniel-lxs
Copy link
Member

@chrarnoldus I can give this the green light and let the team decide if this is something they want, can you take a look at the failing CI workflow?

@chrarnoldus
Copy link
Contributor Author

@daniel-lxs I fixed the initial failure, I don't think the new one is related to the changes in this PR.

@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Review] in Roo Code Roadmap Aug 11, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 11, 2025
@mrubens mrubens merged commit b30372d into RooCodeInc:main Aug 11, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 11, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Aug 11, 2025
mrubens added a commit that referenced this pull request Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Changes Requested size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants