Fix double fetch

[garmr-ulfr]

This pull request makes targeted improvements to the configuration handling logic, focusing on more precise detection of when a configuration refetch is necessary and enhancing debug logging for user changes. The main changes are in the config/config.go file.

Improvements to config refetch logic and logging:

• Refined the shouldRefetch function to only trigger config refetches when both the new and old user info are non-nil and the old user has a valid legacy ID, reducing unnecessary refetches.
• Added a debug log statement to indicate when a user change is detected that requires a config refetch, improving traceability.

Logging cleanup:

• Removed a redundant debug log statement from the fetchConfig method to avoid excessive logging.

closes getlantern/engineering/issues/2899

[Copilot]

Pull request overview

This pull request fixes a bug where user creation could be triggered multiple times by centralizing the user creation logic into the NewRadiance function during application initialization. Additionally, it simplifies the UserInfo.GetData() interface by removing the unnecessary error return value and performs minor cleanup of logging statements.

• Moves user creation to NewRadiance to prevent double-fetching during configuration or API operations
• Simplifies UserInfo.GetData() interface by removing error return, making it more straightforward to use
• Removes verbose trace/debug logging and cleans up error handling across multiple modules

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
radiance.go	Adds centralized user creation via maybeCreateNewUser() called during initialization, removes APIHandler from config options
config/fetcher.go	Removes ensureUser() method and APIClient dependency, eliminating potential double-fetch
config/fetcher_test.go	Updates test instantiation to remove APIClient parameter from newFetcher()
config/config.go	Removes APIHandler from Options struct and simplifies shouldRefetch() logic, removes debug logging
config/config_test.go	Updates test to remove APIClient parameter from newFetcher()
common/user.go	Updates GetData() interface signature to remove error return
common/user/user.go	Implements simplified GetData() without error return, adds logging for user data changes
api/api.go	Updates NewAPIClient to use simplified GetData() without error handling
api/user_test.go	Updates mock to match new GetData() signature
issue/issue.go	Refactors to use AccountType() method instead of manual user data checks
telemetry/otel.go	Updates to use simplified GetData() with nil check instead of error handling
servers/manager.go	Removes verbose trace logging for server operations
vpn/boxoptions.go	Removes trace logging for config file unmarshalling

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[myleshorton]

So I think the only issue with this is for the case where users have poor/partially blocked connectivity, with a mobile user in Iran as maybe the quintessential example. In that scenario, the single call to create a user might fail. If it does, they're dead in the water because we'll never try again.

In that sequence, I think it makes sense to:

1. Couple the user creation to the config fetch if only because it's retried
2. Ideally retry more often in the failure case. This is a different "bug", but we'll currently wait for another 10 minutes if the request fails. To me, that should be the wait time if the request succeeds but it should be more like 30 seconds if the request fails.

[garmr-ulfr]

@myleshorton I re-added the ensure user check when fetching config and updated the fetch loop to retry on failure with exponential backoff.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[myleshorton]

Sorry got caught up in the infrastructure issues today but will look again soon.

[myleshorton]

I would opt for failing fast here and creating an error if apiClient is nil in newFetcher

[myleshorton]

I think the only issue here is that this becomes the one call that could take a long time in NewRadiance. If MaybeCreateNewUser takes a long time, which it could due to the time it can take the various kindling methods to start up, what happens?

[myleshorton]

Added a couple of comments, but, stepping back for a second, the user listener change I believe was added to account for two cases:

1. When a free user becomes pro
2. When a pro user becomes free (expires)

I think you could handle both of those with a small modification to the prior code, particularly in shouldRefetch you could just not refetch if the old config was nil or an empty config (i.e. there was no prior user).

In other words, I think we want to listen for a user's status changing, not a user being created.

[garmr-ulfr]

Yeah, you're right. The changes should be kept to a minimum. All fixed.

[myleshorton]

LGTM!!