feat: return host task from remove-host #250

jason-lynch · 2026-01-14T22:50:34Z

Summary

Updates the remove host workflow to track its status with a host-scoped task. This task is also now returned from the remove-host API endpoint.

Testing

Start the servers from this branch, then:

# resync your api client
cp1-api-sync

# initialize the cluster
cp-init

# stop host-2
cp-docker-exec stop host-2

# remove host-2
cp1-req remove-host host-2

# make sure to include the --force flag if there are any databases on host-2
cp1-req remove-host host-2 --force

# you should get a task ID in the output. now you can try all the host endpoints
cp1-req list-host-tasks host-2
cp1-req get-host-task host-2 <task ID>
cp1-req get-host-task-log host-2 <task ID>

# you should also see the host task in the list-tasks output
cp1-req list-tasks
cp1-req list-tasks --scope host
cp1-req list-tasks --scope host --entity-id host-2

PLAT-347

Summary by CodeRabbit

New Features
- Remove-host API response now includes a task field to track the overall progress of the removal operation.
Improvements
- Enhanced task tracking with parent-child relationships between host removal and database update tasks.
- Improved error handling and cancellation management during host removal operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-14T22:50:40Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

The pull request introduces a task-based tracking mechanism for the remove-host API operation. A new Task field is added to RemoveHostResponse, and the RemoveHost workflow is refactored to create a host-level removal task alongside per-database update tasks. The workflow implementation now includes explicit cancellation handling, error recovery, and ordered task state transitions.

Changes

Cohort / File(s)	Summary
API Schema `api/apiv1/design/host.go`	Added `task` attribute (type Task) to RemoveHostResponse; updated Required fields to include both "task" and "update_database_tasks".
Changelog `changes/unreleased/Changed-20260114-163406.yaml`	New changelog entry documenting that remove-host API response now includes a task field for tracking overall operation progress.
Test Suite `clustertest/cluster_test.go`	Added nil-check for `resp.Task` in Remove; added synchronous wait for host task completion via `c.client.WaitForHostTask()` before cleanup.
API Handler `server/internal/api/apiv1/post_init_handlers.go`	Changed `prepareDatabaseUpdates()` signature from returning `[]workflows.UpdateDatabaseInput` to `[]database.Spec`; inlined database removal validation and refactored to use `RemoveHost()` result mapping for task and database tasks.
Workflow Orchestration `server/internal/workflows/service.go`	Introduced `RemoveHostTasks` struct to group host task with database tasks; refactored `RemoveHost()` to create host-level task, per-database tasks with parent linkage, and return multi-task result instead of task list.
Workflow Implementation `server/internal/workflows/remove_host.go`	Reworked workflow with deferred cancellation detection, centralized error handler, task state initialization, and new `removeHostFromDatabases()` helper for parallel database sub-workflow orchestration; removed `DatabaseTaskIDs` field from RemoveHostInput.

Sequence Diagram

sequenceDiagram
    participant Client
    participant API as API Handler
    participant WfService as Workflow Service
    participant Workflow as RemoveHost Workflow
    participant Activity as Activity Layer

    Client->>API: POST /remove-host
    API->>WfService: RemoveHost(hostID, specs...)
    
    WfService->>WfService: Create host task
    WfService->>WfService: Create per-database tasks (linked to host task)
    WfService->>Workflow: Execute RemoveHost workflow
    
    Workflow->>Workflow: UpdateTask (mark started)
    Workflow->>Workflow: logTaskEvent (removal starting)
    
    Workflow->>Workflow: removeHostFromDatabases()
    Workflow->>Workflow: Execute parallel sub-workflows
    Workflow->>Workflow: logTaskEvent per database (success/failure)
    
    Workflow->>Activity: Remove host (activity execution)
    Activity-->>Workflow: Completion
    
    Workflow->>Workflow: UpdateTask (mark complete)
    Workflow->>Workflow: logTaskEvent (removal success)
    
    Workflow-->>WfService: RemoveHostTasks {host task, database tasks}
    WfService-->>API: RemoveHostTasks
    API-->>Client: RemoveHostResponse {task, update_database_tasks}
    
    Client->>API: Poll task status
    API-->>Client: Task status & progress

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 Hop, hop! With tasks aligned,
The host removal's task is signed,
Each database dances in the flow,
While progress tales do gently grow,
All tracked through workflows, intertwined!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat: return host task from remove-host' clearly summarizes the main change: returning a host task from the remove-host workflow/API endpoint.
Description check	✅ Passed	The PR description covers the summary, testing steps, and issue reference, though checklist items are not explicitly marked and documentation/breaking changes sections are omitted.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

