properly retry when LD client initialization fails

[def-]

Reopened from #32030, does anyone know why we didn't merge this? There are 2 TODOs in the code referencing it.

Claude claims this might not work:

3. The fix doesn't work. The PR's whole mechanism is calling start_with_default_executor again to force a new initialization. That call early-returns:

  pub fn start_with_default_executor(&self) {
      if self.started.load(Ordering::SeqCst) { return; }   // ← no-op on every retry

Original description:

Previously the code initializing the LD client would correctly await initialized_async to see if the initialization succeeded. However, if it didn't succeed it would simply wait a bit and then call initialized_async again. Reading the LD server sdk code, there is no reason to assume that the call would return something different if repeated.

This commit changes the logic to call start_with_default_executor again when initialized_async reports failure, to attempt a new initialization. It also moves to the mz-ore Retry type, instead of implementing manual retry logic.

[aljoscha]

This is curious, I checked the code, and start_with_default_executor will indeed just return when called a second time, and do nothing. I don't know if the assumption of the initial PR is true, eyeballing the code around start_with_default_executor and wait_for_initialization, I'd say that additional waiting can change the result.

Here's the relevant c ode:

    fn start_with_default_executor_internal(&self) {
        // These clones are going to move into the closure, we
        // do not want to move or reference `self`, because
        // then lifetimes will get involved.
        let notify = self.init_notify.clone();
        let init_state = self.init_state.clone();

        self.data_source.subscribe(
            self.data_store.clone(),
            Arc::new(move |success| {
                init_state.store(
                    (if success {
                        ClientInitState::Initialized
                    } else {
                        ClientInitState::InitializationFailed
                    }) as usize,
                    Ordering::SeqCst,
                );
                notify.add_permits(1);
            }),
            self.shutdown_broadcast.subscribe(),
        );
    }

    async fn initialized_async_internal(&self) -> bool {
        if self.offline || self.daemon_mode {
            return true;
        }

        // If the client is not initialized, then we need to wait for it to be initialized.
        // Because we are using atomic types, and not a lock, then there is still the possibility
        // that the value will change between the read and when we wait. We use a semaphore to wait,
        // and we do not forget the permit, therefore if the permit has been added, then we will get
        // it very quickly and reduce blocking.
        if ClientInitState::Initialized != self.init_state.load(Ordering::SeqCst) {
            let _permit = self.init_notify.acquire().await;
        }
        ClientInitState::Initialized == self.init_state.load(Ordering::SeqCst)
    }

So the initialization method puts in a callback that will eventually "notify" the thing the waiting method is waiting on.

From this, though, I'd think we don't actually need a retry loop, because the waiting isn't doing anything except wait on that semaphore. So putting in a longer total timeout should have the same behavior as adding a retry configuration. But ... 🤷‍♂️

[ggevay]

(Note that our LD stuff has changed a bit recently, together with the LD version upgrade, so maybe direct your Claudes to also look at whether any of those changes are relevant.)