Add delete option for servers and instances in VS Code extension

[Copilot]

The Drasi VS Code extension had no way to remove servers or instances once added — only "Add instance", "Add server", and "Edit server URL" were available in the context menu.

Changes

VS Code Extension

• ConnectionRegistry: Added deleteConnection(id) — removes the connection from settings and falls back to the next available connection if the deleted one was active
• DrasiClient: Added deleteInstance(id) — calls DELETE /api/v1/instances/:instanceId
• DrasiExplorer: Added deleteConnection and deleteInstance command handlers, both with confirmation dialogs before acting
• package.json: Registered drasi.connection.delete and drasi.instance.delete commands; wired into view/item/context under 3_danger group for connectionNode, connectionCurrentNode, and instanceNode

REST API

• DELETE /api/v1/instances/:instanceId: New endpoint that removes an instance from the registry, respects read-only mode, and cleans up persistence
• ConfigPersistence.unregister_instance: Clears all associated source, reaction, query, and instance configs on deletion
• OpenAPI spec updated to include the new endpoint

Tests

Added openapi_test.rs cases covering: endpoint presence in OpenAPI spec, successful deletion, not-found error response, and read-only mode rejection.

[Copilot]

Pull request overview

Adds delete functionality for servers and instances across the Drasi VS Code extension and Drasi Server REST API, including persistence cleanup and OpenAPI/test coverage.

Changes:

• Add DELETE /api/v1/instances/{instanceId} endpoint and wire it into the v1 router + OpenAPI.
• Implement persistence cleanup for deleted instances (instance + associated source/reaction/query configs).
• Add VS Code commands/UI for deleting servers and instances, plus OpenAPI integration tests for the new endpoint.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/openapi_test.rs	Adds OpenAPI/spec + functional tests for instance deletion behavior.
src/persistence.rs	Adds unregister_instance to remove per-instance persisted config data.
src/api/v1/routes.rs	Registers the instance delete route.
src/api/v1/openapi.rs	Includes the new handler in the OpenAPI paths list.
src/api/v1/mod.rs	Updates endpoint structure docs to mention delete.
src/api/v1/handlers.rs	Adds OpenAPI wrapper handler for delete instance.
src/api/shared/handlers.rs	Implements shared delete-instance logic (registry removal + persistence cleanup).
dev-tools/vscode/drasi-server/src/sdk/config.ts	Adds ConnectionRegistry.deleteConnection.
dev-tools/vscode/drasi-server/src/drasi-explorer.ts	Registers delete commands and adds confirmation/progress flows.
dev-tools/vscode/drasi-server/src/drasi-client.ts	Adds DrasiClient.deleteInstance.
dev-tools/vscode/drasi-server/package.json	Registers new commands + context menu entries.
dev-tools/vscode/drasi-server/package-lock.json	Updates extension lockfile metadata.

Files not reviewed (1)

• dev-tools/vscode/drasi-server/package-lock.json: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

delete_instance removes the instance from InstanceRegistry but never stops the underlying DrasiLib (the server shutdown path explicitly calls core.stop().await). This can leave background tasks/resources running after deletion. Consider calling core.stop().await (and handling/logging any error) before/while removing it from the registry so deletion actually tears down the instance.

[Copilot]

The OpenAPI annotation documents a 404 response for "Instance not found", but the handler always returns Json<ApiResponse<...>> (HTTP 200) with success: false on not-found (see shared delete_instance). Either update the implementation to return an actual 404 status, or update the OpenAPI responses(...) to match the real behavior so clients aren't misled.

Suggested change

	(status = 404, description = "Instance not found"),
	(status = 200, description = "Instance not found; returned as `success: false` in the ApiResponse body", body = ApiResponse),

[Copilot]

