fix: ensure usage records are published for 0-token responses#2272
fix: ensure usage records are published for 0-token responses#2272LucasInsight wants to merge 1 commit intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where usage records were not being published for API responses that returned zero tokens. It resolves this by adding a deferred call to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4041909029
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
In Execute, this defer order is backwards for Go's LIFO semantics: ensurePublished runs first, calls once.Do, and then trackFailure cannot publish a failed record anymore. That means any error path that returns before an explicit usage publish (for example translateRequest failures in this function) is now recorded as a successful zero-usage request, which corrupts failure accounting. The same pattern appears in the other executors changed in this commit.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where usage records are not published for 0-token responses by adding defer reporter.ensurePublished(ctx) to several executors. While this change correctly identifies the need for ensurePublished, the order of the deferred calls introduces a new issue. Due to Go's LIFO execution of defer statements, the current implementation will prevent failure tracking. I've left comments with suggestions to reorder the defer statements to ensure both success and failure paths are handled correctly.
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
| defer reporter.trackFailure(ctx, &err) | ||
| defer reporter.ensurePublished(ctx) |
There was a problem hiding this comment.
The order of these defer statements is important. In Go, defer statements are executed in a Last-In, First-Out (LIFO) order. With the current order, reporter.ensurePublished(ctx) will be executed before reporter.trackFailure(ctx, &err).
Both ensurePublished and trackFailure use sync.Once to ensure the usage record is published only once. This means if an error occurs, ensurePublished will run first and publish a success record. trackFailure will then run, but its attempt to publish a failure record will be a no-op.
To fix this, the defer statements should be reordered so that trackFailure is executed first.
| defer reporter.trackFailure(ctx, &err) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.ensurePublished(ctx) | |
| defer reporter.trackFailure(ctx, &err) |
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
This PR adds a safety ensurePublished(ctx) fallback across executors so 0-token / missing-usage upstream responses still emit a single usage record. Good direction, and CI looks green.
Blocking:
- The defer order is currently:
defer reporter.trackFailure(ctx, &err)defer reporter.ensurePublished(ctx)
In Go, defers run LIFO. SinceusageReporterusessync.Once,ensurePublishedwill run first and publish a success record (Failed=false), which can preventtrackFailurefrom ever publishing a failure record (Failed=true) whenerr != nil.
Please swap the order everywhere to:defer reporter.ensurePublished(ctx)defer reporter.trackFailure(ctx, &err)
Non-blocking:
- Consider a small helper to register these defers consistently to avoid reintroducing the ordering bug.
Test plan:
- CI: pr-test-build (passed)
- Recommended locally:
go test ./...
2 participants
When an upstream model returns
0tokens, CLIProxyAPI'spublishWithOutcomecorrectly drops the chunk to prevent empty streaming spam. However, several executors were missing thedefer reporter.ensurePublished(ctx)safety call at the end of their execution paths to guarantee that the request itself is logged exactly once before closing the connection. This PR patches all missing executors to ensure 0-usage responses still record the transaction cleanly.Resolves #2273