Skip to content

feat(wassette): unload-component now should delete files on Disk#81

Merged
asw101 merged 5 commits into
mainfrom
mossaka/unload-component2
Aug 5, 2025
Merged

feat(wassette): unload-component now should delete files on Disk#81
asw101 merged 5 commits into
mainfrom
mossaka/unload-component2

Conversation

@Mossaka

@Mossaka Mossaka commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

Currently, unload-component API only deletes the runtime state of the Wassette server, but it doesn not touch the files on the Disk. There is a separate API called uninstall-component. However, this is not a symmetric to load-component that users would expect. This commit deletes the unused uninstall-component function and modifies the unload-componnet API to be able to delete the wasm and co-located policy files on Disk.

Closes #78

Signed-off-by: Jiaxiao Zhou duibao55328@gmail.com

@Mossaka Mossaka requested a review from Copilot August 4, 2025 19:31

This comment was marked as outdated.

@thomastaylor312 thomastaylor312 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One minor nit where I agree with copilot.

Comment thread crates/wassette/src/lib.rs
@Mossaka Mossaka requested a review from Copilot August 5, 2025 07:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the unload_component API to make it the symmetric reverse operation of load_component by deleting component files from disk. The changes remove the redundant uninstall_component function and update unload_component to handle both runtime state cleanup and file removal.

  • Modified unload_component to return a Result and delete component, policy, and metadata files from disk
  • Removed the separate uninstall_component function to eliminate API confusion
  • Updated all call sites to handle the new error-returning signature

Reviewed Changes

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

Show a summary per file
File Description
crates/wassette/src/lib.rs Core implementation changes: added file deletion logic to unload_component, removed uninstall_component, and added helper method for consistent file removal
crates/mcp-server/src/components.rs Updated MCP server handler to handle new error-returning unload_component signature with proper error responses
tests/transport_integration_test.rs Updated test cleanup to handle new Result return type
tests/grant_permission_integration_test.rs Updated test cleanup to handle new Result return type
tests/file_integration_test.rs Updated test cleanup to handle new Result return type

Comment thread crates/wassette/src/lib.rs Outdated
self.remove_file_if_exists(&policy_path, "policy file", id)
.await?;

let metadata_path = self.plugin_dir.join(format!("{id}.policy.meta.json"));

Copilot AI Aug 5, 2025

Copy link

Choose a reason for hiding this comment

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

The metadata file path construction is duplicated across unload_component and detach_policy_from_component. Consider extracting this into a helper method like get_component_metadata_path to maintain consistency and reduce code duplication.

Suggested change
let metadata_path = self.plugin_dir.join(format!("{id}.policy.meta.json"));
let metadata_path = self.get_component_metadata_path(id);

Copilot uses AI. Check for mistakes.
Comment thread crates/wassette/src/lib.rs Outdated
component_file.display()
))
.component_policies
.remove(id);

Copilot AI Aug 5, 2025

Copy link

Choose a reason for hiding this comment

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

The policy registry cleanup logic is duplicated between unload_component and detach_policy_from_component. Consider extracting this into a helper method to reduce code duplication and ensure consistent cleanup behavior.

Note: See the diff below for a potential fix:

@@ -703,11 +712,7 @@
         self.components.write().await.remove(id);
         self.registry.write().await.unregister_component(id);
 
-        self.policy_registry
-            .write()
-            .await
-            .component_policies
-            .remove(id);
+        self.cleanup_policy_registry_for_component(id).await;
 
         let component_file = self.component_path(id);
         self.remove_file_if_exists(&component_file, "component file", id)

Copilot uses AI. Check for mistakes.
@asw101

asw101 commented Aug 5, 2025

Copy link
Copy Markdown
Member

@Mossaka I have been spelunking. Here is one suggestion:

Policy Registry Cleanup Ordering: In unload_component in crates/wassette/src/lib.rs, policy registry cleanup happens before file removal. If file removal fails, the in-memory state becomes inconsistent. Consider:

// Remove files first, then clean up memory on success
let component_file = self.component_path(id);
self.remove_file_if_exists(&component_file, "component file", id).await?;
// ... other file removals

// Only cleanup memory after all files are successfully removed
self.components.write().await.remove(id);
self.registry.write().await.unregister_component(id);
self.policy_registry.write().await.component_policies.remove(id);

The same would apply to detach_policy(). Thoughts?

@Mossaka

Mossaka commented Aug 5, 2025

Copy link
Copy Markdown
Contributor Author

@Mossaka I have been spelunking. Here is one suggestion:

Policy Registry Cleanup Ordering: In unload_component in crates/wassette/src/lib.rs, policy registry cleanup happens before file removal. If file removal fails, the in-memory state becomes inconsistent. Consider:

// Remove files first, then clean up memory on success
let component_file = self.component_path(id);
self.remove_file_if_exists(&component_file, "component file", id).await?;
// ... other file removals

// Only cleanup memory after all files are successfully removed
self.components.write().await.remove(id);
self.registry.write().await.unregister_component(id);
self.policy_registry.write().await.component_policies.remove(id);

The same would apply to detach_policy(). Thoughts?

Done

Mossaka added 4 commits August 5, 2025 11:24
Currently, unload-component API only deletes the runtime state of the Wassette server, but it doesn not touch the files on the Disk. There is a separate API called uninstall-component. However, this is not a symmetric to load-component that users would expect. This commit deletes the unused uninstall-component function and modifies the unload-componnet API to be able to delete the wasm and co-located policy files on Disk.

Signed-off-by: Jiaxiao Zhou <duibao55328@gmail.com>
…er func

Signed-off-by: Jiaxiao Zhou <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou <duibao55328@gmail.com>
…ure file removal before memory cleanup

Signed-off-by: Jiaxiao Zhou <duibao55328@gmail.com>
@Mossaka Mossaka force-pushed the mossaka/unload-component2 branch from 7ce8fb9 to 4695802 Compare August 5, 2025 18:24
…component function

Signed-off-by: Aaron Wislang <aaron.wislang@microsoft.com>
@asw101

asw101 commented Aug 5, 2025

Copy link
Copy Markdown
Member

@Mossaka thank you for fixing the conflict. I've tested the functionality and it's working great. Just pushed the change to make lint happy and will merge momentarily.

@asw101 asw101 merged commit c426064 into main Aug 5, 2025
5 checks passed
@asw101 asw101 deleted the mossaka/unload-component2 branch August 5, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unload-component does not clean up files on disk

4 participants