fix: restore only active WASM channels at startup#2562
fix: restore only active WASM channels at startup#2562henrypark133 wants to merge 2 commits intostagingfrom
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 refactors the WASM channel setup process to support the restoration of persisted active channels. It introduces a registration context for startup, improves the discovery and loading logic to filter by active status, and adds comprehensive unit tests. Review feedback highlights opportunities to remove redundant logging and logic checks in the setup flow, as well as a performance optimization in the main entry point to avoid unnecessary heap allocations when processing channel names.
| for (path, err) in &load_errors { | ||
| tracing::warn!("Failed to load WASM channel {}: {}", path.display(), err); | ||
| } |
There was a problem hiding this comment.
This block causes double logging of WASM channel load failures. These errors are already logged with more detailed context (including the channel name and specific error message) during the loading loop at lines 127-132.
| if !context | ||
| .startup_active_channel_names | ||
| .is_none_or(|active_names| active_names.contains(loaded.name())) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This check is redundant. The loaded_channels vector is derived from startup_entries, which is already filtered by startup_active_channel_names in the caller function setup_wasm_channels (lines 106-114). Removing this redundant check simplifies the loop logic.
| let mut wasm_channels = Vec::new(); | ||
| for name in persisted_active_channels { | ||
| if persisted_active_relay_channels.contains(name) { | ||
| continue; | ||
| } | ||
| wasm_channels.push(name.clone()); | ||
| } | ||
| normalize_persisted_wasm_channel_names(wasm_channels) |
There was a problem hiding this comment.
This logic can be optimized by using an iterator filter instead of allocating an intermediate Vec. Since normalize_persisted_wasm_channel_names accepts any IntoIterator, we can pass the filtered iterator directly. This avoids unnecessary heap allocations, which is important for performance in WASM environments.
let wasm_channels = persisted_active_channels
.iter()
.filter(|name| !persisted_active_relay_channels.contains(*name));
normalize_persisted_wasm_channel_names(wasm_channels)References
- To improve performance in WASM, avoid unnecessary heap allocations. For instance, when processing string parts, use iterators directly instead of collecting them into a Vec.
There was a problem hiding this comment.
Pull request overview
This PR fixes a startup restore bug for WASM channels by separating “installed on disk” from “active/running,” ensuring only persisted-active WASM channels are restored at boot while still listing installed-but-inactive channels as discoverable but inactive.
Changes:
- Load persisted
activated_channelsearlier during startup and pass a WASM-only persisted-active set into WASM channel setup so boot only restores those channels (no-DB mode still restores all installed channels). - Align extension active-state reporting (
active_channel_names) with channels actually running in-process rather than discovered-on-disk. - Harden tests by clearing ambient
CARGO_TARGET_DIRunder the repo’s env mutex, and adjust pending-gate extension-name resolution whenauth_manageris absent.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Computes persisted-active WASM restore set (with relay compatibility) and wires startup restore flow accordingly. |
| src/channels/wasm/setup.rs | Adds optional startup-active filter; loads/registers only startup-selected WASM channels while keeping webhook router available. |
| src/extensions/manager.rs | Clarifies semantics of active_channel_names; test hardening for env isolation (CARGO_TARGET_DIR). |
| src/channels/web/server.rs | Improves pending auth-gate extension name inference without auth_manager; adds regression test for inactive WASM listing. |
| src/registry/artifacts.rs | Hardens artifact tests by clearing CARGO_TARGET_DIR under the env mutex. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if matches!( | ||
| tool_name, | ||
| "tool_install" | "tool-install" | "tool_activate" | "tool_auth" |
There was a problem hiding this comment.
pending_gate_extension_name() handles both underscore and hyphen variants for tool_install, but the new fallback branch only matches underscore forms for tool_activate and tool_auth. Elsewhere in the codebase both hyphenated names exist (e.g. tool-activate, tool-auth), so this will fail to resolve the extension name for those tool names when auth_manager is absent. Consider including the hyphenated variants in the matches!() list (or normalizing tool_name once before matching).
| "tool_install" | "tool-install" | "tool_activate" | "tool_auth" | |
| "tool_install" | |
| | "tool-install" | |
| | "tool_activate" | |
| | "tool-activate" | |
| | "tool_auth" | |
| | "tool-auth" |
| tracing::warn!( | ||
| channel = %name, | ||
| path = %wasm_path.display(), | ||
| error = %err, | ||
| "Failed to load persisted-active WASM channel at startup" |
There was a problem hiding this comment.
This warning message is emitted for any startup load failure, including the no-persistence path where startup_active_channel_names is None and all installed channels are attempted. In that case, the wording "persisted-active" is inaccurate/misleading. Consider making the message conditional on startup_active_channel_names.is_some() (or using more general wording like "Failed to load WASM channel at startup").
| tracing::warn!( | |
| channel = %name, | |
| path = %wasm_path.display(), | |
| error = %err, | |
| "Failed to load persisted-active WASM channel at startup" | |
| let warning_message = if startup_active_channel_names.is_some() { | |
| "Failed to load persisted-active WASM channel at startup" | |
| } else { | |
| "Failed to load WASM channel at startup" | |
| }; | |
| tracing::warn!( | |
| channel = %name, | |
| path = %wasm_path.display(), | |
| error = %err, | |
| "{warning_message}" |
| for (path, err) in &load_errors { | ||
| tracing::warn!("Failed to load WASM channel {}: {}", path.display(), err); | ||
| } |
There was a problem hiding this comment.
setup_wasm_channels() logs load failures twice: once with structured fields inside the load loop and again here via load_errors. This will produce duplicated warnings for each failing channel at startup. Consider removing this second loop or downgrading one of the logs (e.g. keep the structured warn and make this a debug summary).
![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: 1d568ee17b
ℹ️ 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".
| match ironclaw::extensions::naming::canonicalize_extension_name(name.as_ref()) { | ||
| Ok(canonical) => { |
There was a problem hiding this comment.
This canonicalization step drops persisted channel names that were previously valid at runtime (for example names with uppercase letters, dots, or double underscores), because canonicalize_extension_name is stricter than the WASM channel loader’s name checks. In that case the channel remains in activated_channels but is filtered out of persisted_active_wasm_channels, so startup never reloads it and it silently stops restoring after restart. Consider preserving persisted names as-is for WASM restore filtering (or validating with the same rules as WasmChannelLoader).
Useful? React with 👍 / 👎.
github-actions
bot
added
size: XL
Summary
Fixes the WASM channel boot-restore bug where startup treated every channel discovered on disk as active.
Boot now restores only channels persisted in
activated_channels, while keeping installed-but-inactive WASM channels discoverable in/api/extensionswithactive: false.Fixes #2556
Root Cause
Startup was conflating three different states:
setup_wasm_channels(...)loaded every discovered channel fromwasm_channels_dir,ChannelManagerstarted them, andExtensionManager::set_active_channels(...)then reported discovery as runtime-active state.What Changed
Startup restore semantics
activated_channelsbefore WASM channel setup inmain.rs.setup_wasm_channels(...).active_channel_namesaligned with channels actually running now, not channels merely found on disk.No-persistence behavior
setup_wasm_channels(...)now accepts an optional startup-active filter.Legacy relay-name compatibility
is_relay_channel(...).slack-relayso relay restore/auth lookup continues to work.Follow-up fix
pending_gate_extension_name(...)now mirrors auth-manager install-action resolution even whenauth_manageris absent, so post-install auth gates continue to show the extension name (telegram) instead of the underlying credential name (telegram_bot_token).Test hardening
CARGO_TARGET_DIRunder the repo env mutex so they do not depend on the caller's build environment.Files
src/main.rssrc/channels/wasm/setup.rssrc/extensions/manager.rssrc/channels/web/server.rssrc/registry/artifacts.rsTests
Ran:
cargo fmt --checkgit diff --checkcargo clippy --all-targets --all-features -- -D warningscargo test --lib channels::web::server::tests::pending_gate_extension_name_uses_install_parameters_for_post_install_auth -- --exact --nocapturecargo test --lib registry::artifacts::tests::test_resolve_target_dir_default -- --exact --nocapturecargo test --lib registry::artifacts::tests::test_find_any_wasm_artifact_not_found -- --exact --nocapturecargo test --lib extensions::manager::tests::ensure_extension_ready_auto_installs_registry_wasm_tool_on_explicit_activate -- --exact --nocapturecargo test --lib channels::wasm::setup::tests::register_startup_loaded_channels_only_restores_persisted_active_channels -- --exact --nocapturecargo test --lib channels::wasm::setup::tests::register_startup_loaded_channels_without_persistence_restores_all_channels -- --exact --nocaptureBehavior After This Change