@1egoman , could you please help review this token-lifecycle.md ?

I actually generated this for him while I had some active LLM context around this topic, I removed it as I don't want this SDK to be an authoritative documentation source compared to docs.livekit.io.

I do think we should have some better docs around token source and lifetimes, while going through this process it felt a little disjointed and agents/frontend-focused and not just a general document for token source terms, functions, lifecycles, etc for client SDKs.

nit, would you consider forward declare those types, and move the #include "livekit/token_source.h" to the cpp ?

One benefit for it is that it could speed up the build.

Added

nit, just do "virtual ~TokenSourceFixed() = default" here and remove the cpp line

The same for virtual ~TokenSourceConfigurable() = default;

Added

consider a helper function like

namespace {

template <typename FetchFn>
bool connectWithTokenSource(Room& room, const RoomOptions& options, FetchFn&& fetch) {
  Result<TokenSourceResponse, TokenSourceError> details =
      Result<TokenSourceResponse, TokenSourceError>::failure(TokenSourceError{"token source not invoked"});

  try {
    details = fetch().get();
  } catch (const std::exception& e) {
    LK_LOG_ERROR("Room::connect failed: token source threw: {}", e.what());
    return false;
  } catch (...) {
    LK_LOG_ERROR("Room::connect failed: token source threw unknown exception");
    return false;
  }

  if (!details) {
    LK_LOG_ERROR("Room::connect failed: token source error: {}", details.error().message);
    return false;
  }

  const auto& value = details.value();
  return room.connect(value.server_url, value.participant_token, options);
}

} // namespace

Then these two function can become:

bool Room::connect(TokenSourceFixed& token_source, const RoomOptions& options) {
  return connectWithTokenSource(*this, options, [&] {
    return token_source.fetch();
  });
}

bool Room::connect(TokenSourceConfigurable& token_source,
                   const TokenRequestOptions& request_options,
                   const RoomOptions& options) {
  return connectWithTokenSource(*this, options, [&] {
    return token_source.fetch(request_options, false);
  });
}

FWIW, we opted not to do this in other sdks, and determined this was the session api's job. Relevant pull request: livekit/client-sdk-js#1677

Added!

any idea why CustomTokenSource will skip the force_refresh ?

This comment actually led to an API design change, see here: e39e55e

Having such an argument for a custom source would require passing that argument along to the custom implementation, which felt weird as the custom token source was just a user function that could decide anything it needed to. I have instead dropped force_refresh entirely, and opted for caching_token_source.invalidate() which matches how Swift/Android do it.

In short, this original design around forcing refresh was modeled after JS with the force_refresh. But JS actually goes for an inheritance-based approach to cacheing token sources, vs. here which is using a decorator (Swift/Android do it this way), so forcing refresh was consistent across all sub-classes and didn't have an awkward drop like this did.

It was basically a divergence in both approaches (force + decorator vs. force + inheritance), now it's more aligned to Swift/Android.

any reason why we want to run the inner_->fetch() under the lock ?

Will it be safer to do ?

{
  std::scoped_lock lock(mutex_);
  if (!force_refresh && cached_details_ && cached_options_ &&
      tokenRequestOptionsEqual(*cached_options_, *options_snapshot) &&
      isParticipantTokenValid(cached_details_->participant_token)) {
    return TokenSourceResult::success(*cached_details_);
  }
}

auto result = inner_->fetch(*options_snapshot, force_refresh).get();

Holding the mutex over the fetch is how JS and Swift do it, I'm open to your change but it would be a divergence

isn't the endpoint_ created in line 210 already ? any reason why it needs to be re-created ?

Great catch, this slipped through. Now have a private constructor that accepts an already made endpoint so both the public and test access versions can avoid recreation

question, does path need to include lpszExtraInfo ? like when lpszExtraInfoLength > 0 ?

Addressed