Repository navigation
Conversation
|
| Err(err) => warn!( | ||
| "Failed to read the token of profile `{}` again, keeping the one this run started with: {err}", | ||
| profile.profile_name | ||
| ), |
There was a problem hiding this comment.
Expired token survives concurrent login
If codspeed auth login is writing a refreshed token when the upload rereads the config, the read can encounter a partial file. This branch then keeps the token from the start of the run. If that token has expired, the upload fails and the run’s results are lost—the failure this change aims to prevent. The login path uses fs::write, so the refresh needs an atomic read/write boundary or a retry before falling back.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/api_client.rs
Line: 136-139
Comment:
**Expired token survives concurrent login**
If `codspeed auth login` is writing a refreshed token when the upload rereads the config, the read can encounter a partial file. This branch then keeps the token from the start of the run. If that token has expired, the upload fails and the run’s results are lost—the failure this change aims to prevent. The login path uses `fs::write`, so the refresh needs an atomic read/write boundary or a retry before falling back.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid, but very unlikely (a tiny file rewritten about once an hour), and in that case the run keeps its start token, which is the current behavior, so I'd leave an atomic write in auth login for a follow-up.
There was a problem hiding this comment.
That’s fair. The race window is small, and falling back to the run-start token preserves the existing behavior rather than introducing a regression in this PR. I’ll consider the partial-read risk deferred to a follow-up that makes the auth login profile write atomic.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Merging this PR will not alter performance
|
`codspeed run` read the token persisted by `codspeed auth login` once, at start, and uploaded with it. A run that outlived that token (the Wizard refreshes its one-hour tokens by running `codspeed auth login` again while the benchmarks run) uploaded with the expired one and failed with `401 Unauthorized -> Reason: Invalid token`, losing the results of the whole run. Read the profile token again right before each upload, the same way an OIDC token is minted again. Tokens given through `--token` or `--oauth-token` cannot change while the run goes, so they are kept as they are. The token is still read at start, so a missing or invalid token fails the command before the benchmarks run. The legacy-config test now isolates `XDG_CONFIG_HOME` with `temp_env` instead of setting it for the whole process, so it does not race with the new tests. Refs COD-3768 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
acac8f1 to
6be3b53
Compare
| /// A token obtained through `codspeed auth login` and passed through | ||
| /// `--oauth-token` / `CODSPEED_OAUTH_TOKEN`. | ||
| CliLogin(String), | ||
| /// The token `codspeed auth login` persisted for the selected profile. Read | ||
| /// again before every upload, because a login that ran during the | ||
| /// benchmarks may have stored a newer token. | ||
| PersistedCliLogin { | ||
| token: String, | ||
| profile: ProfileLocation, | ||
| }, |
There was a problem hiding this comment.
Why do we need to add a new authentication method? We can keep the same method and simply re-resolve it before upload right? Isn't it what we talked about?
There was a problem hiding this comment.
This keeps the profile selected at the start of the run, to avoid this case:
codspeed profile set Acodspeed runstarts and reads the token of profile Acodspeed profile set Bcodspeed runfinishes, and the upload reads the token of profile B
It's probably rare, but possible.
There was a problem hiding this comment.
Do you think this edge case is too unlikely to be worth handling?
There was a problem hiding this comment.
Yes I think it a bit far-fetched. What you are telling me here is that the existing CliLogin method does not read the profile file? If it does, then maybe the ProfileLocation should be added to CliLogin instead of creating a duplicated logic
`CliLogin` already holds the token persisted by `codspeed auth login`. Give it an optional `ProfileLocation` instead of adding a separate variant: `Some` when the token was read from a profile, so it is read again before each upload, and `None` when it came from `--oauth-token` or `CODSPEED_OAUTH_TOKEN`. No behavior change. Refs COD-3768 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Closing: instead of reading the token again from disk, the CLI will get a refresh mechanism (COD-3776). |
Problem
codspeed runreads the token saved bycodspeed auth loginonce, at start. It uploads with that same token at the end.If the token expires during the run, the upload fails with
401 Invalid token, and all the results are lost. Runningcodspeed auth loginagain during the run does not help: the running CLI never reads the new token.For example, a setup that saves a new 1-hour token before the old one expires still loses a 53-minute run this way.
Fix
Read the saved token again right before each upload. This is what the CLI already does for OIDC tokens in CI.
codspeed auth loginis read again. A token from--token,CODSPEED_TOKEN,--oauth-tokenorCODSPEED_OAUTH_TOKENcan't change during a run, so it stays as is.Proof
A 30 s walltime run, with the saved token changed 12 s into the run:
The unit tests in
api_client.rscover the same cases.Refs COD-3768