Skip to content

fix: Don't change ConfiguredAddr when adding a transport #6804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Hocuri merged 1 commit into from
Jun 18, 2025
Merged

fix: Don't change ConfiguredAddr when adding a transport #6804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions src/login_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,20 +809,14 @@ impl ConfiguredLoginParam {
entered_param: &EnteredLoginParam,
) -> Result<()> {
let addr = addr_normalize(&self.addr);
let provider_id = self.provider.map(|provider| provider.id);
let configured_addr = context.get_config(Config::ConfiguredAddr).await?;
if let Some(configured_addr) = configured_addr {
if let Some(configured_addr) = &configured_addr {
ensure!(
addr_cmp(&configured_addr, &addr,),
addr_cmp(configured_addr, &addr),
"Adding a second transport is not supported right now."
);
}
context
.sql
.set_raw_config(
Config::ConfiguredProvider.as_ref(),
self.provider.map(|provider| provider.id),
)
.await?;
context
.sql
.execute(
Expand All @@ -837,10 +831,17 @@ impl ConfiguredLoginParam {
),
)
.await?;
context
.sql
.set_raw_config(Config::ConfiguredAddr.as_ref(), Some(&addr))
.await?;
if configured_addr.is_none() {
Copy link
Collaborator

@iequidoo iequidoo Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can race with itself, e.g. if two set_config_ex() calls are done in parallel for an unconfigured context. While this is unlikely, i'd suggest to add some try_set_raw_config() which does INSERT OR IGNORE. Extra locking it's better to avoid. Orphaned transports entry shouldn't be a problem i think.

Copy link
Collaborator Author

@Hocuri Hocuri Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this approach is that ConfiguredProvider should be set iff there was no ConfiguredAddr. While that would be possible to solve with some extra logic, I don't think the complexity (and by that, possibility of new bugs, and of confusing us in the future) is worth it. Many functions are not supposed to be called concurrently already (e.g. receive_imf()), and I don't see any reason why anyone would call save_to_transports_table() multiple times in parallel.

Copy link
Collaborator

@iequidoo iequidoo Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this approach is that ConfiguredProvider should be set iff there was no ConfiguredAddr.

Not a problem, try_set_raw_config() could return bool (whether the insertion happened).

save_to_transports_table() won't be called in parallel directly of course because it's an internal function, but set_config() can be. Both calls would succeed then which doesn't look correct.

While there's no such problem for other config keys, maybe just document that set_config() mustn't be called in parallel for the same key? Then save_to_transports_table() wouldn't violate anything.

EDIT: But overall doing INSERT OR REPLACE for a config key which isn't supposed to change is bug-prone and will shoot sooner or later. But if this code is a temporary solution while multi-transport isn't fully supported, that's fine.

Copy link
Collaborator

@link2xt link2xt Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfiguredProvider needs to go away eventually anyway. Then worst issue of the race condition is that another transport is set as default.

If race condition was a real problem here, then I'd start a transaction and do all the checks and replacement within it with raw config table changes. But in this case it is a config that is only changed in a response to UI actions or during initial bot setup.

// If there is no transport yet, set the new transport as the primary one
context
.sql
.set_raw_config(Config::ConfiguredProvider.as_ref(), provider_id)
.await?;
context
.sql
.set_raw_config(Config::ConfiguredAddr.as_ref(), Some(&addr))
.await?;
}
Ok(())
}

Expand Down
Loading