fix: persist WASM channel workspace writes across callbacks#264
fix: persist WASM channel workspace writes across callbacks#264ilblackdragon merged 2 commits intomainfrom
Conversation
WASM channel callbacks (polling, webhooks, on_start) call workspace_write() to persist state, but the host code never committed these writes — take_pending_writes() was never called. Additionally, no WorkspaceReader was injected into channel capabilities, so workspace_read() always returned None. This caused Telegram's polling offset to reset to 0 on every tick, making getUpdates re-deliver already-processed messages and producing 2-4 duplicate LLM responses per user message. Add ChannelWorkspaceStore (Arc-wrapped HashMap with std::sync::RwLock) that persists across callback invocations within a channel's lifetime. Inject it as the WorkspaceReader and commit pending writes after every callback execution (on_start, on_poll, on_http_request, execute_poll). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @serrrfirat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a significant issue where state changes made within WASM channel callbacks were not being persisted, leading to data loss and incorrect behavior, particularly with Telegram polling offsets. By introducing a dedicated in-memory workspace store and integrating its read and commit operations into the WASM channel's lifecycle, the system can now reliably maintain state. This enhancement ensures that WASM-based channels operate correctly and prevents undesirable outcomes such as duplicate message processing. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical issue where WASM channel workspace writes were not being persisted, leading to problems like message re-delivery. The introduction of ChannelWorkspaceStore to maintain state across callback invocations is a solid approach. The changes are well-implemented across all relevant callback paths (on_start, on_poll, on_http_request, and execute_poll), ensuring that workspace reads and writes now function as expected. I've added a couple of suggestions to improve the robustness of the ChannelWorkspaceStore by explicitly handling potential RwLock poisoning, which will prevent silent failures in edge cases and aid in debugging by logging errors.
| if let Ok(mut data) = self.data.write() { | ||
| for write in writes { | ||
| tracing::debug!( | ||
| path = %write.path, | ||
| content_len = write.content.len(), | ||
| "Committing workspace write to channel store" | ||
| ); | ||
| data.insert(write.path.clone(), write.content.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation silently ignores a poisoned RwLock if self.data.write() returns an Err. This means if a panic occurs while another thread holds the write lock, subsequent writes will be silently dropped, which could lead to the same kind of data loss issues this PR is trying to fix. It's more robust to handle this case by logging an error to make debugging easier and prevent silent failures.
| if let Ok(mut data) = self.data.write() { | |
| for write in writes { | |
| tracing::debug!( | |
| path = %write.path, | |
| content_len = write.content.len(), | |
| "Committing workspace write to channel store" | |
| ); | |
| data.insert(write.path.clone(), write.content.clone()); | |
| } | |
| } | |
| match self.data.write() { | |
| Ok(mut data) => { | |
| for write in writes { | |
| tracing::debug!( | |
| path = %write.path, | |
| content_len = write.content.len(), | |
| "Committing workspace write to channel store" | |
| ); | |
| data.insert(write.path.clone(), write.content.clone()); | |
| } | |
| } | |
| Err(e) => { | |
| tracing::error!(error = %e, "Failed to acquire write lock on ChannelWorkspaceStore, workspace writes will be lost. This may indicate a panic occurred in another thread."); | |
| } | |
| } |
References
- When handling errors, it is good practice to log them to capture debugging information and prevent silent failures, similar to how
JoinErrorfromtokio::task::spawn_blockingshould be logged.
| fn read(&self, path: &str) -> Option<String> { | ||
| self.data.read().ok()?.get(path).cloned() | ||
| } |
There was a problem hiding this comment.
Similar to the commit_writes function, the read function will silently fail by returning None if the RwLock is poisoned (self.data.read().ok()?). This can mask underlying issues where a panic might have occurred during a write. It would be more robust to log an error in case of a poisoned lock to aid in debugging.
fn read(&self, path: &str) -> Option<String> {
match self.data.read() {
Ok(data) => data.get(path).cloned(),
Err(e) => {
tracing::error!(error = %e, "Failed to acquire read lock on ChannelWorkspaceStore, read will fail. This may indicate a panic occurred in another thread.");
None
}
}
}References
- When handling errors, it is good practice to log them to capture debugging information and prevent silent failures, similar to how
JoinErrorfromtokio::task::spawn_blockingshould be logged.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: persist WASM channel workspace writes across callbacks WASM channel callbacks (polling, webhooks, on_start) call workspace_write() to persist state, but the host code never committed these writes — take_pending_writes() was never called. Additionally, no WorkspaceReader was injected into channel capabilities, so workspace_read() always returned None. This caused Telegram's polling offset to reset to 0 on every tick, making getUpdates re-deliver already-processed messages and producing 2-4 duplicate LLM responses per user message. Add ChannelWorkspaceStore (Arc-wrapped HashMap with std::sync::RwLock) that persists across callback invocations within a channel's lifetime. Inject it as the WorkspaceReader and commit pending writes after every callback execution (on_start, on_poll, on_http_request, execute_poll). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: persist WASM channel workspace writes across callbacks WASM channel callbacks (polling, webhooks, on_start) call workspace_write() to persist state, but the host code never committed these writes — take_pending_writes() was never called. Additionally, no WorkspaceReader was injected into channel capabilities, so workspace_read() always returned None. This caused Telegram's polling offset to reset to 0 on every tick, making getUpdates re-deliver already-processed messages and producing 2-4 duplicate LLM responses per user message. Add ChannelWorkspaceStore (Arc-wrapped HashMap with std::sync::RwLock) that persists across callback invocations within a channel's lifetime. Inject it as the WorkspaceReader and commit pending writes after every callback execution (on_start, on_poll, on_http_request, execute_poll). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
on_poll,on_http_request,on_start) callworkspace_write()to persist state (e.g., Telegram polling offset), buttake_pending_writes()was never called after execution — writes were silently discardedWorkspaceReaderwas injected into channel capabilities, soworkspace_read()always returnedNonegetUpdatesre-deliver already-processed messages and producing 2-4 duplicate LLM responses per user messageFix
ChannelWorkspaceStore— anArc-wrappedHashMapwithstd::sync::RwLockthat persists across callback invocations within a channel's lifetimeWorkspaceReaderinto capabilities before each callbacktake_pending_writes()and commit to the store after every callback (on_start,on_poll,on_http_request,execute_poll)Files changed
src/channels/wasm/host.rsChannelWorkspaceStorestruct + implWorkspaceReader+ testssrc/channels/wasm/wrapper.rsWasmChannel, inject reader + commit writes in all 4 callback pathsTest plan
cargo buildcompiles cleanChannelWorkspaceStorecommit and read behaviorcargo test— all existing tests passcargo clippy --all --all-features— no warnings🤖 Generated with Claude Code