jason-lynch · 2026-01-15T23:05:55Z

@coderabbitai review

coderabbitai · 2026-01-15T23:06:00Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@server/internal/workflows/service.go`:
- Around line 447-456: The CreateWorkflowInstance result is ignored so
hostTask.WorkflowInstanceID and hostTask.WorkflowExecutionID are never set;
update the call to s.client.CreateWorkflowInstance (used with
s.workflows.RemoveHost and RemoveHostInput) to capture the returned instance,
then assign instance.InstanceID and instance.ExecutionID (or the actual field
names returned by CreateWorkflowInstance) into hostTask.WorkflowInstanceID and
hostTask.WorkflowExecutionID before returning; mirror the pattern used in
createWorkflow at lines where workflow instance fields are populated so
subsequent abortTasks/status tracking can correlate the hostTask to its
workflow.

🧹 Nitpick comments (4)

server/internal/workflows/service.go (1)
430-431: Consider consolidating abort calls.

The two abortTasks calls could be combined into one to reduce verbosity.
♻️ Suggested refactor
-		s.abortTasks(ctx, hostTask)
-		s.abortTasks(ctx, databaseTasks...)
+		s.abortTasks(ctx, append([]*task.Task{hostTask}, databaseTasks...)...)
server/internal/workflows/remove_host.go (3)
37-49: Consider logging when task update fails in error handler.

Line 46 silently discards the error from updateTask. If this call fails, the task could remain in "started" state while the workflow has actually failed, leading to inconsistent task state that clients may observe.
♻️ Suggested improvement
-		_ = w.updateTask(ctx, logger, updateTaskInput)
+		if updateErr := w.updateTask(ctx, logger, updateTaskInput); updateErr != nil {
+			logger.With("update_error", updateErr).Error("failed to mark task as failed")
+		}
83-85: Consider reordering the "removing host" log message.

The "removing host" log entry is added after all database updates have completed. From a user's perspective, the task log would show:

Database update messages...

"removing host 'X'"

"successfully removed host 'X'"

It might be clearer to log "removing host" before the database updates begin, reflecting the overall operation start rather than just the final host removal activity.

120-121: Consider nil check for Spec field.

dbInput.Spec is a pointer that's dereferenced without a nil check. While the service layer likely validates this, a nil Spec would cause a panic here.
♻️ Defensive nil check
 for i, dbInput := range inputs {
+	if dbInput.Spec == nil {
+		return fmt.Errorf("update database input at index %d has nil spec", i)
+	}
 	logger.Info("creating update database sub-workflow", "database_id", dbInput.Spec.DatabaseID)

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e77c094 and b59faae.

⛔ Files ignored due to path filters (7)

api/apiv1/gen/control_plane/service.go is excluded by !**/gen/**
api/apiv1/gen/http/control_plane/client/types.go is excluded by !**/gen/**
api/apiv1/gen/http/control_plane/server/types.go is excluded by !**/gen/**
api/apiv1/gen/http/openapi.json is excluded by !**/gen/**
api/apiv1/gen/http/openapi.yaml is excluded by !**/gen/**
api/apiv1/gen/http/openapi3.json is excluded by !**/gen/**
api/apiv1/gen/http/openapi3.yaml is excluded by !**/gen/**

📒 Files selected for processing (6)

api/apiv1/design/host.go
changes/unreleased/Changed-20260114-163406.yaml
clustertest/cluster_test.go
server/internal/api/apiv1/post_init_handlers.go
server/internal/workflows/remove_host.go
server/internal/workflows/service.go

🧰 Additional context used

📓 Path-based instructions (5)

api/apiv1/design/**/*.go