Repository navigation
Add invalidate() to the token source trait - #1592
MaxHeimbrock wants to merge 2 commits into
Conversation
MaxHeimbrock
changed the title
ca54629 to
793fade
Compare
5c25a50 to
085b9b7
Compare
…ent() Code generic over a configurable token source had no way to drop cached credentials, because invalidate() existed only on TokenSourceCached. Add it as a default trait method. TokenSourceCached clears its store and forwards inward, so a source that is cached twice is cleared at every layer; the inherent method now does the same. dispatches_agent() reads roomConfig.agents from the unverified claims, so a caller can tell whether the token will dispatch an agent without depending on livekit-token. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
793fade to
0d617b7
Compare
An agent registered without a name is dispatched automatically into every new room, so a token without roomConfig.agents can still bring an agent in. The helper's false therefore did not mean "no agent is coming", which is exactly how its only caller read it. Drop it rather than ship an API that invites that reading. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxHeimbrock
changed the title
| // Through the trait, as code generic over the source would call it. | ||
| async fn invalidate(source: &(dyn TokenSourceConfigurable + Send + Sync)) { | ||
| source.invalidate().await; | ||
| } | ||
| invalidate(&cached).await; |
There was a problem hiding this comment.
nitpick: Can you swap this for a fully qualified TokenSourceConfigurable::invalidate (or if you went with my other suggestion, TokenSourceCached::invalidate) call?
| // Through the trait, as code generic over the source would call it. | |
| async fn invalidate(source: &(dyn TokenSourceConfigurable + Send + Sync)) { | |
| source.invalidate().await; | |
| } | |
| invalidate(&cached).await; | |
| // Through the trait, as code generic over the source would call it. | |
| TokenSourceConfigurable::invalidate(&cached).await; |
|
|
||
| /// Drops any cached credentials so the next fetch hits the backend. | ||
| /// | ||
| /// The default does nothing, for sources that cache nothing. | ||
| /// [`TokenSourceCached`] clears its store and then forwards to the source | ||
| /// it wraps, so a source cached more than once is cleared at every layer. | ||
| async fn invalidate(&self) {} | ||
|
|
||
| /// Wraps this source in a caching layer that stores the last fetched |
There was a problem hiding this comment.
question: I would think that TokenSourceCached should have the invalidate method, not TokenSourceConfigurable? Avoiding these types of no-op methods was a big reason why we opted to split out fixed / configurable token sources in the first place.
If you split this out then I think the session api layer would need to become a bit more aware of TokenSourceCached, but I don't know if that is necesarily a bad thing, just like how it has to know about TokenSourceConfigurable / TokenSourceFixed.
There was a problem hiding this comment.
I thought about this some more and the idea was nice:
Prevent a cached token source passed into the session from messing with agent dispatch. But we also offer a fixed token source to be used in the session, and that has the same issue with agent dispatch, reusing the same token on reconnecting.
There was no cleaner way I found today unifying the use of fixed and configurable and the generic cached sources in a nicer way than this. And I agree that putting invalidate into token_source.rs feels like leaking the caching concept, which lives as a wrapper in caching.rs as an optional add-on.
So I tried to keep things simpler now, as we do in the other SDKs, and not make this a concern for now. What do you think @1egoman.
|
Closing without merging. The Session crate (#1593) no longer invalidates its token source on disconnect, matching the Swift SDK's Session, so nothing needs |
Add
invalidate()to the token source trait. Stacked on #1591.invalidate()can be used to invalidate a token once it was used for agent dispatch, since joining the same room twice in a row will not trigger a second dispatch. Now theinvalidate()can be called on the token source trait itself, not only on the cached version. Also double wrapped.cached()sources are invalidating recursively (see test).