After deleting an instance, the extension may still have that instance set as the current instance (stored on the current connection). DrasiClient.getCurrentInstanceId() will then keep using the deleted ID and subsequent actions will fail until the user manually selects another instance. Consider clearing instanceId (or selecting the first remaining instance) when the deleted instance matches the current connection's instanceId.

[Copilot]

instanceId is interpolated directly into the URL path. If instance IDs can contain characters that need escaping (spaces, #, /, etc.), requests can break or be routed incorrectly. Consider URL-encoding the path segment (e.g., encodeURIComponent(instanceId)) when constructing the DELETE URL.

[danielgerlag]

Thanks for putting this together — the feature itself is well-scoped and the VS Code UX (confirmation dialogs, QuickPick fallback, progress notifications) is solid. The persistence cleanup in unregister_instance is clean too. There are a few issues that need addressing before this merges, mostly around the instance lifecycle and the API's error handling conventions.

---

Changes requested

🔴 Critical: Instance not shut down before removal

When registry.remove() is called, it simply drops the Arc<DrasiLib> from the HashMap. DrasiLib has no impl Drop, so the Tokio tasks backing all sources, queries, and reactions in the instance keep running — network connections stay open, the internal graph update loop is never aborted, and resources aren't cleanly released.

DrasiLib has a shutdown() method designed exactly for this case — it stops all components and aborts the graph update loop, performing a full teardown. Please call it on the removed instance before proceeding with persistence cleanup:

match registry.remove(&instance_id).await {
    Some(instance) => {
        // Cleanly shut down all sources, queries, reactions, and internal tasks
        if let Err(e) = instance.shutdown().await {
            warn!("Errors while shutting down instance '{instance_id}': {e}");
        }
        // ... persistence cleanup ...
    }
    None => { /* not found */ }
}

---

🔴 Critical: Handler returns 200 OK for all error cases

The delete_instance handler (both in src/api/shared/handlers.rs and src/api/v1/handlers.rs) returns Json<ApiResponse<StatusResponse>> unconditionally. This means "not found" and "read-only mode" both come back as HTTP 200 with success: false in the body, which violates the error handling architecture documented in CLAUDE.md and implemented throughout the rest of the codebase.

The established pattern is Result<Json<ApiResponse<T>>, ErrorResponse> — ErrorResponse implements IntoResponse and sets the correct HTTP status code automatically. Please align with this:

pub async fn delete_instance(...) -> Result<Json<ApiResponse<StatusResponse>>, ErrorResponse> {
    if *read_only {
        return Err(ErrorResponse::new(error_codes::CONFIG_READ_ONLY, "Server is in read-only mode"));
    }
    match registry.remove(&instance_id).await {
        Some(instance) => {
            instance.shutdown().await.ok();
            persist_after_operation(&config_persistence, "deleting instance").await?;
            Ok(Json(ApiResponse::success(StatusResponse { ... })))
        }
        None => Err(ErrorResponse::new(error_codes::INSTANCE_NOT_FOUND, format!("Instance '{instance_id}' not found"))),
    }
}

Also note: the persist_after_operation return value is currently ignored. It returns Result<(), ErrorResponse> and should be ?-propagated as shown above.

---

🟠 High: Tests validate the wrong HTTP status codes

The tests test_delete_instance_not_found_returns_error and test_delete_instance_read_only_rejected both assert StatusCode::OK for error responses (the comments even say "Response should be 200 with error body"). Once the handler return type is corrected, these should assert the proper codes — StatusCode::NOT_FOUND (404) for the missing instance case, and the appropriate code for read-only mode.

---

🟡 Medium: VS Code tree refreshes even when deletion fails

In drasi-explorer.ts, this.refresh() is called unconditionally after withProgress completes, regardless of whether the API call succeeded or threw. This means the tree view refreshes even when an error message is shown — which could be confusing since the instance is still there. Moving this.refresh() inside the try block (after the successful deleteInstance call) would make the behaviour clearer.

---

Happy to discuss any of these — let me know if anything needs clarification!