Skip to content

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

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

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

Conversation

@Mossaka
Copy link
Copy Markdown
Collaborator

@Mossaka Mossaka commented Aug 4, 2025

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.

Copy link
Copy Markdown
Collaborator

@thomastaylor312 thomastaylor312 left a comment

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.

@Mossaka Mossaka requested a review from Copilot August 5, 2025 07:40
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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

self.remove_file_if_exists(&policy_path, "policy file", id)
.await?;

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

Copilot AI Aug 5, 2025

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.
component_file.display()
))
.component_policies
.remove(id);
Copy link

Copilot AI Aug 5, 2025

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
Copy link
Copy Markdown
Member

asw101 commented Aug 5, 2025

@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
Copy link
Copy Markdown
Collaborator Author

Mossaka commented Aug 5, 2025

@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
Copy link
Copy Markdown
Member

asw101 commented Aug 5, 2025

@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