Skip to content

Fix deploy.delete compatibility for model-name stop actions#57

Merged
skyguan92 merged 2 commits into
Approaching-AI:developfrom
xu16601526267:fix/stop-button-backend
May 14, 2026
Merged

Fix deploy.delete compatibility for model-name stop actions#57
skyguan92 merged 2 commits into
Approaching-AI:developfrom
xu16601526267:fix/stop-button-backend

Conversation

@xu16601526267
Copy link
Copy Markdown
Contributor

@xu16601526267 xu16601526267 commented May 9, 2026

Summary

  • make deploy.delete resolve an active deployment by model name when no direct deployment-name match is found
  • keep deletion behavior unchanged once the concrete deployment name is resolved
  • add a regression test covering the UI-style model-name stop path

Why

Some UI stop actions surface the model name rather than the concrete deployment name. Before this change, deploy.delete failed with deployment not found in that case even though the deployment was running.

Testing

  • go test ./cmd/aima -run 'TestDeployDelete|TestAutomationToolAdapterAllowsDeployDelete'

Copy link
Copy Markdown
Contributor

@skyguan92 skyguan92 left a comment

Choose a reason for hiding this comment

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

I found one correctness issue before merge.

DeployDelete now resolves model-name input to modelStatus.Name and deletes that concrete deployment, but the post-delete verification still calls findMatchingDeployments(ctx, name, ...) with the original model-name query. In the fallback scenario, that original query is exactly the one that failed normal matching. If Delete returns nil while the deployment remains active/listed/status-resolvable, this verification can return no matches, then the code proceeds to tombstone the deployment and remove proxy backends even though the runtime still has it.

Please verify the resolved deployment name(s) after delete, or carry a verification query set that includes each match.Status.Name. It would also be worth adding a regression where fallback resolution succeeds but Delete leaves the resolved deployment active.

@skyguan92
Copy link
Copy Markdown
Contributor

Blocking issue still exists in the fallback path.

DeployDelete resolves model-name input via findDeploymentStatus(ctx, name, ...), then deletes match.Status.Name, but the post-delete verification still checks only findMatchingDeployments(ctx, name, ...) with the original query.

That can miss a still-active resolved deployment when the original model-name input is only accepted by runtime Status(modelName) aliasing and the returned status does not itself match deploymentMatchesQuery(status, modelName). In that case Delete can return nil while the concrete deployment is still active, and the code will still remove proxy backends and write tombstones.

I verified this with a local reproducer on the PR merge result:

func TestDeployDeleteFallbackFailsWhenResolvedDeploymentStillActive(t *testing.T) {
    // Status("qwen3.6-35b-a3b") aliases to "qwen3-6-35b-a3b-sglang".
    // Delete("qwen3-6-35b-a3b-sglang") returns nil but keeps the deployment active.
    // Current code returns nil; expected a "still active after delete" error.
}

The reproducer fails as:

DeployDelete error = nil, want verification failure

Suggested fix: carry the resolved deployment names into verification, at minimum checking each match.Status.Name after delete. A verification query set containing the original input, each resolved deployment name, and deploymentModelKey(match.Status) would also cover the model-key path. Please add a regression for the fallback-resolved deployment remaining active after Delete returns nil.

Copy link
Copy Markdown
Contributor

@skyguan92 skyguan92 left a comment

Choose a reason for hiding this comment

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

Re-reviewed latest head 093b02e. The fallback delete verification issue from my earlier review is now covered by resolved-name verification and regression tests. Local verification: go test ./... passed on the PR merge ref.

@skyguan92 skyguan92 merged commit dd7331c into Approaching-AI:develop May 14, 2026
1 check passed
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.

3 participants