Fix WASM channel startup restore semantics#2563
Conversation
github-actions
bot
added
scope: channel/web
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the WASM channel startup process by ensuring that only explicitly activated channels are restored. It introduces a fallback mechanism where the setup wizard's configuration is used only until runtime activation state is persisted in the database. I have identified two critical issues regarding extension name canonicalization: the current check for active channels is case-sensitive and fails to account for name normalization (e.g., dashes vs underscores), and the returned list of channel names should use canonicalized forms to maintain consistency with the activation tracking logic in the main entry point.
| let channel_name = loaded.name().to_string(); | ||
| if !startup_active_channel_names.contains(&channel_name) { | ||
| tracing::debug!( | ||
| channel = %channel_name, | ||
| "Skipping installed but inactive WASM channel during startup restore" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The check against startup_active_channel_names is case-sensitive and does not account for extension name canonicalization (e.g., converting dashes to underscores). Since startup_active_channel_names is populated using normalize_extension_names, it contains canonicalized names. If loaded.name() returns a non-canonical form (like my-channel or Telegram), it will be incorrectly skipped during startup restore.
You should canonicalize the loaded channel's name before checking the set, handling potential errors gracefully as per project standards.
let raw_name = loaded.name();
let channel_name = match crate::extensions::naming::canonicalize_extension_name(raw_name) {
Ok(name) => name,
Err(e) => {
tracing::warn!(channel = %raw_name, error = %e, "Skipping WASM channel with invalid name");
continue;
}
};
if !startup_active_channel_names.contains(&channel_name) {References
- When parsing resources that can fail, centralize the parsing logic into a single function that handles errors gracefully (e.g., by logging a warning and returning None) to ensure consistent behavior.
| channel_names.push(name.clone()); | ||
| channels.push((name, channel)); |
There was a problem hiding this comment.
To ensure consistency with the activation tracking logic in main.rs, the channel_names vector should contain canonicalized names. If register_wasm_channel returns the raw name from the manifest, it may cause mismatches when checking active_at_startup.contains(name) later. Using the canonicalized channel_name (as defined at the start of the loop) ensures that the returned list matches the authoritative state in the settings store.
| channel_names.push(name.clone()); | |
| channels.push((name, channel)); | |
| channel_names.push(channel_name.clone()); | |
| channels.push((channel_name, channel)); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eab6c039c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(Some(value)) => match serde_json::from_value(value) { | ||
| Ok(names) => names, | ||
| Ok(Some(value)) => match serde_json::from_value::<Vec<String>>(value) { | ||
| Ok(names) => normalize_extension_names(names), |
There was a problem hiding this comment.
Normalizing persisted activated_channels names at load time can break relay auto-restore for upgraded instances that still use legacy hyphenated keys. After this conversion, restore_relay_channels() receives canonicalized names (e.g. slack_relay) but is_relay_channel() only checks an exact relay:{name}:team_id key, so existing relay:slack-relay:team_id entries are missed and startup silently skips reconnecting the relay until a manual reactivation migrates state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adjusts WASM channel startup/restore behavior to align “active” semantics with persisted runtime activation state, rather than treating all installed channels as active at boot.
Changes:
- Introduces
activated_channels-first restore logic withsettings.channels.wasm_channels(nowconfigured_wasm_channelsin resolved config) as a first-run fallback. - Adds
normalize_extension_names()to canonicalize/dedupe/skip invalid channel/extension names when restoring. - Updates startup registration so only startup-active WASM channels are restored/registered, and extends tests to ensure installed-but-inactive channels report correctly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Computes startup-active channel set and passes it into WASM channel setup; uses it for post-startup auto-activation. |
| src/channels/wasm/setup.rs | Filters startup restore to only channels in the startup-active set. |
| src/extensions/manager.rs | Adds load_startup_active_channels() and normalizes persisted activated_channels. |
| src/extensions/naming.rs | Adds normalize_extension_names() + unit test. |
| src/config/channels.rs | Adds configured_wasm_channels to resolved config (from settings wizard selection). |
| src/config/mod.rs | Updates Config::for_testing() to include configured_wasm_channels. |
| src/tunnel/mod.rs | Updates test helper ChannelsConfig construction for new field. |
| src/settings.rs | Updates docs for wasm_channels to reflect fallback semantics. |
| src/setup/README.md | Documents the first-run fallback vs persisted activated_channels behavior. |
| src/channels/web/server.rs | Adds regression test ensuring installed inactive WASM channels list as inactive. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let startup_active_channel_names = if let Some(ref ext_mgr) = components.extension_manager { | ||
| ext_mgr | ||
| .load_startup_active_channels( | ||
| &config.owner_id, | ||
| config.channels.configured_wasm_channels.clone(), | ||
| ) | ||
| .await | ||
| } else { | ||
| normalize_extension_names(config.channels.configured_wasm_channels.clone()) | ||
| }; | ||
| let startup_active_channel_name_set: HashSet<String> = | ||
| startup_active_channel_names.iter().cloned().collect(); |
| for loaded in loaded_channels { | ||
| let channel_name = loaded.name().to_string(); | ||
| if !startup_active_channel_names.contains(&channel_name) { | ||
| tracing::debug!( | ||
| channel = %channel_name, | ||
| "Skipping installed but inactive WASM channel during startup restore" | ||
| ); | ||
| continue; | ||
| } |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Reviewed: clean semantics fix for WASM channel startup restore. Normalization is consistent across both read paths, fallback logic is sound, test coverage is solid.
Summary
Testing