From b2e4a5af484d9c7d00d25a6bd4752b3ef3c29e98 Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Wed, 29 Jul 2026 18:28:07 -0300 Subject: [PATCH 1/7] Add taskworker: one poll loop for every worker flavour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The poll→execute→update loop was hand-copied four times in cmd/worker.go, none of the copies having backoff, context cancellation, or tests. This adds the shared implementation the flavours will move onto. The loop: - backs off between empty or failed polls, interruptibly. Without this the four cmd/worker.go loops spin: a test that removes the backoff records 36 million polls in 220ms against 10 with it. - fails a task individually when it cannot be converted, rather than discarding its whole batch - reports results on a context detached from the loop, so work finishing during Ctrl-C is not abandoned - turns a handler panic into a failed task instead of a dead worker Status is an open string type rather than an enum: JavaScript workers forward whatever their script returns, and FAILED_WITH_TERMINAL_ERROR is documented and in use. Handler returns only a Result, with no error alongside it, because the flavours shape failures differently and workflows can observe the difference. ToTaskResult is a pure function so the mapping most at risk of drifting during the port can be table-tested against all three pre-existing result shapes. StdioHandler and GojaHandler carry the two execution models over unchanged, including the quirk where an unrecognised stdio status reports "invalid status from worker: FAILED" rather than naming the offending value. Co-Authored-By: Claude Opus 5 (1M context) --- internal/taskworker/goja.go | 255 +++++++++++++++++++ internal/taskworker/goja_test.go | 312 +++++++++++++++++++++++ internal/taskworker/runner.go | 133 ++++++++++ internal/taskworker/runner_test.go | 202 +++++++++++++++ internal/taskworker/stdio.go | 174 +++++++++++++ internal/taskworker/stdio_test.go | 279 ++++++++++++++++++++ internal/taskworker/taskworker.go | 242 ++++++++++++++++++ internal/taskworker/taskworker_test.go | 338 +++++++++++++++++++++++++ 8 files changed, 1935 insertions(+) create mode 100644 internal/taskworker/goja.go create mode 100644 internal/taskworker/goja_test.go create mode 100644 internal/taskworker/runner.go create mode 100644 internal/taskworker/runner_test.go create mode 100644 internal/taskworker/stdio.go create mode 100644 internal/taskworker/stdio_test.go create mode 100644 internal/taskworker/taskworker.go create mode 100644 internal/taskworker/taskworker_test.go diff --git a/internal/taskworker/goja.go b/internal/taskworker/goja.go new file mode 100644 index 0000000..3e2f8c5 --- /dev/null +++ b/internal/taskworker/goja.go @@ -0,0 +1,255 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "context" + "crypto/md5" + "crypto/sha1" + "crypto/sha256" + "encoding/base64" + "encoding/hex" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strings" + "time" + + "github.com/dop251/goja" + log "github.com/sirupsen/logrus" +) + +// gojaResult is the JSON contract a JavaScript worker returns from its script. +type gojaResult struct { + Status string `json:"status"` + Body map[string]interface{} `json:"body"` +} + +// GojaHandler runs a JavaScript worker in the CLI's embedded interpreter. +// +// The program is compiled once and each task gets a fresh goja.Runtime: Runtimes are not +// safe for concurrent use, and a Handler is shared across the goroutines of a batch poll. +type GojaHandler struct { + program *goja.Program +} + +// NewGojaHandler compiles script for repeated execution. name appears in stack traces. +func NewGojaHandler(script, name string) (*GojaHandler, error) { + program, err := goja.Compile(name, script, false) + if err != nil { + return nil, fmt.Errorf("error compiling JavaScript worker: %w", err) + } + return &GojaHandler{program: program}, nil +} + +func (h *GojaHandler) Handle(ctx context.Context, t Task) Result { + log.Infof("Processing task: %s (workflow: %s)", t.ID, t.WorkflowID) + + vm := goja.New() + + var taskObj interface{} + if err := json.Unmarshal(t.Raw, &taskObj); err != nil { + log.Errorf("Error unmarshaling task: %v", err) + return gojaFailure(fmt.Sprintf("Error unmarshaling task: %v", err)) + } + + dollarObj := vm.NewObject() + if err := dollarObj.Set("task", taskObj); err != nil { + log.Errorf("Error setting task in $: %v", err) + return gojaFailure(fmt.Sprintf("Error setting task: %v", err)) + } + if err := vm.Set("$", dollarObj); err != nil { + log.Errorf("Error setting $ object: %v", err) + return gojaFailure(fmt.Sprintf("Error setting $ object: %v", err)) + } + + injectUtilities(vm) + + value, err := vm.RunProgram(h.program) + if err != nil { + log.Errorf("Error executing script for task %s: %v", t.ID, err) + return gojaFailure(fmt.Sprintf("Script execution error: %v", err)) + } + + return gojaResultToResult(value) +} + +// gojaFailure builds the failure shape JavaScript workers have always produced: the +// message lands under an "error" output key rather than in ReasonForIncompletion, which +// workflows may read as ${task.output.error}. +func gojaFailure(reason string) Result { + return Result{Status: StatusFailed, Output: map[string]interface{}{"error": reason}} +} + +// gojaResultToResult interprets whatever the script returned. +// +// A script may return {status, body}, or any other value, or nothing at all; each case +// has an established meaning that workflows depend on. Note that the status is passed +// through verbatim — unlike stdio workers, JavaScript workers may return statuses the +// CLI does not model, such as FAILED_WITH_TERMINAL_ERROR. +func gojaResultToResult(value goja.Value) Result { + if value == nil || goja.IsUndefined(value) || goja.IsNull(value) { + return Result{Status: StatusCompleted, Output: map[string]interface{}{}} + } + + exported := value.Export() + resultBytes, err := json.Marshal(exported) + if err != nil { + log.Errorf("Error marshaling script result: %v", err) + return Result{Status: StatusCompleted, Output: map[string]interface{}{}} + } + + var parsed gojaResult + if err := json.Unmarshal(resultBytes, &parsed); err != nil || parsed.Status == "" { + log.Warnf("Script result not in expected format, treating as completed") + return Result{Status: StatusCompleted, Output: map[string]interface{}{"result": exported}} + } + + body := parsed.Body + if body == nil { + body = make(map[string]interface{}) + } + return Result{Status: Status(parsed.Status), Output: body} +} + +func injectUtilities(vm *goja.Runtime) { + // HTTP utilities + httpObj := vm.NewObject() + httpObj.Set("get", func(url string, headers map[string]interface{}) map[string]interface{} { + return httpRequest("GET", url, headers, "") + }) + httpObj.Set("post", func(url string, headers map[string]interface{}, body string) map[string]interface{} { + return httpRequest("POST", url, headers, body) + }) + httpObj.Set("put", func(url string, headers map[string]interface{}, body string) map[string]interface{} { + return httpRequest("PUT", url, headers, body) + }) + httpObj.Set("delete", func(url string, headers map[string]interface{}) map[string]interface{} { + return httpRequest("DELETE", url, headers, "") + }) + vm.Set("http", httpObj) + + // Crypto utilities + cryptoObj := vm.NewObject() + cryptoObj.Set("md5", func(text string) string { + hash := md5.Sum([]byte(text)) + return hex.EncodeToString(hash[:]) + }) + cryptoObj.Set("sha1", func(text string) string { + hash := sha1.Sum([]byte(text)) + return hex.EncodeToString(hash[:]) + }) + cryptoObj.Set("sha256", func(text string) string { + hash := sha256.Sum256([]byte(text)) + return hex.EncodeToString(hash[:]) + }) + cryptoObj.Set("base64Encode", func(text string) string { + return base64.StdEncoding.EncodeToString([]byte(text)) + }) + cryptoObj.Set("base64Decode", func(text string) string { + decoded, err := base64.StdEncoding.DecodeString(text) + if err != nil { + return "" + } + return string(decoded) + }) + vm.Set("crypto", cryptoObj) + + // Utility functions + utilObj := vm.NewObject() + utilObj.Set("sleep", func(ms int) { + time.Sleep(time.Duration(ms) * time.Millisecond) + }) + utilObj.Set("uuid", func() string { + return fmt.Sprintf("%d-%d", time.Now().UnixNano(), os.Getpid()) + }) + utilObj.Set("env", func(key string) string { + return os.Getenv(key) + }) + vm.Set("util", utilObj) + + // String utilities + stringObj := vm.NewObject() + stringObj.Set("toUpper", strings.ToUpper) + stringObj.Set("toLower", strings.ToLower) + stringObj.Set("trim", strings.TrimSpace) + stringObj.Set("split", func(s, sep string) []string { + return strings.Split(s, sep) + }) + stringObj.Set("join", func(arr []string, sep string) string { + return strings.Join(arr, sep) + }) + stringObj.Set("replace", func(s, old, new string) string { + return strings.ReplaceAll(s, old, new) + }) + stringObj.Set("contains", strings.Contains) + stringObj.Set("hasPrefix", strings.HasPrefix) + stringObj.Set("hasSuffix", strings.HasSuffix) + vm.Set("str", stringObj) +} + +func httpRequest(method, url string, headers map[string]interface{}, body string) map[string]interface{} { + var bodyReader io.Reader + if body != "" { + bodyReader = strings.NewReader(body) + } + + req, err := http.NewRequest(method, url, bodyReader) + if err != nil { + return map[string]interface{}{ + "error": err.Error(), + "status": 0, + } + } + + for key, value := range headers { + if strVal, ok := value.(string); ok { + req.Header.Set(key, strVal) + } + } + + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Do(req) + if err != nil { + return map[string]interface{}{ + "error": err.Error(), + "status": 0, + } + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return map[string]interface{}{ + "error": err.Error(), + "status": resp.StatusCode, + } + } + + var jsonBody interface{} + if err := json.Unmarshal(respBody, &jsonBody); err == nil { + return map[string]interface{}{ + "status": resp.StatusCode, + "body": jsonBody, + "text": string(respBody), + } + } + + return map[string]interface{}{ + "status": resp.StatusCode, + "text": string(respBody), + } +} diff --git a/internal/taskworker/goja_test.go b/internal/taskworker/goja_test.go new file mode 100644 index 0000000..e14edd0 --- /dev/null +++ b/internal/taskworker/goja_test.go @@ -0,0 +1,312 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +// Tests for the JavaScript worker runtime and the two on-the-wire result shapes. These +// moved here with injectUtilities and httpRequest when the worker flavours converged onto +// one poll loop; they previously lived in cmd/worker_test.go. +package taskworker + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/dop251/goja" +) + +func TestStdioResultJSON(t *testing.T) { + tests := []struct { + name string + input string + status string + hasOut bool + }{ + { + name: "completed with output", + input: `{"status":"COMPLETED","output":{"key":"value"},"logs":["done"]}`, + status: "COMPLETED", + hasOut: true, + }, + { + name: "failed with reason", + input: `{"status":"FAILED","reason":"timeout"}`, + status: "FAILED", + hasOut: false, + }, + { + name: "in progress", + input: `{"status":"IN_PROGRESS"}`, + status: "IN_PROGRESS", + hasOut: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var result stdioResult + err := json.Unmarshal([]byte(tt.input), &result) + if err != nil { + t.Fatalf("unmarshal failed: %v", err) + } + if result.Status != tt.status { + t.Errorf("Status: got %q, want %q", result.Status, tt.status) + } + if tt.hasOut && result.Output == nil { + t.Error("expected non-nil output") + } + }) + } +} + +func TestGojaResultJSON(t *testing.T) { + result := gojaResult{ + Status: "COMPLETED", + Body: map[string]interface{}{ + "message": "hello", + }, + } + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("marshal failed: %v", err) + } + + var decoded gojaResult + if err := json.Unmarshal(data, &decoded); err != nil { + t.Fatalf("unmarshal failed: %v", err) + } + + if decoded.Status != "COMPLETED" { + t.Errorf("Status: got %q, want %q", decoded.Status, "COMPLETED") + } + if decoded.Body["message"] != "hello" { + t.Errorf("Body.message: got %v, want %q", decoded.Body["message"], "hello") + } +} + +func TestHttpRequest(t *testing.T) { + t.Run("GET request", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Errorf("expected GET, got %s", r.Method) + } + if r.Header.Get("X-Custom") != "test" { + t.Errorf("missing custom header") + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"result":"ok"}`)) + })) + defer server.Close() + + result := httpRequest("GET", server.URL, map[string]interface{}{"X-Custom": "test"}, "") + if result["status"] != http.StatusOK { + t.Errorf("status: got %v, want %d", result["status"], http.StatusOK) + } + body, ok := result["body"].(map[string]interface{}) + if !ok { + t.Fatal("expected body to be a map") + } + if body["result"] != "ok" { + t.Errorf("body.result: got %v, want %q", body["result"], "ok") + } + }) + + t.Run("POST request with body", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" { + t.Errorf("expected POST, got %s", r.Method) + } + w.Write([]byte(`{"created":true}`)) + })) + defer server.Close() + + result := httpRequest("POST", server.URL, nil, `{"name":"test"}`) + if result["status"] != http.StatusOK { + t.Errorf("status: got %v, want %d", result["status"], http.StatusOK) + } + }) + + t.Run("non-JSON response", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("plain text response")) + })) + defer server.Close() + + result := httpRequest("GET", server.URL, nil, "") + if result["text"] != "plain text response" { + t.Errorf("text: got %v, want %q", result["text"], "plain text response") + } + if result["body"] != nil { + t.Errorf("expected nil body for non-JSON, got %v", result["body"]) + } + }) + + t.Run("connection error", func(t *testing.T) { + result := httpRequest("GET", "http://localhost:1", nil, "") + if result["error"] == nil { + t.Error("expected error for connection failure") + } + if result["status"] != 0 { + t.Errorf("status: got %v, want 0", result["status"]) + } + }) +} + +func TestInjectUtilitiesCrypto(t *testing.T) { + vm := goja.New() + injectUtilities(vm) + + tests := []struct { + name string + script string + want string + }{ + {"md5", `crypto.md5("hello")`, "5d41402abc4b2a76b9719d911017c592"}, + {"sha1", `crypto.sha1("hello")`, "aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d"}, + {"sha256", `crypto.sha256("hello")`, "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"}, + {"base64Encode", `crypto.base64Encode("hello world")`, "aGVsbG8gd29ybGQ="}, + {"base64Decode", `crypto.base64Decode("aGVsbG8gd29ybGQ=")`, "hello world"}, + {"base64Decode invalid", `crypto.base64Decode("!!!invalid!!!")`, ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + val, err := vm.RunString(tt.script) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() != tt.want { + t.Errorf("got %q, want %q", val.String(), tt.want) + } + }) + } +} + +func TestInjectUtilitiesString(t *testing.T) { + vm := goja.New() + injectUtilities(vm) + + tests := []struct { + name string + script string + want string + }{ + {"toUpper", `str.toUpper("hello")`, "HELLO"}, + {"toLower", `str.toLower("WORLD")`, "world"}, + {"trim", `str.trim(" spaces ")`, "spaces"}, + {"contains true", `str.contains("hello world", "world")`, "true"}, + {"contains false", `str.contains("hello", "xyz")`, "false"}, + {"hasPrefix", `str.hasPrefix("hello", "hel")`, "true"}, + {"hasSuffix", `str.hasSuffix("hello", "llo")`, "true"}, + {"replace", `str.replace("foo bar foo", "foo", "baz")`, "baz bar baz"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + val, err := vm.RunString(tt.script) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() != tt.want { + t.Errorf("got %q, want %q", val.String(), tt.want) + } + }) + } +} + +func TestInjectUtilitiesSplit(t *testing.T) { + vm := goja.New() + injectUtilities(vm) + + val, err := vm.RunString(`JSON.stringify(str.split("a,b,c", ","))`) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() != `["a","b","c"]` { + t.Errorf("got %q, want %q", val.String(), `["a","b","c"]`) + } +} + +func TestInjectUtilitiesJoin(t *testing.T) { + vm := goja.New() + injectUtilities(vm) + + val, err := vm.RunString(`str.join(["a","b","c"], "-")`) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() != "a-b-c" { + t.Errorf("got %q, want %q", val.String(), "a-b-c") + } +} + +func TestInjectUtilitiesEnv(t *testing.T) { + vm := goja.New() + injectUtilities(vm) + + os.Setenv("TEST_CONDUCTOR_VAR", "test_value") + defer os.Unsetenv("TEST_CONDUCTOR_VAR") + + val, err := vm.RunString(`util.env("TEST_CONDUCTOR_VAR")`) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() != "test_value" { + t.Errorf("got %q, want %q", val.String(), "test_value") + } + + // Non-existent env var + val, err = vm.RunString(`util.env("NONEXISTENT_VAR_12345")`) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() != "" { + t.Errorf("got %q, want empty string", val.String()) + } +} + +func TestInjectUtilitiesUUID(t *testing.T) { + vm := goja.New() + injectUtilities(vm) + + val, err := vm.RunString(`util.uuid()`) + if err != nil { + t.Fatalf("script error: %v", err) + } + if val.String() == "" { + t.Error("expected non-empty UUID") + } +} + +func TestInjectUtilitiesHTTP(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"method":"` + r.Method + `"}`)) + })) + defer server.Close() + + vm := goja.New() + injectUtilities(vm) + + // Test http.get + val, err := vm.RunString(`JSON.stringify(http.get("` + server.URL + `", {}))`) + if err != nil { + t.Fatalf("script error: %v", err) + } + result := val.String() + if result == "" { + t.Error("expected non-empty result") + } +} diff --git a/internal/taskworker/runner.go b/internal/taskworker/runner.go new file mode 100644 index 0000000..e6af50c --- /dev/null +++ b/internal/taskworker/runner.go @@ -0,0 +1,133 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/antihax/optional" + "github.com/conductor-sdk/conductor-go/sdk/client" + "github.com/conductor-sdk/conductor-go/sdk/model" +) + +// RunnerOptions carries the poll and reporting settings that the worker commands expose +// as flags. Each poll field is applied only when set, because always sending a field — +// an empty Workerid, for instance — would change the outgoing request. +type RunnerOptions struct { + WorkerID string + Domain string + Count int32 + PollTimeoutMs int32 + // UseTaskWorkerID reports the polled task's own worker id on the result instead of + // WorkerID. JavaScript workers behave this way and workflows can observe it. + UseTaskWorkerID bool +} + +// conductorRunner adapts Conductor's TaskResourceApiService to Runner. It is the ONLY +// place model.* and *client.TaskResourceApiService appear in this package — the loop and +// the handlers see only taskworker.Task and taskworker.Result. +type conductorRunner struct { + client *client.TaskResourceApiService + opts RunnerOptions +} + +// NewConductorRunner returns a Runner backed by the Conductor task client, which the cmd +// layer supplies via internal.GetTaskClient(). +func NewConductorRunner(taskClient *client.TaskResourceApiService, opts RunnerOptions) Runner { + return &conductorRunner{client: taskClient, opts: opts} +} + +func (r *conductorRunner) Poll(ctx context.Context, taskType string) ([]PolledTask, error) { + opts := &client.TaskResourceApiBatchPollOpts{} + if r.opts.WorkerID != "" { + opts.Workerid = optional.NewString(r.opts.WorkerID) + } + if r.opts.Domain != "" { + opts.Domain = optional.NewString(r.opts.Domain) + } + if r.opts.Count > 0 { + opts.Count = optional.NewInt32(r.opts.Count) + } + if r.opts.PollTimeoutMs > 0 { + opts.Timeout = optional.NewInt32(r.opts.PollTimeoutMs) + } + + tasks, _, err := r.client.BatchPoll(ctx, taskType, opts) + if err != nil { + return nil, err + } + + polled := make([]PolledTask, 0, len(tasks)) + for _, t := range tasks { + polled = append(polled, taskFromModel(t)) + } + return polled, nil +} + +func (r *conductorRunner) Update(ctx context.Context, t Task, res Result) error { + _, _, err := r.client.UpdateTask(ctx, ToTaskResult(t, res, r.opts)) + return err +} + +// taskFromModel converts one SDK task, carrying any conversion failure on the task +// itself so the loop can fail it individually instead of dropping its batch peers. +func taskFromModel(t model.Task) PolledTask { + task := Task{ + ID: t.TaskId, + WorkflowID: t.WorkflowInstanceId, + Type: t.TaskType, + WorkerID: t.WorkerId, + } + + raw, err := json.Marshal(t) + if err != nil { + return PolledTask{Task: task, Err: fmt.Errorf("marshal task: %w", err)} + } + task.Raw = raw + return PolledTask{Task: task} +} + +// ToTaskResult maps a Result onto the SDK's TaskResult. It is a pure function so the +// mapping — the part most likely to drift during a refactor — can be table-tested +// without a live client. +func ToTaskResult(t Task, r Result, opts RunnerOptions) *model.TaskResult { + result := &model.TaskResult{ + TaskId: t.ID, + WorkflowInstanceId: t.WorkflowID, + Status: model.TaskResultStatus(r.Status), + OutputData: r.Output, + } + + if opts.UseTaskWorkerID { + result.WorkerId = t.WorkerID + } else if opts.WorkerID != "" { + result.WorkerId = opts.WorkerID + } + + if r.Reason != "" { + result.ReasonForIncompletion = r.Reason + } + + if len(r.Logs) > 0 { + logs := make([]model.TaskExecLog, len(r.Logs)) + for i, line := range r.Logs { + logs[i] = model.TaskExecLog{Log: line} + } + result.Logs = logs + } + + return result +} diff --git a/internal/taskworker/runner_test.go b/internal/taskworker/runner_test.go new file mode 100644 index 0000000..b0a8d64 --- /dev/null +++ b/internal/taskworker/runner_test.go @@ -0,0 +1,202 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "encoding/json" + "testing" + + "github.com/conductor-sdk/conductor-go/sdk/model" +) + +// TestToTaskResultPreservesFlavourShapes pins the three result shapes that existed before +// the worker flavours were converged onto one loop. Workflows can observe all three, so a +// change here is a breaking change rather than a refactor. +func TestToTaskResultPreservesFlavourShapes(t *testing.T) { + polled := Task{ID: "t1", WorkflowID: "wf1", Type: "greet", WorkerID: "worker-from-task"} + + tests := []struct { + name string + result Result + opts RunnerOptions + want model.TaskResult + }{ + { + // worker js: reports the polled task's own worker id, and a failure carries + // the message under an "error" output key with no ReasonForIncompletion. + name: "js failure shape", + result: Result{Status: StatusFailed, Output: map[string]interface{}{"error": "script blew up"}}, + opts: RunnerOptions{UseTaskWorkerID: true}, + want: model.TaskResult{ + TaskId: "t1", + WorkflowInstanceId: "wf1", + WorkerId: "worker-from-task", + Status: model.FailedTask, + OutputData: map[string]interface{}{"error": "script blew up"}, + }, + }, + { + // worker stdio: uses the configured --worker-id, and a failure carries + // ReasonForIncompletion plus logs and no "error" key. + name: "stdio failure shape", + result: Result{Status: StatusFailed, Reason: "exit 1", Logs: []string{"stderr line"}}, + opts: RunnerOptions{WorkerID: "my-worker"}, + want: model.TaskResult{ + TaskId: "t1", + WorkflowInstanceId: "wf1", + WorkerId: "my-worker", + Status: model.FailedTask, + ReasonForIncompletion: "exit 1", + Logs: []model.TaskExecLog{{Log: "stderr line"}}, + }, + }, + { + // skill tools: constant worker id, output wrapped under "result". + name: "skill success shape", + result: Result{Status: StatusCompleted, Output: map[string]interface{}{"result": "Hello\n"}}, + opts: RunnerOptions{WorkerID: "conductor-cli"}, + want: model.TaskResult{ + TaskId: "t1", + WorkflowInstanceId: "wf1", + WorkerId: "conductor-cli", + Status: model.CompletedTask, + OutputData: map[string]interface{}{"result": "Hello\n"}, + }, + }, + { + // An unset --worker-id must leave WorkerId empty rather than sending "". + name: "no worker id configured", + result: Result{Status: StatusCompleted}, + opts: RunnerOptions{}, + want: model.TaskResult{ + TaskId: "t1", + WorkflowInstanceId: "wf1", + Status: model.CompletedTask, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ToTaskResult(polled, tt.result, tt.opts) + assertTaskResultEqual(t, got, tt.want) + }) + } +} + +// TestToTaskResultPassesStatusThrough covers the open Status type: worker js forwards +// whatever status its script returned, including values the CLI does not model. +func TestToTaskResultPassesStatusThrough(t *testing.T) { + tests := []Status{ + StatusCompleted, + StatusFailed, + StatusInProgress, + StatusFailedWithTerminalError, + Status("SOMETHING_THE_CLI_DOES_NOT_KNOW"), + } + + for _, status := range tests { + t.Run(string(status), func(t *testing.T) { + got := ToTaskResult(Task{ID: "t1"}, Result{Status: status}, RunnerOptions{}) + if string(got.Status) != string(status) { + t.Errorf("Status = %q, want %q — status must pass through unchanged", got.Status, status) + } + }) + } +} + +func TestToTaskResultOmitsEmptyLogsAndReason(t *testing.T) { + got := ToTaskResult(Task{ID: "t1"}, Result{Status: StatusCompleted}, RunnerOptions{}) + if got.Logs != nil { + t.Errorf("Logs = %v, want nil", got.Logs) + } + if got.ReasonForIncompletion != "" { + t.Errorf("ReasonForIncompletion = %q, want empty", got.ReasonForIncompletion) + } +} + +func TestTaskFromModelCarriesFullTaskAsRaw(t *testing.T) { + polled := taskFromModel(model.Task{ + TaskId: "t1", + WorkflowInstanceId: "wf1", + TaskType: "greet", + WorkerId: "w1", + InputData: map[string]interface{}{"name": "Miguel"}, + }) + + if polled.Err != nil { + t.Fatalf("conversion error = %v", polled.Err) + } + if polled.Task.ID != "t1" || polled.Task.WorkflowID != "wf1" || polled.Task.Type != "greet" { + t.Errorf("identity fields wrong: %+v", polled.Task) + } + if polled.Task.WorkerID != "w1" { + t.Errorf("WorkerID = %q, want w1 — worker js reports this back", polled.Task.WorkerID) + } + + // Raw must be the whole task, since goja exposes it as $.task and stdio writes it to + // the child's stdin. Both would break if it were only inputData. + var raw map[string]interface{} + if err := json.Unmarshal(polled.Task.Raw, &raw); err != nil { + t.Fatalf("Raw is not valid JSON: %v", err) + } + for _, key := range []string{"taskId", "workflowInstanceId", "taskType", "inputData"} { + if _, ok := raw[key]; !ok { + t.Errorf("Raw is missing %q — it must be the full task, not just inputData", key) + } + } + + input, err := polled.Task.InputData() + if err != nil { + t.Fatalf("InputData() error = %v", err) + } + if string(input) != `{"name":"Miguel"}` { + t.Errorf("InputData() = %s", input) + } +} + +func assertTaskResultEqual(t *testing.T, got *model.TaskResult, want model.TaskResult) { + t.Helper() + if got.TaskId != want.TaskId { + t.Errorf("TaskId = %q, want %q", got.TaskId, want.TaskId) + } + if got.WorkflowInstanceId != want.WorkflowInstanceId { + t.Errorf("WorkflowInstanceId = %q, want %q", got.WorkflowInstanceId, want.WorkflowInstanceId) + } + if got.WorkerId != want.WorkerId { + t.Errorf("WorkerId = %q, want %q", got.WorkerId, want.WorkerId) + } + if got.Status != want.Status { + t.Errorf("Status = %q, want %q", got.Status, want.Status) + } + if got.ReasonForIncompletion != want.ReasonForIncompletion { + t.Errorf("ReasonForIncompletion = %q, want %q", got.ReasonForIncompletion, want.ReasonForIncompletion) + } + if len(got.Logs) != len(want.Logs) { + t.Fatalf("len(Logs) = %d, want %d", len(got.Logs), len(want.Logs)) + } + for i := range want.Logs { + if got.Logs[i].Log != want.Logs[i].Log { + t.Errorf("Logs[%d] = %q, want %q", i, got.Logs[i].Log, want.Logs[i].Log) + } + } + if len(got.OutputData) != len(want.OutputData) { + t.Fatalf("len(OutputData) = %d, want %d", len(got.OutputData), len(want.OutputData)) + } + for k, v := range want.OutputData { + if got.OutputData[k] != v { + t.Errorf("OutputData[%q] = %v, want %v", k, got.OutputData[k], v) + } + } +} diff --git a/internal/taskworker/stdio.go b/internal/taskworker/stdio.go new file mode 100644 index 0000000..720688c --- /dev/null +++ b/internal/taskworker/stdio.go @@ -0,0 +1,174 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "os" + "os/exec" + "time" + + log "github.com/sirupsen/logrus" +) + +// stdioResult is the JSON contract a stdio worker writes to its stdout. +type stdioResult struct { + Status string `json:"status"` + Output map[string]interface{} `json:"output,omitempty"` + Logs []string `json:"logs,omitempty"` + Reason string `json:"reason,omitempty"` +} + +// StdioOptions configures a StdioHandler. +type StdioOptions struct { + // Command and Args are the worker program to run, once per task. + Command string + Args []string + // Env is appended to the child's environment. It is passed in rather than read from + // viper so this package does not depend on process-global config, and so tests can + // assert what the child receives without mutating global state. + Env []string + // Domain, when set, is exported to the child as POLL_DOMAIN. + Domain string + // ExecTimeout bounds a single task's execution. Zero means no timeout. + ExecTimeout time.Duration + // Verbose prints the task JSON and the result JSON to stdout. + Verbose bool +} + +// StdioHandler runs an external program per task: the full task JSON goes in on stdin, +// a result JSON comes back on stdout. It is safe for concurrent use — each Handle call +// builds its own command and buffers. +type StdioHandler struct { + opts StdioOptions +} + +// NewStdioHandler returns a Handler that executes opts.Command for each task. +func NewStdioHandler(opts StdioOptions) *StdioHandler { + return &StdioHandler{opts: opts} +} + +func (h *StdioHandler) Handle(ctx context.Context, t Task) Result { + log.Infof("Processing task: %s (workflow: %s)", t.ID, t.WorkflowID) + + if h.opts.Verbose { + fmt.Println("=== Task Input ===") + fmt.Println(string(t.Raw)) + fmt.Println("==================") + } + + if h.opts.ExecTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, h.opts.ExecTimeout) + defer cancel() + } + + cmd := exec.CommandContext(ctx, h.opts.Command, h.opts.Args...) + cmd.Env = append(cmd.Environ(), + "TASK_TYPE="+t.Type, + "TASK_ID="+t.ID, + "WORKFLOW_ID="+t.WorkflowID, + "EXECUTION_ID="+t.WorkflowID, + ) + if h.opts.Domain != "" { + cmd.Env = append(cmd.Env, "POLL_DOMAIN="+h.opts.Domain) + } + cmd.Env = append(cmd.Env, h.opts.Env...) + + cmd.Stdin = bytes.NewReader(t.Raw) + + // The child's streams are both captured and echoed, so a worker's own output stays + // visible in the terminal while still being available for parsing and for logs. + var stdout, stderr bytes.Buffer + cmd.Stdout = io.MultiWriter(&stdout, os.Stdout) + cmd.Stderr = io.MultiWriter(&stderr, os.Stderr) + + result := h.runAndParse(cmd, &stdout, &stderr) + + if h.opts.Verbose { + resultJSON, _ := json.MarshalIndent(result, "", " ") + if result.Status == StatusFailed { + fmt.Println("=== Task Result (Error) ===") + fmt.Println(string(resultJSON)) + fmt.Println("===========================") + } else { + fmt.Println("=== Task Result ===") + fmt.Println(string(resultJSON)) + fmt.Println("===================") + } + } + + log.Infof("Task %s completed with status: %s", t.ID, result.Status) + return result +} + +// runAndParse executes the child and turns its outcome into a Result. +func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) Result { + if err := cmd.Run(); err != nil { + stderrOutput := stderr.String() + log.Errorf("Worker execution failed: %v", err) + if stderrOutput != "" { + log.Errorf("Worker stderr:\n%s", stderrOutput) + } + return Result{ + Status: StatusFailed, + Reason: fmt.Sprintf("worker execution failed: %v", err), + Logs: []string{stderrOutput}, + } + } + + var parsed stdioResult + if err := json.Unmarshal(stdout.Bytes(), &parsed); err != nil { + stdoutOutput := stdout.String() + log.Errorf("Failed to parse worker output as JSON: %v", err) + log.Errorf("Worker stdout:\n%s", stdoutOutput) + return Result{ + Status: StatusFailed, + Reason: fmt.Sprintf("invalid worker stdout JSON: %v", err), + Logs: []string{stdoutOutput}, + } + } + + return normalizeStdioResult(parsed) +} + +// normalizeStdioResult constrains a stdio worker's status to the three values the stdio +// contract documents. Unlike JavaScript workers — which forward any status straight +// through — an unrecognised status here fails the task. +func normalizeStdioResult(parsed stdioResult) Result { + result := Result{ + Status: Status(parsed.Status), + Output: parsed.Output, + Logs: parsed.Logs, + Reason: parsed.Reason, + } + + switch result.Status { + case StatusCompleted, StatusFailed, StatusInProgress: + return result + } + + // Carried over verbatim from the pre-convergence implementation, including its + // quirk: Status is overwritten before the reason is formatted, so the message always + // reads "invalid status from worker: FAILED" rather than naming the offending value. + // Preserved here to keep the convergence a pure refactor; see issue #92 for the + // follow-up that fixes the message. + result.Status = StatusFailed + result.Reason = fmt.Sprintf("invalid status from worker: %s", result.Status) + return result +} diff --git a/internal/taskworker/stdio_test.go b/internal/taskworker/stdio_test.go new file mode 100644 index 0000000..3f09a83 --- /dev/null +++ b/internal/taskworker/stdio_test.go @@ -0,0 +1,279 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "context" + "encoding/json" + "os/exec" + "strings" + "sync" + "testing" + "time" +) + +func stdioTask() Task { + return Task{ + ID: "task-1", + WorkflowID: "wf-1", + Type: "greet", + Raw: json.RawMessage(`{"taskId":"task-1","taskType":"greet","inputData":{"name":"Miguel"}}`), + } +} + +// shWorker builds options that run a shell snippet as the worker program. +func shWorker(script string) StdioOptions { + return StdioOptions{Command: "sh", Args: []string{"-c", script}} +} + +func TestStdioHandlerCompletedResult(t *testing.T) { + h := NewStdioHandler(shWorker(`echo '{"status":"COMPLETED","output":{"message":"hi"},"logs":["did it"]}'`)) + + got := h.Handle(context.Background(), stdioTask()) + + if got.Status != StatusCompleted { + t.Errorf("Status = %q, want COMPLETED", got.Status) + } + if got.Output["message"] != "hi" { + t.Errorf("Output = %v, want message=hi", got.Output) + } + if len(got.Logs) != 1 || got.Logs[0] != "did it" { + t.Errorf("Logs = %v, want [did it]", got.Logs) + } +} + +func TestStdioHandlerReceivesFullTaskOnStdin(t *testing.T) { + // The worker echoes back what it read, so the test can assert the whole task — + // not just inputData — arrived on stdin. + h := NewStdioHandler(shWorker(`payload=$(cat); echo "{\"status\":\"COMPLETED\",\"output\":{\"echoed\":$payload}}"`)) + + got := h.Handle(context.Background(), stdioTask()) + + if got.Status != StatusCompleted { + t.Fatalf("Status = %q, want COMPLETED", got.Status) + } + echoed, ok := got.Output["echoed"].(map[string]interface{}) + if !ok { + t.Fatalf("Output[echoed] = %#v, want an object", got.Output["echoed"]) + } + for _, key := range []string{"taskId", "taskType", "inputData"} { + if _, present := echoed[key]; !present { + t.Errorf("stdin payload missing %q — the whole task must be written, not just inputData", key) + } + } +} + +func TestStdioHandlerExportsTaskMetadataToChild(t *testing.T) { + h := NewStdioHandler(shWorker( + `echo "{\"status\":\"COMPLETED\",\"output\":{\"type\":\"$TASK_TYPE\",\"id\":\"$TASK_ID\",\"wf\":\"$WORKFLOW_ID\",\"exec\":\"$EXECUTION_ID\"}}"`)) + + got := h.Handle(context.Background(), stdioTask()) + + want := map[string]string{"type": "greet", "id": "task-1", "wf": "wf-1", "exec": "wf-1"} + for k, v := range want { + if got.Output[k] != v { + t.Errorf("child env produced %s=%v, want %q", k, got.Output[k], v) + } + } +} + +func TestStdioHandlerExportsDomainAndInjectedEnv(t *testing.T) { + opts := shWorker(`echo "{\"status\":\"COMPLETED\",\"output\":{\"domain\":\"$POLL_DOMAIN\",\"injected\":\"$CONDUCTOR_AUTH_TOKEN\"}}"`) + opts.Domain = "prod" + opts.Env = []string{"CONDUCTOR_AUTH_TOKEN=tok-123"} + h := NewStdioHandler(opts) + + got := h.Handle(context.Background(), stdioTask()) + + if got.Output["domain"] != "prod" { + t.Errorf("POLL_DOMAIN = %v, want prod", got.Output["domain"]) + } + if got.Output["injected"] != "tok-123" { + t.Errorf("injected env did not reach the child: %v", got.Output["injected"]) + } +} + +func TestStdioHandlerNonZeroExitFails(t *testing.T) { + h := NewStdioHandler(shWorker(`echo "something broke" >&2; exit 3`)) + + got := h.Handle(context.Background(), stdioTask()) + + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if !strings.Contains(got.Reason, "worker execution failed") { + t.Errorf("Reason = %q, want it to mention worker execution failed", got.Reason) + } + if len(got.Logs) != 1 || !strings.Contains(got.Logs[0], "something broke") { + t.Errorf("Logs = %v, want stderr captured", got.Logs) + } +} + +func TestStdioHandlerMalformedJSONFails(t *testing.T) { + h := NewStdioHandler(shWorker(`echo 'this is not json'`)) + + got := h.Handle(context.Background(), stdioTask()) + + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if !strings.Contains(got.Reason, "invalid worker stdout JSON") { + t.Errorf("Reason = %q, want it to mention invalid worker stdout JSON", got.Reason) + } + if len(got.Logs) != 1 || !strings.Contains(got.Logs[0], "this is not json") { + t.Errorf("Logs = %v, want the raw stdout captured for debugging", got.Logs) + } +} + +func TestStdioHandlerExecTimeoutKillsChild(t *testing.T) { + opts := StdioOptions{Command: "sleep", Args: []string{"30"}} + opts.ExecTimeout = 100 * time.Millisecond + h := NewStdioHandler(opts) + + start := time.Now() + got := h.Handle(context.Background(), stdioTask()) + elapsed := time.Since(start) + + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if elapsed > 5*time.Second { + t.Errorf("took %v — the exec timeout did not kill the child", elapsed) + } +} + +func TestStdioHandlerCancelledContextStopsChild(t *testing.T) { + h := NewStdioHandler(StdioOptions{Command: "sleep", Args: []string{"30"}}) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(50 * time.Millisecond) + cancel() + }() + + start := time.Now() + got := h.Handle(ctx, stdioTask()) + + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("took %v — cancelling the context did not stop the child", elapsed) + } +} + +// TestStdioHandlerExecTimeoutDoesNotReapGrandchildren documents a limitation carried over +// from the pre-convergence implementation: the timeout signals only the direct child, and +// Run then waits for the captured stdout/stderr pipes to close. A worker that forks a +// long-running subprocess keeps those pipes open, so the effective timeout is the +// grandchild's lifetime, not ExecTimeout. +// +// Bounding a whole process tree needs process groups (setpgid plus kill on the negative +// pgid) and is platform-specific, so it is deliberately not part of the convergence work. +// The test is skipped by default because asserting it means waiting out the grandchild. +func TestStdioHandlerExecTimeoutDoesNotReapGrandchildren(t *testing.T) { + t.Skip("documents a known pre-existing limitation; unskip to observe it (takes ~3s)") + + // A compound script forks a real grandchild rather than exec-ing into it. + opts := shWorker(`sleep 3; echo '{"status":"COMPLETED"}'`) + opts.ExecTimeout = 100 * time.Millisecond + h := NewStdioHandler(opts) + + start := time.Now() + h.Handle(context.Background(), stdioTask()) + + if elapsed := time.Since(start); elapsed < time.Second { + t.Errorf("returned in %v — grandchildren are now reaped, so this limitation is fixed "+ + "and both this test and the comment on it should be removed", elapsed) + } +} + +// TestStdioHandlerUnknownStatusIsNormalized documents that stdio workers, unlike +// JavaScript workers, do not get to invent statuses. +func TestStdioHandlerUnknownStatusIsNormalized(t *testing.T) { + h := NewStdioHandler(shWorker(`echo '{"status":"BANANA"}'`)) + + got := h.Handle(context.Background(), stdioTask()) + + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if !strings.Contains(got.Reason, "invalid status from worker") { + t.Errorf("Reason = %q, want it to mention an invalid status", got.Reason) + } +} + +// TestNormalizeStdioResultPreservesStatuses covers the three statuses that pass through +// untouched, plus the pre-existing quirk in the rejection message. +func TestNormalizeStdioResultPreservesStatuses(t *testing.T) { + for _, status := range []Status{StatusCompleted, StatusFailed, StatusInProgress} { + t.Run(string(status), func(t *testing.T) { + got := normalizeStdioResult(stdioResult{Status: string(status), Reason: "as-is"}) + if got.Status != status { + t.Errorf("Status = %q, want %q", got.Status, status) + } + if got.Reason != "as-is" { + t.Errorf("Reason = %q, want it left alone", got.Reason) + } + }) + } + + t.Run("unknown status message quirk is preserved", func(t *testing.T) { + got := normalizeStdioResult(stdioResult{Status: "WEIRD"}) + // Faithful to the pre-convergence behaviour: the message names FAILED rather + // than the offending status, because Status is overwritten first. + if got.Reason != "invalid status from worker: FAILED" { + t.Errorf("Reason = %q, want the pre-existing wording preserved", got.Reason) + } + }) + + t.Run("empty status is rejected", func(t *testing.T) { + got := normalizeStdioResult(stdioResult{}) + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED for an empty status", got.Status) + } + }) +} + +// TestStdioHandlerConcurrentUse guards the Handler concurrency contract: one handler is +// shared across all goroutines of a batch poll. +func TestStdioHandlerConcurrentUse(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh not available") + } + h := NewStdioHandler(shWorker(`echo "{\"status\":\"COMPLETED\",\"output\":{\"id\":\"$TASK_ID\"}}"`)) + + const n = 8 + var wg sync.WaitGroup + results := make([]Result, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + task := stdioTask() + task.ID = string(rune('a' + i)) + results[i] = h.Handle(context.Background(), task) + }(i) + } + wg.Wait() + + for i, got := range results { + if got.Status != StatusCompleted { + t.Errorf("result %d status = %q, want COMPLETED", i, got.Status) + } + if want := string(rune('a' + i)); got.Output["id"] != want { + t.Errorf("result %d id = %v, want %q — concurrent Handle calls crossed state", i, got.Output["id"], want) + } + } +} diff --git a/internal/taskworker/taskworker.go b/internal/taskworker/taskworker.go new file mode 100644 index 0000000..6db39e5 --- /dev/null +++ b/internal/taskworker/taskworker.go @@ -0,0 +1,242 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +// Package taskworker is the single poll→execute→update loop shared by every worker +// flavour in the CLI: stdio workers, JavaScript (goja) workers, registry workers, and +// skill tool workers. It is layered — the loop and its two interfaces live here, the +// Conductor SDK is confined to the runner bridge, and each flavour's execution logic +// lives in a Handler. +// +// Before this package existed the loop was hand-copied four times in cmd/worker.go with +// no backoff, no context cancellation, and no tests. +package taskworker + +import ( + "context" + "encoding/json" + "fmt" + "time" + + log "github.com/sirupsen/logrus" +) + +// defaultPollBackoff is the idle wait between polls that return no task or an error. +// Runners also long-poll the server, so this is a hot-loop backstop rather than the +// primary pacing mechanism. +const defaultPollBackoff = 100 * time.Millisecond + +// Status is the task status reported back to Conductor. It is deliberately an open +// string type rather than a closed enum: JavaScript workers forward whatever status +// their script returns straight through, and FAILED_WITH_TERMINAL_ERROR is documented +// and in use. Handlers that need to constrain the set normalise it themselves. +type Status string + +const ( + StatusCompleted Status = "COMPLETED" + StatusFailed Status = "FAILED" + StatusInProgress Status = "IN_PROGRESS" + StatusFailedWithTerminalError Status = "FAILED_WITH_TERMINAL_ERROR" +) + +// Task is one polled task, decoupled from the SDK's model.Task. +type Task struct { + ID string + WorkflowID string + Type string + // WorkerID is the worker id carried by the polled task. JavaScript workers report + // this value back on the result rather than the configured --worker-id. + WorkerID string + // Raw is json.Marshal(model.Task) — the whole task, not the raw HTTP body and not + // just inputData. Stdio workers write it to the child's stdin and goja workers + // expose it as $.task, so both depend on it being the SDK struct's marshalling. + Raw json.RawMessage +} + +// InputData returns just the task's inputData, for handlers that want the input rather +// than the whole task. A task with no inputData yields "null", matching what the skill +// worker produced previously by marshalling a nil map. +func (t Task) InputData() (json.RawMessage, error) { + var envelope struct { + InputData json.RawMessage `json:"inputData"` + } + if err := json.Unmarshal(t.Raw, &envelope); err != nil { + return nil, err + } + if len(envelope.InputData) == 0 { + return json.RawMessage("null"), nil + } + return envelope.InputData, nil +} + +// Result is the outcome of executing one task. +// +// There is no error return alongside it: every outcome is a Result, so there is exactly +// one way to express failure. Handlers build their own failure Results because the +// flavours shape failures differently — JavaScript workers report the message under an +// "error" output key, stdio workers use ReasonForIncompletion plus logs — and those +// differences are observable by workflows. +type Result struct { + Status Status + Output map[string]interface{} + Logs []string + Reason string +} + +// Failure builds a Result for the common shape: FAILED with a reason and no output. +func Failure(reason string) Result { + return Result{Status: StatusFailed, Reason: reason} +} + +// Handler executes one task. +// +// A Handler MUST be safe for concurrent use: one Handler is shared across all the +// goroutines of a batch poll. +type Handler interface { + Handle(ctx context.Context, t Task) Result +} + +// HandlerFunc adapts a function to Handler. +type HandlerFunc func(ctx context.Context, t Task) Result + +func (f HandlerFunc) Handle(ctx context.Context, t Task) Result { return f(ctx, t) } + +// PolledTask is one entry from a poll. Conversion happens per task so that a single +// malformed task fails on its own instead of discarding its whole batch. +type PolledTask struct { + Task Task + // Err is set when this task could not be converted from the SDK model. The loop + // fails such a task individually and continues with the rest of the batch. + Err error +} + +// Runner is the server boundary: polling for work and reporting results. The production +// implementation wraps the Conductor SDK; tests inject a fake. +type Runner interface { + Poll(ctx context.Context, taskType string) ([]PolledTask, error) + Update(ctx context.Context, t Task, r Result) error +} + +// Config tunes the loop. +type Config struct { + // PollBackoff is the wait after an empty or failed poll. Zero uses the default. + PollBackoff time.Duration +} + +// Worker runs the poll→execute→update loop for a single task type over a Runner. +type Worker struct { + runner Runner + cfg Config +} + +// NewWorker returns a Worker backed by the given Runner. +func NewWorker(runner Runner, cfg Config) *Worker { + if cfg.PollBackoff <= 0 { + cfg.PollBackoff = defaultPollBackoff + } + return &Worker{runner: runner, cfg: cfg} +} + +// Run polls taskType and dispatches each task to h until ctx is cancelled. +// +// Cancellation is not immediate: Run returns once the in-flight batch finishes. A +// handler that blocks — a goja script has no interrupt wired, for instance — delays +// shutdown for as long as it runs. +// +// Transient poll failures back off and retry rather than stop the loop, and a failing +// task affects only itself. +func (w *Worker) Run(ctx context.Context, taskType string, h Handler) { + for { + if ctx.Err() != nil { + return + } + + polled, err := w.runner.Poll(ctx, taskType) + if err != nil || len(polled) == 0 { + if !sleep(ctx, w.cfg.PollBackoff) { + return + } + continue + } + + w.runBatch(ctx, polled, h) + } +} + +// runBatch executes every task in a poll batch concurrently and waits for all of them. +// +// Waiting for the whole batch before polling again preserves the pre-existing --count +// semantics: the next poll is gated on the slowest task in the batch. Decoupling poll +// cadence from execution would be a behaviour change and is deliberately not done here. +func (w *Worker) runBatch(ctx context.Context, polled []PolledTask, h Handler) { + done := make(chan struct{}) + var pending int + + for _, p := range polled { + pending++ + go func(p PolledTask) { + defer func() { done <- struct{}{} }() + w.runOne(ctx, p, h) + }(p) + } + + for i := 0; i < pending; i++ { + <-done + } +} + +// runOne executes a single task and reports its result. A conversion error from the poll +// seam, or a panic in the handler, fails that task rather than the loop. +func (w *Worker) runOne(ctx context.Context, p PolledTask, h Handler) { + if p.Err != nil { + w.update(ctx, p.Task, Failure(p.Err.Error())) + return + } + + result := w.safeHandle(ctx, p.Task, h) + w.update(ctx, p.Task, result) +} + +// safeHandle runs the handler, converting a panic into a failed task so that one +// misbehaving handler cannot take the whole worker process down. +func (w *Worker) safeHandle(ctx context.Context, t Task, h Handler) (result Result) { + defer func() { + if r := recover(); r != nil { + result = Failure(fmt.Sprintf("worker panicked: %v", r)) + } + }() + return h.Handle(ctx, t) +} + +// update reports a result, deliberately detached from the loop's cancellation. +// +// A task that finished while the user was pressing Ctrl-C still has its result +// delivered; using the cancelled context would abandon completed work and leave the +// task in-flight until the server times it out. +func (w *Worker) update(ctx context.Context, t Task, r Result) { + if err := w.runner.Update(context.WithoutCancel(ctx), t, r); err != nil { + log.Errorf("Error updating task %s: %v", t.ID, err) + } +} + +// sleep waits d or until ctx is cancelled; it returns false if ctx was cancelled, which +// keeps the loop responsive to Ctrl-C during idle waits. +func sleep(ctx context.Context, d time.Duration) bool { + t := time.NewTimer(d) + defer t.Stop() + select { + case <-ctx.Done(): + return false + case <-t.C: + return true + } +} diff --git a/internal/taskworker/taskworker_test.go b/internal/taskworker/taskworker_test.go new file mode 100644 index 0000000..717a33d --- /dev/null +++ b/internal/taskworker/taskworker_test.go @@ -0,0 +1,338 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "context" + "encoding/json" + "errors" + "sync" + "sync/atomic" + "testing" + "time" +) + +// fakeRunner is a scripted Runner. Each Poll returns the next entry from batches, then +// reports empty polls forever. +type fakeRunner struct { + mu sync.Mutex + batches [][]PolledTask + errs []error + polls atomic.Int32 + updates []update +} + +type update struct { + task Task + result Result +} + +func (f *fakeRunner) Poll(ctx context.Context, taskType string) ([]PolledTask, error) { + n := int(f.polls.Add(1)) - 1 + + f.mu.Lock() + defer f.mu.Unlock() + if n < len(f.errs) && f.errs[n] != nil { + return nil, f.errs[n] + } + if n < len(f.batches) { + return f.batches[n], nil + } + return nil, nil +} + +func (f *fakeRunner) Update(ctx context.Context, t Task, r Result) error { + f.mu.Lock() + defer f.mu.Unlock() + f.updates = append(f.updates, update{task: t, result: r}) + return nil +} + +func (f *fakeRunner) recorded() []update { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]update, len(f.updates)) + copy(out, f.updates) + return out +} + +func task(id string) Task { + return Task{ID: id, WorkflowID: "wf-1", Type: "greet", Raw: json.RawMessage(`{"taskId":"` + id + `"}`)} +} + +func okHandler() Handler { + return HandlerFunc(func(ctx context.Context, t Task) Result { + return Result{Status: StatusCompleted, Output: map[string]interface{}{"id": t.ID}} + }) +} + +// runFor starts Run and cancels it once cancel() reports true, so tests never rely on +// wall-clock sleeps to decide the loop has done enough. +func runFor(t *testing.T, w *Worker, h Handler, until func() bool) { + t.Helper() + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + w.Run(ctx, "greet", h) + close(done) + }() + + deadline := time.After(2 * time.Second) + for !until() { + select { + case <-deadline: + cancel() + t.Fatal("condition not reached within 2s") + default: + time.Sleep(time.Millisecond) + } + } + cancel() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Run did not return after cancellation") + } +} + +func TestRunDispatchesPolledTasks(t *testing.T) { + r := &fakeRunner{batches: [][]PolledTask{{{Task: task("t1")}}}} + w := NewWorker(r, Config{PollBackoff: time.Millisecond}) + + runFor(t, w, okHandler(), func() bool { return len(r.recorded()) >= 1 }) + + got := r.recorded() + if got[0].task.ID != "t1" { + t.Errorf("task ID = %q, want t1", got[0].task.ID) + } + if got[0].result.Status != StatusCompleted { + t.Errorf("status = %q, want COMPLETED", got[0].result.Status) + } +} + +func TestRunBacksOffOnPollErrorWithoutSpinning(t *testing.T) { + r := &fakeRunner{errs: []error{errors.New("boom"), errors.New("boom"), errors.New("boom")}} + w := NewWorker(r, Config{PollBackoff: 50 * time.Millisecond}) + + ctx, cancel := context.WithTimeout(context.Background(), 220*time.Millisecond) + defer cancel() + w.Run(ctx, "greet", okHandler()) + + // With a 50ms backoff over ~220ms, a correct loop polls a handful of times. A loop + // without backoff would poll thousands of times. + if polls := r.polls.Load(); polls > 10 { + t.Errorf("polled %d times in 220ms with a 50ms backoff — loop is not backing off", polls) + } +} + +func TestRunBacksOffOnEmptyPollWithoutSpinning(t *testing.T) { + r := &fakeRunner{} // always empty + w := NewWorker(r, Config{PollBackoff: 50 * time.Millisecond}) + + ctx, cancel := context.WithTimeout(context.Background(), 220*time.Millisecond) + defer cancel() + w.Run(ctx, "greet", okHandler()) + + if polls := r.polls.Load(); polls > 10 { + t.Errorf("polled %d times in 220ms with a 50ms backoff — loop is not backing off", polls) + } +} + +func TestRunReturnsWhenContextCancelledMidBackoff(t *testing.T) { + r := &fakeRunner{} // always empty, so the loop sits in backoff + w := NewWorker(r, Config{PollBackoff: time.Hour}) + + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + w.Run(ctx, "greet", okHandler()) + close(done) + }() + + time.Sleep(20 * time.Millisecond) // let it reach the backoff sleep + cancel() + + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("Run did not return while waiting in backoff — cancellation is not interrupting the sleep") + } +} + +func TestRunFailedTaskDoesNotStopLoop(t *testing.T) { + r := &fakeRunner{batches: [][]PolledTask{ + {{Task: task("bad")}}, + {{Task: task("good")}}, + }} + w := NewWorker(r, Config{PollBackoff: time.Millisecond}) + + h := HandlerFunc(func(ctx context.Context, t Task) Result { + if t.ID == "bad" { + return Failure("handler said no") + } + return Result{Status: StatusCompleted} + }) + + runFor(t, w, h, func() bool { return len(r.recorded()) >= 2 }) + + got := r.recorded() + if got[0].result.Status != StatusFailed || got[0].result.Reason != "handler said no" { + t.Errorf("first update = %+v, want FAILED with reason", got[0].result) + } + if got[1].result.Status != StatusCompleted { + t.Errorf("second update = %+v, want COMPLETED — loop did not survive the failure", got[1].result) + } +} + +func TestRunHandlerPanicFailsOnlyThatTask(t *testing.T) { + r := &fakeRunner{batches: [][]PolledTask{ + {{Task: task("panics")}}, + {{Task: task("fine")}}, + }} + w := NewWorker(r, Config{PollBackoff: time.Millisecond}) + + h := HandlerFunc(func(ctx context.Context, t Task) Result { + if t.ID == "panics" { + panic("kaboom") + } + return Result{Status: StatusCompleted} + }) + + runFor(t, w, h, func() bool { return len(r.recorded()) >= 2 }) + + got := r.recorded() + if got[0].result.Status != StatusFailed { + t.Errorf("panicking task status = %q, want FAILED", got[0].result.Status) + } + if got[1].result.Status != StatusCompleted { + t.Errorf("loop did not survive a handler panic") + } +} + +func TestRunConversionErrorFailsOnlyThatTaskAndPeersStillRun(t *testing.T) { + r := &fakeRunner{batches: [][]PolledTask{{ + {Task: task("broken"), Err: errors.New("marshal task: nope")}, + {Task: task("peer")}, + }}} + w := NewWorker(r, Config{PollBackoff: time.Millisecond}) + + runFor(t, w, okHandler(), func() bool { return len(r.recorded()) >= 2 }) + + byID := map[string]Result{} + for _, u := range r.recorded() { + byID[u.task.ID] = u.result + } + + if got := byID["broken"]; got.Status != StatusFailed || got.Reason != "marshal task: nope" { + t.Errorf("broken task = %+v, want FAILED carrying the conversion error", got) + } + if got := byID["peer"]; got.Status != StatusCompleted { + t.Errorf("peer task = %+v, want COMPLETED — a bad task discarded its batch peers", got) + } +} + +func TestRunBatchExecutesConcurrently(t *testing.T) { + const n = 4 + batch := make([]PolledTask, 0, n) + for i := 0; i < n; i++ { + batch = append(batch, PolledTask{Task: task(string(rune('a' + i)))}) + } + r := &fakeRunner{batches: [][]PolledTask{batch}} + w := NewWorker(r, Config{PollBackoff: time.Millisecond}) + + // Every handler waits on the same barrier. If the batch ran serially the barrier + // would never be satisfied and the test would time out. + var wg sync.WaitGroup + wg.Add(n) + h := HandlerFunc(func(ctx context.Context, t Task) Result { + wg.Done() + wg.Wait() + return Result{Status: StatusCompleted} + }) + + barrierMet := make(chan struct{}) + go func() { wg.Wait(); close(barrierMet) }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go w.Run(ctx, "greet", h) + + select { + case <-barrierMet: + case <-time.After(2 * time.Second): + t.Fatal("batch tasks did not run concurrently — all 4 never ran at once") + } +} + +func TestUpdateStillDeliversAfterContextCancelled(t *testing.T) { + r := &fakeRunner{batches: [][]PolledTask{{{Task: task("t1")}}}} + w := NewWorker(r, Config{PollBackoff: time.Millisecond}) + + // The handler cancels the loop context before returning, mimicking Ctrl-C landing + // while a task is in flight. The result must still be reported. + ctx, cancel := context.WithCancel(context.Background()) + h := HandlerFunc(func(hctx context.Context, t Task) Result { + cancel() + return Result{Status: StatusCompleted} + }) + + done := make(chan struct{}) + go func() { w.Run(ctx, "greet", h); close(done) }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Run did not return") + } + + got := r.recorded() + if len(got) != 1 { + t.Fatalf("recorded %d updates, want 1 — a result finished during shutdown was dropped", len(got)) + } + if got[0].result.Status != StatusCompleted { + t.Errorf("status = %q, want COMPLETED", got[0].result.Status) + } +} + +func TestInputData(t *testing.T) { + tests := []struct { + name string + raw string + want string + }{ + {name: "present", raw: `{"taskId":"t","inputData":{"name":"Miguel"}}`, want: `{"name":"Miguel"}`}, + {name: "absent yields null", raw: `{"taskId":"t"}`, want: `null`}, + {name: "explicit null", raw: `{"inputData":null}`, want: `null`}, + {name: "empty object", raw: `{"inputData":{}}`, want: `{}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Task{Raw: json.RawMessage(tt.raw)}.InputData() + if err != nil { + t.Fatalf("InputData() error = %v", err) + } + if string(got) != tt.want { + t.Errorf("InputData() = %s, want %s", got, tt.want) + } + }) + } +} + +func TestInputDataMalformedRawErrors(t *testing.T) { + if _, err := (Task{Raw: json.RawMessage(`not json`)}).InputData(); err == nil { + t.Error("InputData() on malformed Raw returned nil error") + } +} From b879075b3452de615e0cbb0c4ba8936d3d5985f5 Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Wed, 29 Jul 2026 18:28:28 -0300 Subject: [PATCH 2/7] Move worker js, stdio and remote onto the shared loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deletes the four duplicated poll loops in favour of taskworker.Worker. Each flavour is now just a handler plus poll options: worker stdio → StdioHandler worker js → GojaHandler worker remote (PYTHON) → StdioHandler worker remote (NODEJS) → GojaHandler Behaviour preserved: the result shape of each flavour, --count batch semantics including the next poll waiting on the slowest task, the credential environment handed to child processes, stdout/stderr echoing, and --verbose banners. Deliberate changes: - the four loops gain backoff, so a poll error no longer spins the CPU - Ctrl-C and SIGTERM now shut a worker down and exit 0, where the loops were previously for{} bodies that only died when the process was killed - in-flight child processes are cancelled on shutdown, since the exec context now derives from the loop's worker js and worker remote gain --poll-timeout and --exec-timeout, matching worker stdio. --timeout stays as a hidden deprecated alias for --poll-timeout. This resolves #91: worker remote fed one --timeout value to both the poll (milliseconds) and the execution budget (seconds), so --timeout 100 meant 100ms and 100s at the same time. --exec-timeout defaults to 100s on remote to keep a hanging worker bounded as it was before. injectUtilities, httpRequest and the two result-shape tests move to internal/taskworker with the code they cover. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/worker.go | 715 ++++++++------------------------------------- cmd/worker_test.go | 289 ------------------ 2 files changed, 117 insertions(+), 887 deletions(-) diff --git a/cmd/worker.go b/cmd/worker.go index 84a4252..1d1d579 100644 --- a/cmd/worker.go +++ b/cmd/worker.go @@ -11,35 +11,25 @@ * specific language governing permissions and limitations under the License. */ - package cmd import ( - "bytes" - "context" - "crypto/md5" - "crypto/sha1" - "crypto/sha256" - "encoding/base64" - "encoding/hex" "encoding/json" "fmt" "io" "net/http" "os" "os/exec" + "os/signal" "path/filepath" "strings" - "sync" + "syscall" "time" - "github.com/antihax/optional" + "github.com/conductor-oss/conductor-cli/internal" + "github.com/conductor-oss/conductor-cli/internal/taskworker" "github.com/conductor-sdk/conductor-go/sdk/authentication" - "github.com/conductor-sdk/conductor-go/sdk/client" - "github.com/conductor-sdk/conductor-go/sdk/model" "github.com/conductor-sdk/conductor-go/sdk/settings" - "github.com/dop251/goja" - "github.com/conductor-oss/conductor-cli/internal" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -115,9 +105,9 @@ The worker runs in continuous mode, polling for tasks and executing them in para } workerListRemoteCmd = &cobra.Command{ - Use: "list-remote", - Short: "List available workers in the job-runner registry (EXPERIMENTAL, Orkes Conductor only)", - Long: `⚠️ EXPERIMENTAL FEATURE - List all available workers in the Orkes Conductor job-runner registry. + Use: "list-remote", + Short: "List available workers in the job-runner registry (EXPERIMENTAL, Orkes Conductor only)", + Long: `⚠️ EXPERIMENTAL FEATURE - List all available workers in the Orkes Conductor job-runner registry. ⚠️ Requires Orkes Conductor. Not available in OSS Conductor.`, RunE: listRemoteWorkers, SilenceUsage: true, @@ -167,298 +157,30 @@ func runJsWorker(cmd *cobra.Command, args []string) error { return fmt.Errorf("--type flag is required") } - count, _ := cmd.Flags().GetInt32("count") - workerId, _ := cmd.Flags().GetString("worker-id") - domain, _ := cmd.Flags().GetString("domain") - timeout, _ := cmd.Flags().GetInt32("timeout") - scriptContent, err := os.ReadFile(jsFile) if err != nil { return fmt.Errorf("error reading JavaScript file: %v", err) } + pollOpts, _ := workerPollFlags(cmd) + fmt.Printf("Starting worker for task type: %s\n", taskType) fmt.Printf("JavaScript file: %s\n", jsFile) - fmt.Printf("Worker ID: %s\n", workerId) - - for { - opts := &client.TaskResourceApiBatchPollOpts{} - if workerId != "" { - opts.Workerid = optional.NewString(workerId) - } - if domain != "" { - opts.Domain = optional.NewString(domain) - } - if count > 0 { - opts.Count = optional.NewInt32(count) - } - if timeout > 0 { - opts.Timeout = optional.NewInt32(timeout) - } - - taskClient := internal.GetTaskClient() - tasks, _, err := taskClient.BatchPoll(context.Background(), taskType, opts) - if err != nil { - log.Errorf("Error polling tasks: %v", err) - continue - } - - if len(tasks) == 0 { - log.Debug("No tasks available") - continue - } - - log.Infof("Polled %d task(s)", len(tasks)) - - var wg sync.WaitGroup - for _, task := range tasks { - wg.Add(1) - go func(t model.Task) { - defer wg.Done() - processTask(t, string(scriptContent), taskClient) - }(task) - } - - wg.Wait() - } -} - -func processTask(task model.Task, script string, taskClient *client.TaskResourceApiService) { - log.Infof("Processing task: %s (workflow: %s)", task.TaskId, task.WorkflowInstanceId) - - vm := goja.New() - - taskJSON, err := json.Marshal(task) - if err != nil { - log.Errorf("Error marshaling task: %v", err) - updateTaskFailed(taskClient, task, fmt.Sprintf("Error marshaling task: %v", err)) - return - } - - var taskObj interface{} - err = json.Unmarshal(taskJSON, &taskObj) - if err != nil { - log.Errorf("Error unmarshaling task: %v", err) - updateTaskFailed(taskClient, task, fmt.Sprintf("Error unmarshaling task: %v", err)) - return - } - - dollarObj := vm.NewObject() - err = dollarObj.Set("task", taskObj) - if err != nil { - log.Errorf("Error setting task in $: %v", err) - updateTaskFailed(taskClient, task, fmt.Sprintf("Error setting task: %v", err)) - return - } - err = vm.Set("$", dollarObj) - if err != nil { - log.Errorf("Error setting $ object: %v", err) - updateTaskFailed(taskClient, task, fmt.Sprintf("Error setting $ object: %v", err)) - return - } - - injectUtilities(vm) - - result, err := vm.RunString(script) - if err != nil { - log.Errorf("Error executing script for task %s: %v", task.TaskId, err) - updateTaskFailed(taskClient, task, fmt.Sprintf("Script execution error: %v", err)) - return - } - - if result != nil && !goja.IsUndefined(result) && !goja.IsNull(result) { - resultJSON := result.Export() - resultBytes, err := json.Marshal(resultJSON) - if err != nil { - log.Errorf("Error marshaling script result: %v", err) - updateTaskCompleted(taskClient, task, map[string]interface{}{}) - return - } - - var taskResult TaskResult - err = json.Unmarshal(resultBytes, &taskResult) - if err != nil { - log.Warnf("Script result not in expected format, treating as completed") - updateTaskCompleted(taskClient, task, map[string]interface{}{"result": resultJSON}) - return - } - - if taskResult.Body == nil { - taskResult.Body = make(map[string]interface{}) - } - updateTaskWithStatus(taskClient, task, taskResult.Status, taskResult.Body) - } else { - updateTaskCompleted(taskClient, task, map[string]interface{}{}) - } -} - -func updateTaskCompleted(taskClient *client.TaskResourceApiService, task model.Task, output map[string]interface{}) { - updateTaskWithStatus(taskClient, task, "COMPLETED", output) -} - -func updateTaskFailed(taskClient *client.TaskResourceApiService, task model.Task, reason string) { - output := map[string]interface{}{ - "error": reason, - } - updateTaskWithStatus(taskClient, task, "FAILED", output) -} - -func updateTaskWithStatus(taskClient *client.TaskResourceApiService, task model.Task, status string, output map[string]interface{}) { - log.Infof("Updating task %s with status: %s", task.TaskId, status) - - taskResult := &model.TaskResult{ - TaskId: task.TaskId, - WorkflowInstanceId: task.WorkflowInstanceId, - WorkerId: task.WorkerId, - Status: model.TaskResultStatus(status), - OutputData: output, - } - - _, _, err := taskClient.UpdateTask(context.Background(), taskResult) - if err != nil { - log.Errorf("Error updating task %s: %v", task.TaskId, err) - return - } - - log.Infof("Task %s updated successfully with status: %s", task.TaskId, status) -} - -// injectUtilities adds utility functions to the JavaScript VM -func injectUtilities(vm *goja.Runtime) { - // HTTP utilities - httpObj := vm.NewObject() - httpObj.Set("get", func(url string, headers map[string]interface{}) map[string]interface{} { - return httpRequest("GET", url, headers, "") - }) - httpObj.Set("post", func(url string, headers map[string]interface{}, body string) map[string]interface{} { - return httpRequest("POST", url, headers, body) - }) - httpObj.Set("put", func(url string, headers map[string]interface{}, body string) map[string]interface{} { - return httpRequest("PUT", url, headers, body) - }) - httpObj.Set("delete", func(url string, headers map[string]interface{}) map[string]interface{} { - return httpRequest("DELETE", url, headers, "") - }) - vm.Set("http", httpObj) - - // Crypto utilities - cryptoObj := vm.NewObject() - cryptoObj.Set("md5", func(text string) string { - hash := md5.Sum([]byte(text)) - return hex.EncodeToString(hash[:]) - }) - cryptoObj.Set("sha1", func(text string) string { - hash := sha1.Sum([]byte(text)) - return hex.EncodeToString(hash[:]) - }) - cryptoObj.Set("sha256", func(text string) string { - hash := sha256.Sum256([]byte(text)) - return hex.EncodeToString(hash[:]) - }) - cryptoObj.Set("base64Encode", func(text string) string { - return base64.StdEncoding.EncodeToString([]byte(text)) - }) - cryptoObj.Set("base64Decode", func(text string) string { - decoded, err := base64.StdEncoding.DecodeString(text) - if err != nil { - return "" - } - return string(decoded) - }) - vm.Set("crypto", cryptoObj) - - // Utility functions - utilObj := vm.NewObject() - utilObj.Set("sleep", func(ms int) { - time.Sleep(time.Duration(ms) * time.Millisecond) - }) - utilObj.Set("uuid", func() string { - return fmt.Sprintf("%d-%d", time.Now().UnixNano(), os.Getpid()) - }) - utilObj.Set("env", func(key string) string { - return os.Getenv(key) - }) - vm.Set("util", utilObj) - - // String utilities - stringObj := vm.NewObject() - stringObj.Set("toUpper", strings.ToUpper) - stringObj.Set("toLower", strings.ToLower) - stringObj.Set("trim", strings.TrimSpace) - stringObj.Set("split", func(s, sep string) []string { - return strings.Split(s, sep) - }) - stringObj.Set("join", func(arr []string, sep string) string { - return strings.Join(arr, sep) - }) - stringObj.Set("replace", func(s, old, new string) string { - return strings.ReplaceAll(s, old, new) - }) - stringObj.Set("contains", strings.Contains) - stringObj.Set("hasPrefix", strings.HasPrefix) - stringObj.Set("hasSuffix", strings.HasSuffix) - vm.Set("str", stringObj) -} - -func httpRequest(method, url string, headers map[string]interface{}, body string) map[string]interface{} { - var bodyReader io.Reader - if body != "" { - bodyReader = strings.NewReader(body) - } - - req, err := http.NewRequest(method, url, bodyReader) - if err != nil { - return map[string]interface{}{ - "error": err.Error(), - "status": 0, - } - } - - for key, value := range headers { - if strVal, ok := value.(string); ok { - req.Header.Set(key, strVal) - } - } - - client := &http.Client{Timeout: 30 * time.Second} - resp, err := client.Do(req) - if err != nil { - return map[string]interface{}{ - "error": err.Error(), - "status": 0, - } - } - defer resp.Body.Close() + fmt.Printf("Worker ID: %s\n", pollOpts.WorkerID) - respBody, err := io.ReadAll(resp.Body) + handler, err := taskworker.NewGojaHandler(string(scriptContent), jsFile) if err != nil { - return map[string]interface{}{ - "error": err.Error(), - "status": resp.StatusCode, - } - } - - var jsonBody interface{} - if err := json.Unmarshal(respBody, &jsonBody); err == nil { - return map[string]interface{}{ - "status": resp.StatusCode, - "body": jsonBody, - "text": string(respBody), - } + return err } - return map[string]interface{}{ - "status": resp.StatusCode, - "text": string(respBody), - } + return runWorkerLoop(cmd, taskType, handler, jsRunnerOptions(pollOpts)) } -// WorkerResult represents the expected output from a worker command -type WorkerResult struct { - Status string `json:"status"` // COMPLETED | FAILED | IN_PROGRESS - Output map[string]interface{} `json:"output,omitempty"` - Logs []string `json:"logs,omitempty"` - Reason string `json:"reason,omitempty"` +// jsRunnerOptions adjusts poll options for JavaScript workers, which report the polled +// task's own worker id on the result rather than the configured --worker-id. +func jsRunnerOptions(opts taskworker.RunnerOptions) taskworker.RunnerOptions { + opts.UseTaskWorkerID = true + return opts } func execWorker(cmd *cobra.Command, args []string) error { @@ -481,234 +203,67 @@ func execWorker(cmd *cobra.Command, args []string) error { count, _ := cmd.Flags().GetInt32("count") verbose, _ := cmd.Flags().GetBool("verbose") - taskClient := internal.GetTaskClient() - fmt.Printf("Starting worker for task type: %s\n", taskType) fmt.Printf("Command: %s %v\n", workerCmd, workerArgs) if workerId != "" { fmt.Printf("Worker ID: %s\n", workerId) } - for { - opts := &client.TaskResourceApiBatchPollOpts{} - if workerId != "" { - opts.Workerid = optional.NewString(workerId) - } - if domain != "" { - opts.Domain = optional.NewString(domain) - } - if count > 0 { - opts.Count = optional.NewInt32(count) - } - if pollTimeout > 0 { - opts.Timeout = optional.NewInt32(pollTimeout) - } - - tasks, _, err := taskClient.BatchPoll(context.Background(), taskType, opts) - if err != nil { - log.Errorf("Error polling tasks: %v", err) - continue - } - - if len(tasks) == 0 { - log.Debug("No tasks available") - continue - } - - log.Infof("Polled %d task(s)", len(tasks)) - - // Process tasks in parallel goroutines - var wg sync.WaitGroup - for _, task := range tasks { - wg.Add(1) - go func(t model.Task) { - defer wg.Done() - executeExternalWorker(t, workerCmd, workerArgs, workerId, domain, execTimeout, verbose, taskClient) - }(task) - } + handler := taskworker.NewStdioHandler(taskworker.StdioOptions{ + Command: workerCmd, + Args: workerArgs, + Env: workerChildEnv(), + Domain: domain, + ExecTimeout: time.Duration(execTimeout) * time.Second, + Verbose: verbose, + }) - wg.Wait() - } + return runWorkerLoop(cmd, taskType, handler, taskworker.RunnerOptions{ + WorkerID: workerId, + Domain: domain, + Count: count, + PollTimeoutMs: pollTimeout, + }) } -func executeExternalWorker(task model.Task, workerCmd string, workerArgs []string, workerId, domain string, execTimeout int32, verbose bool, taskClient *client.TaskResourceApiService) { - log.Infof("Processing task: %s (workflow: %s)", task.TaskId, task.WorkflowInstanceId) - - taskJSON, err := json.Marshal(task) - if err != nil { - log.Errorf("Error marshaling task: %v", err) - updateExecTaskFailed(taskClient, task, workerId, fmt.Sprintf("error marshaling task: %v", err)) - return - } - - if verbose { - fmt.Println("=== Task Input ===") - fmt.Println(string(taskJSON)) - fmt.Println("==================") - } +// runWorkerLoop drives a handler with the shared poll loop until the user interrupts it. +// Every worker flavour funnels through here, so the loop exists once rather than per +// flavour. +func runWorkerLoop(cmd *cobra.Command, taskType string, h taskworker.Handler, opts taskworker.RunnerOptions) error { + ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + defer stop() - ctx := context.Background() - if execTimeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, time.Duration(execTimeout)*time.Second) - defer cancel() - } + runner := taskworker.NewConductorRunner(internal.GetTaskClient(), opts) + taskworker.NewWorker(runner, taskworker.Config{}).Run(ctx, taskType, h) + return nil +} - execCmd := exec.CommandContext(ctx, workerCmd, workerArgs...) - execCmd.Env = append(execCmd.Environ(), - "TASK_TYPE="+task.TaskType, - "TASK_ID="+task.TaskId, - "WORKFLOW_ID="+task.WorkflowInstanceId, - "EXECUTION_ID="+task.WorkflowInstanceId, - ) - if domain != "" { - execCmd.Env = append(execCmd.Env, "POLL_DOMAIN="+domain) - } +// workerChildEnv builds the Conductor environment handed to worker subprocesses, so a +// worker can call back into Conductor with the same server and credentials as the CLI. +// +// Resolving viper here keeps process-global config in the cmd layer: internal/taskworker +// receives a plain []string. +func workerChildEnv() []string { + var env []string - serverUrl := viper.GetString("server") - if serverUrl != "" { + if serverUrl := viper.GetString("server"); serverUrl != "" { serverUrl = strings.TrimSuffix(serverUrl, "/") if !strings.HasSuffix(serverUrl, "/api") { serverUrl = serverUrl + "/api" } - execCmd.Env = append(execCmd.Env, "CONDUCTOR_SERVER_URL="+serverUrl) - } - - authKey := viper.GetString("auth-key") - authSecret := viper.GetString("auth-secret") - if authKey != "" { - execCmd.Env = append(execCmd.Env, "CONDUCTOR_ACCESS_KEY_ID="+authKey) - } - if authSecret != "" { - execCmd.Env = append(execCmd.Env, "CONDUCTOR_ACCESS_KEY_SECRET="+authSecret) - } - - authToken := viper.GetString("auth-token") - if authToken != "" { - execCmd.Env = append(execCmd.Env, "CONDUCTOR_AUTH_TOKEN="+authToken) - } - - execCmd.Stdin = bytes.NewReader(taskJSON) - - var stdout, stderr bytes.Buffer - execCmd.Stdout = io.MultiWriter(&stdout, os.Stdout) - execCmd.Stderr = io.MultiWriter(&stderr, os.Stderr) - - execErr := execCmd.Run() - - var result WorkerResult - if execErr != nil { - stderrOutput := stderr.String() - log.Errorf("Worker execution failed: %v", execErr) - if stderrOutput != "" { - log.Errorf("Worker stderr:\n%s", stderrOutput) - } - - result = WorkerResult{ - Status: "FAILED", - Reason: fmt.Sprintf("worker execution failed: %v", execErr), - Logs: []string{stderrOutput}, - } - } else { - if err := json.Unmarshal(stdout.Bytes(), &result); err != nil { - stdoutOutput := stdout.String() - log.Errorf("Failed to parse worker output as JSON: %v", err) - log.Errorf("Worker stdout:\n%s", stdoutOutput) - - result = WorkerResult{ - Status: "FAILED", - Reason: fmt.Sprintf("invalid worker stdout JSON: %v", err), - Logs: []string{stdoutOutput}, - } - } - } - - if verbose { - resultJSON, _ := json.MarshalIndent(result, "", " ") - if result.Status == "FAILED" { - fmt.Println("=== Task Result (Error) ===") - fmt.Println(string(resultJSON)) - fmt.Println("===========================") - } else { - fmt.Println("=== Task Result ===") - fmt.Println(string(resultJSON)) - fmt.Println("===================") - } - } - - switch result.Status { - case "COMPLETED", "FAILED", "IN_PROGRESS": - default: - result.Status = "FAILED" - result.Reason = fmt.Sprintf("invalid status from worker: %s", result.Status) - } - - var status model.TaskResultStatus - switch result.Status { - case "COMPLETED": - status = model.CompletedTask - case "FAILED": - status = model.FailedTask - case "IN_PROGRESS": - status = model.InProgressTask - default: - status = model.FailedTask + env = append(env, "CONDUCTOR_SERVER_URL="+serverUrl) } - - taskResult := model.TaskResult{ - TaskId: task.TaskId, - WorkflowInstanceId: task.WorkflowInstanceId, - Status: status, + if authKey := viper.GetString("auth-key"); authKey != "" { + env = append(env, "CONDUCTOR_ACCESS_KEY_ID="+authKey) } - - if result.Output != nil { - taskResult.OutputData = result.Output + if authSecret := viper.GetString("auth-secret"); authSecret != "" { + env = append(env, "CONDUCTOR_ACCESS_KEY_SECRET="+authSecret) } - - if len(result.Logs) > 0 { - logs := make([]model.TaskExecLog, len(result.Logs)) - for i, logLine := range result.Logs { - logs[i] = model.TaskExecLog{ - Log: logLine, - } - } - taskResult.Logs = logs - } - - if result.Reason != "" { - taskResult.ReasonForIncompletion = result.Reason + if authToken := viper.GetString("auth-token"); authToken != "" { + env = append(env, "CONDUCTOR_AUTH_TOKEN="+authToken) } - if workerId != "" { - taskResult.WorkerId = workerId - } - - _, _, err = taskClient.UpdateTask(context.Background(), &taskResult) - if err != nil { - log.Errorf("Error updating task %s: %v", task.TaskId, err) - return - } - - log.Infof("Task %s completed with status: %s", task.TaskId, result.Status) -} - -func updateExecTaskFailed(taskClient *client.TaskResourceApiService, task model.Task, workerId, reason string) { - taskResult := model.TaskResult{ - TaskId: task.TaskId, - WorkflowInstanceId: task.WorkflowInstanceId, - Status: model.FailedTask, - ReasonForIncompletion: reason, - OutputData: map[string]interface{}{"error": reason}, - } - - if workerId != "" { - taskResult.WorkerId = workerId - } - - _, _, err := taskClient.UpdateTask(context.Background(), &taskResult) - if err != nil { - log.Errorf("Error updating task %s as failed: %v", task.TaskId, err) - } + return env } func listRemoteWorkers(cmd *cobra.Command, args []string) error { @@ -996,61 +551,24 @@ func fileExists(path string) bool { } func executeJsWorkerFromFile(cmd *cobra.Command, workerFile, taskType string) error { - count, _ := cmd.Flags().GetInt32("count") - workerId, _ := cmd.Flags().GetString("worker-id") - domain, _ := cmd.Flags().GetString("domain") - timeout, _ := cmd.Flags().GetInt32("timeout") - scriptContent, err := os.ReadFile(workerFile) if err != nil { return fmt.Errorf("error reading worker file: %v", err) } + pollOpts, _ := workerPollFlags(cmd) + log.Infof("Starting JavaScript worker for task type: %s", taskType) - if workerId != "" { - log.Infof("Worker ID: %s", workerId) + if pollOpts.WorkerID != "" { + log.Infof("Worker ID: %s", pollOpts.WorkerID) } - for { - opts := &client.TaskResourceApiBatchPollOpts{} - if workerId != "" { - opts.Workerid = optional.NewString(workerId) - } - if domain != "" { - opts.Domain = optional.NewString(domain) - } - if count > 0 { - opts.Count = optional.NewInt32(count) - } - if timeout > 0 { - opts.Timeout = optional.NewInt32(timeout) - } - - taskClient := internal.GetTaskClient() - tasks, _, err := taskClient.BatchPoll(context.Background(), taskType, opts) - if err != nil { - log.Errorf("Error polling tasks: %v", err) - continue - } - - if len(tasks) == 0 { - log.Debug("No tasks available") - continue - } - - log.Infof("Polled %d task(s)", len(tasks)) - - var wg sync.WaitGroup - for _, task := range tasks { - wg.Add(1) - go func(t model.Task) { - defer wg.Done() - processTask(t, string(scriptContent), taskClient) - }(task) - } - - wg.Wait() + handler, err := taskworker.NewGojaHandler(string(scriptContent), workerFile) + if err != nil { + return err } + + return runWorkerLoop(cmd, taskType, handler, jsRunnerOptions(pollOpts)) } func setupPythonEnvironment(cacheDir string, dependencies []string) error { @@ -1130,10 +648,7 @@ func equalStringSlices(a, b []string) bool { } func executePythonWorkerFromFile(cmd *cobra.Command, workerFile, taskType string) error { - count, _ := cmd.Flags().GetInt32("count") - workerId, _ := cmd.Flags().GetString("worker-id") - domain, _ := cmd.Flags().GetString("domain") - execTimeout, _ := cmd.Flags().GetInt32("timeout") + pollOpts, execTimeout := workerPollFlags(cmd) pythonCmd := "python3" cacheDir := filepath.Dir(workerFile) @@ -1147,52 +662,54 @@ func executePythonWorkerFromFile(cmd *cobra.Command, workerFile, taskType string } log.Infof("Starting Python worker for task type: %s", taskType) - if workerId != "" { - log.Infof("Worker ID: %s", workerId) + if pollOpts.WorkerID != "" { + log.Infof("Worker ID: %s", pollOpts.WorkerID) } - taskClient := internal.GetTaskClient() - - for { - opts := &client.TaskResourceApiBatchPollOpts{} - if workerId != "" { - opts.Workerid = optional.NewString(workerId) - } - if domain != "" { - opts.Domain = optional.NewString(domain) - } - if count > 0 { - opts.Count = optional.NewInt32(count) - } - if execTimeout > 0 { - opts.Timeout = optional.NewInt32(execTimeout) - } - - tasks, _, err := taskClient.BatchPoll(context.Background(), taskType, opts) - if err != nil { - log.Errorf("Error polling tasks: %v", err) - continue - } - - if len(tasks) == 0 { - log.Debug("No tasks available") - continue - } + handler := taskworker.NewStdioHandler(taskworker.StdioOptions{ + Command: pythonCmd, + Args: []string{workerFile}, + Env: workerChildEnv(), + Domain: pollOpts.Domain, + ExecTimeout: execTimeout, + }) - log.Infof("Polled %d task(s)", len(tasks)) + return runWorkerLoop(cmd, taskType, handler, pollOpts) +} - // Process tasks in parallel goroutines - var wg sync.WaitGroup - for _, task := range tasks { - wg.Add(1) - go func(t model.Task) { - defer wg.Done() - executeExternalWorker(t, pythonCmd, []string{workerFile}, workerId, domain, execTimeout, false, taskClient) - }(task) - } +// workerPollFlags reads the poll and execution flags shared by the worker subcommands. +// +// --poll-timeout and --exec-timeout are the canonical names. --timeout is a deprecated +// alias for --poll-timeout, kept because `worker js` and `worker remote` shipped with it. +// Previously `worker remote` fed one --timeout value to both, so the same number meant +// milliseconds of poll wait and seconds of execution budget at once (issue #91). +func workerPollFlags(cmd *cobra.Command) (taskworker.RunnerOptions, time.Duration) { + opts := taskworker.RunnerOptions{} + opts.WorkerID, _ = cmd.Flags().GetString("worker-id") + opts.Domain, _ = cmd.Flags().GetString("domain") + opts.Count, _ = cmd.Flags().GetInt32("count") + + opts.PollTimeoutMs, _ = cmd.Flags().GetInt32("poll-timeout") + if cmd.Flags().Changed("timeout") && !cmd.Flags().Changed("poll-timeout") { + legacy, _ := cmd.Flags().GetInt32("timeout") + opts.PollTimeoutMs = legacy + } + + execSeconds, _ := cmd.Flags().GetInt32("exec-timeout") + return opts, time.Duration(execSeconds) * time.Second +} - wg.Wait() - } +// addPollTimeoutFlags registers the two timeout flags on a worker subcommand, plus the +// deprecated --timeout alias that `worker js` and `worker remote` shipped with. +// +// The alias is hidden rather than removed so existing invocations keep working; it maps +// to --poll-timeout only. See workerPollFlags and issue #91. +func addPollTimeoutFlags(cmd *cobra.Command, execTimeoutDefault int32) { + cmd.Flags().Int32("poll-timeout", 100, "Poll timeout in milliseconds") + cmd.Flags().Int32("exec-timeout", execTimeoutDefault, "Worker execution timeout in seconds (0 = no timeout)") + cmd.Flags().Int32("timeout", 100, "Deprecated: use --poll-timeout") + _ = cmd.Flags().MarkHidden("timeout") + _ = cmd.Flags().MarkDeprecated("timeout", "use --poll-timeout instead") } func init() { @@ -1201,24 +718,26 @@ func init() { workerJsCmd.Flags().Int32("count", 1, "Number of tasks to poll in each batch") workerJsCmd.Flags().String("worker-id", "", "Worker ID") workerJsCmd.Flags().String("domain", "", "Domain") - workerJsCmd.Flags().Int32("timeout", 100, "Timeout in milliseconds") + addPollTimeoutFlags(workerJsCmd, 0) workerStdioCmd.Flags().String("type", "", "Task type to poll for (required)") workerStdioCmd.MarkFlagRequired("type") workerStdioCmd.Flags().String("worker-id", "", "Worker ID") workerStdioCmd.Flags().String("domain", "", "Domain") - workerStdioCmd.Flags().Int32("poll-timeout", 100, "Poll timeout in milliseconds") - workerStdioCmd.Flags().Int32("exec-timeout", 0, "Execution timeout in seconds (0 = no timeout)") workerStdioCmd.Flags().Int32("count", 1, "Number of tasks to poll in each batch") workerStdioCmd.Flags().Bool("verbose", false, "Print task and result JSON to stdout") + addPollTimeoutFlags(workerStdioCmd, 0) workerRemoteCmd.Flags().String("type", "", "Task type to poll for (required)") workerRemoteCmd.MarkFlagRequired("type") workerRemoteCmd.Flags().Int32("count", 1, "Number of tasks to poll in each batch") workerRemoteCmd.Flags().String("worker-id", "", "Worker ID") workerRemoteCmd.Flags().String("domain", "", "Domain") - workerRemoteCmd.Flags().Int32("timeout", 100, "Timeout in milliseconds") workerRemoteCmd.Flags().Bool("refresh", false, "Force refresh worker from registry (ignore cache)") + // Remote workers previously derived their execution timeout from --timeout, whose + // default was 100. Defaulting --exec-timeout to 100s keeps a hanging remote worker + // bounded as it was before the two timeouts were separated. + addPollTimeoutFlags(workerRemoteCmd, 100) workerListRemoteCmd.Flags().String("namespace", "default", "Namespace to list workers from") diff --git a/cmd/worker_test.go b/cmd/worker_test.go index 1c30b2d..62fa50c 100644 --- a/cmd/worker_test.go +++ b/cmd/worker_test.go @@ -15,13 +15,9 @@ package cmd import ( "encoding/json" - "net/http" - "net/http/httptest" "os" "path/filepath" "testing" - - "github.com/dop251/goja" ) func TestGetWorkerFile(t *testing.T) { @@ -142,291 +138,6 @@ func TestLoadMetadata(t *testing.T) { }) } -func TestWorkerResultJSON(t *testing.T) { - tests := []struct { - name string - input string - status string - hasOut bool - }{ - { - name: "completed with output", - input: `{"status":"COMPLETED","output":{"key":"value"},"logs":["done"]}`, - status: "COMPLETED", - hasOut: true, - }, - { - name: "failed with reason", - input: `{"status":"FAILED","reason":"timeout"}`, - status: "FAILED", - hasOut: false, - }, - { - name: "in progress", - input: `{"status":"IN_PROGRESS"}`, - status: "IN_PROGRESS", - hasOut: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var result WorkerResult - err := json.Unmarshal([]byte(tt.input), &result) - if err != nil { - t.Fatalf("unmarshal failed: %v", err) - } - if result.Status != tt.status { - t.Errorf("Status: got %q, want %q", result.Status, tt.status) - } - if tt.hasOut && result.Output == nil { - t.Error("expected non-nil output") - } - }) - } -} - -func TestTaskResultJSON(t *testing.T) { - result := TaskResult{ - Status: "COMPLETED", - Body: map[string]interface{}{ - "message": "hello", - }, - } - - data, err := json.Marshal(result) - if err != nil { - t.Fatalf("marshal failed: %v", err) - } - - var decoded TaskResult - if err := json.Unmarshal(data, &decoded); err != nil { - t.Fatalf("unmarshal failed: %v", err) - } - - if decoded.Status != "COMPLETED" { - t.Errorf("Status: got %q, want %q", decoded.Status, "COMPLETED") - } - if decoded.Body["message"] != "hello" { - t.Errorf("Body.message: got %v, want %q", decoded.Body["message"], "hello") - } -} - -func TestHttpRequest(t *testing.T) { - t.Run("GET request", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != "GET" { - t.Errorf("expected GET, got %s", r.Method) - } - if r.Header.Get("X-Custom") != "test" { - t.Errorf("missing custom header") - } - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"result":"ok"}`)) - })) - defer server.Close() - - result := httpRequest("GET", server.URL, map[string]interface{}{"X-Custom": "test"}, "") - if result["status"] != http.StatusOK { - t.Errorf("status: got %v, want %d", result["status"], http.StatusOK) - } - body, ok := result["body"].(map[string]interface{}) - if !ok { - t.Fatal("expected body to be a map") - } - if body["result"] != "ok" { - t.Errorf("body.result: got %v, want %q", body["result"], "ok") - } - }) - - t.Run("POST request with body", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != "POST" { - t.Errorf("expected POST, got %s", r.Method) - } - w.Write([]byte(`{"created":true}`)) - })) - defer server.Close() - - result := httpRequest("POST", server.URL, nil, `{"name":"test"}`) - if result["status"] != http.StatusOK { - t.Errorf("status: got %v, want %d", result["status"], http.StatusOK) - } - }) - - t.Run("non-JSON response", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("plain text response")) - })) - defer server.Close() - - result := httpRequest("GET", server.URL, nil, "") - if result["text"] != "plain text response" { - t.Errorf("text: got %v, want %q", result["text"], "plain text response") - } - if result["body"] != nil { - t.Errorf("expected nil body for non-JSON, got %v", result["body"]) - } - }) - - t.Run("connection error", func(t *testing.T) { - result := httpRequest("GET", "http://localhost:1", nil, "") - if result["error"] == nil { - t.Error("expected error for connection failure") - } - if result["status"] != 0 { - t.Errorf("status: got %v, want 0", result["status"]) - } - }) -} - -func TestInjectUtilitiesCrypto(t *testing.T) { - vm := goja.New() - injectUtilities(vm) - - tests := []struct { - name string - script string - want string - }{ - {"md5", `crypto.md5("hello")`, "5d41402abc4b2a76b9719d911017c592"}, - {"sha1", `crypto.sha1("hello")`, "aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d"}, - {"sha256", `crypto.sha256("hello")`, "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"}, - {"base64Encode", `crypto.base64Encode("hello world")`, "aGVsbG8gd29ybGQ="}, - {"base64Decode", `crypto.base64Decode("aGVsbG8gd29ybGQ=")`, "hello world"}, - {"base64Decode invalid", `crypto.base64Decode("!!!invalid!!!")`, ""}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - val, err := vm.RunString(tt.script) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() != tt.want { - t.Errorf("got %q, want %q", val.String(), tt.want) - } - }) - } -} - -func TestInjectUtilitiesString(t *testing.T) { - vm := goja.New() - injectUtilities(vm) - - tests := []struct { - name string - script string - want string - }{ - {"toUpper", `str.toUpper("hello")`, "HELLO"}, - {"toLower", `str.toLower("WORLD")`, "world"}, - {"trim", `str.trim(" spaces ")`, "spaces"}, - {"contains true", `str.contains("hello world", "world")`, "true"}, - {"contains false", `str.contains("hello", "xyz")`, "false"}, - {"hasPrefix", `str.hasPrefix("hello", "hel")`, "true"}, - {"hasSuffix", `str.hasSuffix("hello", "llo")`, "true"}, - {"replace", `str.replace("foo bar foo", "foo", "baz")`, "baz bar baz"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - val, err := vm.RunString(tt.script) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() != tt.want { - t.Errorf("got %q, want %q", val.String(), tt.want) - } - }) - } -} - -func TestInjectUtilitiesSplit(t *testing.T) { - vm := goja.New() - injectUtilities(vm) - - val, err := vm.RunString(`JSON.stringify(str.split("a,b,c", ","))`) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() != `["a","b","c"]` { - t.Errorf("got %q, want %q", val.String(), `["a","b","c"]`) - } -} - -func TestInjectUtilitiesJoin(t *testing.T) { - vm := goja.New() - injectUtilities(vm) - - val, err := vm.RunString(`str.join(["a","b","c"], "-")`) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() != "a-b-c" { - t.Errorf("got %q, want %q", val.String(), "a-b-c") - } -} - -func TestInjectUtilitiesEnv(t *testing.T) { - vm := goja.New() - injectUtilities(vm) - - os.Setenv("TEST_CONDUCTOR_VAR", "test_value") - defer os.Unsetenv("TEST_CONDUCTOR_VAR") - - val, err := vm.RunString(`util.env("TEST_CONDUCTOR_VAR")`) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() != "test_value" { - t.Errorf("got %q, want %q", val.String(), "test_value") - } - - // Non-existent env var - val, err = vm.RunString(`util.env("NONEXISTENT_VAR_12345")`) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() != "" { - t.Errorf("got %q, want empty string", val.String()) - } -} - -func TestInjectUtilitiesUUID(t *testing.T) { - vm := goja.New() - injectUtilities(vm) - - val, err := vm.RunString(`util.uuid()`) - if err != nil { - t.Fatalf("script error: %v", err) - } - if val.String() == "" { - t.Error("expected non-empty UUID") - } -} - -func TestInjectUtilitiesHTTP(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"method":"` + r.Method + `"}`)) - })) - defer server.Close() - - vm := goja.New() - injectUtilities(vm) - - // Test http.get - val, err := vm.RunString(`JSON.stringify(http.get("` + server.URL + `", {}))`) - if err != nil { - t.Fatalf("script error: %v", err) - } - result := val.String() - if result == "" { - t.Error("expected non-empty result") - } -} - func TestWorkerCodeResponseJSON(t *testing.T) { input := `{ "id": "wc-123", From 40410358c1d31cf36ba676cc529d70a2a6fe2dad Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Wed, 29 Jul 2026 18:28:28 -0300 Subject: [PATCH 3/7] Move skill tool workers onto the shared loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces skillworker's own loop, TaskRunner and conductorRunner with an adapter onto taskworker, leaving this package owning just the tool logic and the {"result": …} envelope. ToolHandler is kept rather than collapsed into taskworker.Handler. Seven constructors return it, three functions pass it as map[string]ToolHandler, and ten tests assert against it; adapting instead is one line at startSkillWorkers and avoids duplicating the result wrap across every tool. It is a cohesive local shape — tool logic in, raw JSON out — and stays this package's vocabulary. Preserved: the {skillName}__{tool} task type, the conductor-cli worker id, one task per poll, and wrapResult's fallbacks — non-JSON output carried through as a string, and nil output yielding {"result": null}. The loop and runner tests are deleted rather than kept: they covered the code being removed, and taskworker's tests cover its replacement. handlers_test.go is untouched and remains the regression guard for the tool logic. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/skill_run.go | 6 +- internal/skillworker/adapter.go | 76 ++++++++++++++ internal/skillworker/runner.go | 105 ------------------- internal/skillworker/runner_test.go | 69 ------------- internal/skillworker/worker.go | 95 ++--------------- internal/skillworker/worker_test.go | 154 ---------------------------- 6 files changed, 88 insertions(+), 417 deletions(-) create mode 100644 internal/skillworker/adapter.go delete mode 100644 internal/skillworker/runner.go delete mode 100644 internal/skillworker/runner_test.go delete mode 100644 internal/skillworker/worker_test.go diff --git a/cmd/skill_run.go b/cmd/skill_run.go index 9c457d9..9ee810d 100644 --- a/cmd/skill_run.go +++ b/cmd/skill_run.go @@ -29,6 +29,7 @@ import ( "github.com/conductor-oss/conductor-cli/internal" "github.com/conductor-oss/conductor-cli/internal/agent" "github.com/conductor-oss/conductor-cli/internal/skillworker" + "github.com/conductor-oss/conductor-cli/internal/taskworker" ) // Skill run/serve flag defaults. @@ -173,9 +174,10 @@ func scriptOptions() skillworker.ScriptOptions { // They run until ctx is cancelled. func startSkillWorkers(ctx context.Context, registry map[string]skillworker.ToolHandler) { taskClient := internal.GetTaskClient() + opts := skillworker.RunnerOptions() for taskType, handler := range registry { - w := skillworker.NewWorker(skillworker.NewConductorRunner(taskClient)) - go w.Run(ctx, taskType, handler) + w := taskworker.NewWorker(taskworker.NewConductorRunner(taskClient, opts), taskworker.Config{}) + go w.Run(ctx, taskType, skillworker.AsTaskHandler(handler)) } } diff --git a/internal/skillworker/adapter.go b/internal/skillworker/adapter.go new file mode 100644 index 0000000..8cdce27 --- /dev/null +++ b/internal/skillworker/adapter.go @@ -0,0 +1,76 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package skillworker + +import ( + "context" + "encoding/json" + + "github.com/conductor-oss/conductor-cli/internal/taskworker" +) + +// WorkerID identifies the CLI in the task results it reports for skill tools. +const WorkerID = "conductor-cli" + +// PollTimeoutMs is the server-side long-poll wait used for skill tool tasks. +const PollTimeoutMs = 100 + +// RunnerOptions returns the poll settings for skill tool workers. Skill tools take one +// task at a time and always identify as the CLI, unlike the worker commands where the +// user chooses a worker id. +func RunnerOptions() taskworker.RunnerOptions { + return taskworker.RunnerOptions{ + WorkerID: WorkerID, + Count: 1, + PollTimeoutMs: PollTimeoutMs, + } +} + +// AsTaskHandler adapts a ToolHandler onto the shared worker loop. +// +// ToolHandler stays the vocabulary of this package — tool logic in, raw JSON out — and +// this adapter supplies what the loop needs around it: the task's inputData rather than +// the whole task, the {"result": …} envelope the skill agent expects, and the mapping +// from a handler error to a failed task. +func AsTaskHandler(h ToolHandler) taskworker.Handler { + return taskworker.HandlerFunc(func(ctx context.Context, t taskworker.Task) taskworker.Result { + input, err := t.InputData() + if err != nil { + return taskworker.Failure(err.Error()) + } + + output, err := h.Handle(ctx, input) + if err != nil { + return taskworker.Failure(err.Error()) + } + + return taskworker.Result{ + Status: taskworker.StatusCompleted, + Output: wrapResult(output), + } + }) +} + +// wrapResult wraps a handler's raw output under the "result" key the skill agent expects. +// Decoding to a generic value happens only here, at the seam. Output that is not valid +// JSON is carried through as a string rather than failing the task. +func wrapResult(output json.RawMessage) map[string]interface{} { + var v interface{} + if len(output) > 0 { + if err := json.Unmarshal(output, &v); err != nil { + v = string(output) + } + } + return map[string]interface{}{outputKeyResult: v} +} diff --git a/internal/skillworker/runner.go b/internal/skillworker/runner.go deleted file mode 100644 index 6ae99b8..0000000 --- a/internal/skillworker/runner.go +++ /dev/null @@ -1,105 +0,0 @@ -/* - * Copyright 2026 Conductor Authors. - *

- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ - -package skillworker - -import ( - "context" - "encoding/json" - "fmt" - - "github.com/antihax/optional" - "github.com/conductor-sdk/conductor-go/sdk/client" - "github.com/conductor-sdk/conductor-go/sdk/model" -) - -// Poll tuning: one task per poll, with a short server-side long-poll wait. The -// worker loop adds its own backoff on top (see pollBackoff). -const ( - pollBatchSize = 1 // tasks requested per poll - pollTimeoutMs = 100 // server-side long-poll wait, milliseconds -) - -// conductorRunner adapts Conductor's TaskResourceApiService to TaskRunner. It is the -// ONLY place model.* and *client.TaskResourceApiService appear in this package — -// the worker loop and the handlers see only skillworker.Task and json.RawMessage. -type conductorRunner struct { - client *client.TaskResourceApiService -} - -// NewConductorRunner returns a TaskRunner backed by the Conductor task client -// (supplied by the cmd layer via internal.GetTaskClient()). -func NewConductorRunner(taskClient *client.TaskResourceApiService) TaskRunner { - return &conductorRunner{client: taskClient} -} - -func (r *conductorRunner) Poll(ctx context.Context, taskType string) (Task, bool, error) { - opts := &client.TaskResourceApiBatchPollOpts{ - Workerid: optional.NewString(workerID), - Count: optional.NewInt32(pollBatchSize), - Timeout: optional.NewInt32(pollTimeoutMs), - } - tasks, _, err := r.client.BatchPoll(ctx, taskType, opts) - if err != nil { - return Task{}, false, err - } - if len(tasks) == 0 { - return Task{}, false, nil - } - return taskFromModel(tasks[0]) -} - -func (r *conductorRunner) Complete(ctx context.Context, t Task, output json.RawMessage) error { - return r.update(ctx, t, model.CompletedTask, wrapResult(output), "") -} - -func (r *conductorRunner) Fail(ctx context.Context, t Task, reason string) error { - return r.update(ctx, t, model.FailedTask, nil, reason) -} - -func (r *conductorRunner) update(ctx context.Context, t Task, status model.TaskResultStatus, output map[string]interface{}, reason string) error { - result := &model.TaskResult{ - TaskId: t.ID, - WorkflowInstanceId: t.WorkflowID, - WorkerId: workerID, - Status: status, - OutputData: output, - } - if reason != "" { - result.ReasonForIncompletion = reason - } - _, _, err := r.client.UpdateTask(ctx, result) - return err -} - -// taskFromModel marshals the SDK task's input map to bytes at the seam, so the -// map[string]interface{} never crosses into the worker loop or the handlers. -func taskFromModel(t model.Task) (Task, bool, error) { - input, err := json.Marshal(t.InputData) - if err != nil { - return Task{}, false, fmt.Errorf("marshal task input: %w", err) - } - return Task{ID: t.TaskId, WorkflowID: t.WorkflowInstanceId, Input: input}, true, nil -} - -// wrapResult wraps a handler's raw output under the "result" key the skill agent -// expects. Decoding to a generic value happens only here, at the seam. -func wrapResult(output json.RawMessage) map[string]interface{} { - var v interface{} - if len(output) > 0 { - if err := json.Unmarshal(output, &v); err != nil { - v = string(output) - } - } - return map[string]interface{}{outputKeyResult: v} -} diff --git a/internal/skillworker/runner_test.go b/internal/skillworker/runner_test.go deleted file mode 100644 index dfeecc8..0000000 --- a/internal/skillworker/runner_test.go +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Copyright 2026 Conductor Authors. - *

- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ - -package skillworker - -import ( - "encoding/json" - "testing" - - "github.com/conductor-sdk/conductor-go/sdk/model" -) - -// TestTaskFromModelMarshalsInput verifies the bridge maps the SDK task's InputData -// map to json.RawMessage at the seam (no map crosses into the loop/handlers). -func TestTaskFromModelMarshalsInput(t *testing.T) { - mt := model.Task{ - TaskId: "t1", - WorkflowInstanceId: "w1", - InputData: map[string]interface{}{"path": "notes.md"}, - } - task, ok, err := taskFromModel(mt) - if err != nil || !ok { - t.Fatalf("taskFromModel: ok=%v err=%v", ok, err) - } - if task.ID != "t1" || task.WorkflowID != "w1" { - t.Errorf("task ids = %+v", task) - } - var decoded map[string]string - if err := json.Unmarshal(task.Input, &decoded); err != nil { - t.Fatalf("input not valid JSON: %v", err) - } - if decoded["path"] != "notes.md" { - t.Errorf("input = %s", task.Input) - } -} - -// TestWrapResultWrapsUnderResultKey checks the output wrapping the skill agent -// expects, for both an object and a bare-string handler output. -func TestWrapResultWrapsUnderResultKey(t *testing.T) { - obj := wrapResult(json.RawMessage(`{"files":["a","b"]}`)) - inner, ok := obj[outputKeyResult].(map[string]interface{}) - if !ok { - t.Fatalf("result not an object: %#v", obj) - } - if _, ok := inner["files"]; !ok { - t.Errorf("wrapped object lost its fields: %#v", inner) - } - - str := wrapResult(json.RawMessage(`"hello"`)) - if str[outputKeyResult] != "hello" { - t.Errorf("string result = %#v", str[outputKeyResult]) - } - - // Empty output still produces the result key (nil value). - empty := wrapResult(nil) - if _, ok := empty[outputKeyResult]; !ok { - t.Errorf("empty output missing result key: %#v", empty) - } -} diff --git a/internal/skillworker/worker.go b/internal/skillworker/worker.go index f5da002..8b3136f 100644 --- a/internal/skillworker/worker.go +++ b/internal/skillworker/worker.go @@ -13,30 +13,26 @@ // Package skillworker is the local tool-worker runtime for skill run/serve. When a // skill agent runs on the server, it dispatches tool tasks (read_skill_file, each -// script, workspace tools) back to the CLI; this package polls for those tasks, -// runs them locally, and returns the result. It is layered: the poll→handle→update -// loop and its two interfaces live here, the Conductor SDK is confined to the -// runner bridge, and the concrete tool logic lives in the handlers (later stage). +// script, workspace tools) back to the CLI; the tools run locally and their results are +// returned. +// +// The poll→handle→update loop itself lives in internal/taskworker, shared with the +// worker commands. This package owns the tool logic (handlers.go) and the adapter that +// puts it on that loop (adapter.go). package skillworker import ( "context" "encoding/json" - "time" ) // Worker protocol constants — fixed by the skill agent/server contract, so they are // named constants, never inline literals. const ( - taskTypeSep = "__" // task type is "{skillName}__{tool}" - outputKeyResult = "result" // handler output is wrapped as {result: } - workerID = "conductor-cli" // identifies this worker in task results + taskTypeSep = "__" // task type is "{skillName}__{tool}" + outputKeyResult = "result" // handler output is wrapped as {result: } ) -// pollBackoff is the idle wait between polls that return no task or an error. The -// production runner also long-polls the server, so this is a hot-loop backstop. -const pollBackoff = 100 * time.Millisecond - // TaskType builds the "{skillName}__{tool}" task type dispatched for a skill tool. func TaskType(skillName, tool string) string { return skillName + taskTypeSep + tool @@ -47,78 +43,3 @@ func TaskType(skillName, tool string) string { type ToolHandler interface { Handle(ctx context.Context, input json.RawMessage) (json.RawMessage, error) } - -// Task is one polled tool task, decoupled from the SDK's model.Task. -type Task struct { - ID string - WorkflowID string - Input json.RawMessage -} - -// TaskRunner is the poll/complete/fail seam. The production impl (runner.go) wraps -// Conductor's TaskResourceApiService; tests inject a fake. It keeps model.Task and -// *client.TaskResourceApiService out of the worker loop and the handlers. -type TaskRunner interface { - // Poll returns the next task for taskType. ok=false means no task was available - // (poll again); a non-nil err is a real polling failure. - Poll(ctx context.Context, taskType string) (task Task, ok bool, err error) - Complete(ctx context.Context, t Task, output json.RawMessage) error - Fail(ctx context.Context, t Task, reason string) error -} - -// Worker runs the poll→handle→update loop for a single task type over a TaskRunner. -type Worker struct { - runner TaskRunner -} - -// NewWorker returns a Worker backed by the given TaskRunner. -func NewWorker(runner TaskRunner) *Worker { - return &Worker{runner: runner} -} - -// Run polls taskType and dispatches each task to h until ctx is cancelled. Transient -// poll failures back off and retry rather than stop the loop; a handler error fails -// only that task. Run returns when ctx is done. -func (w *Worker) Run(ctx context.Context, taskType string, h ToolHandler) { - for { - select { - case <-ctx.Done(): - return - default: - } - - task, ok, err := w.runner.Poll(ctx, taskType) - if err != nil { - if !sleep(ctx, pollBackoff) { - return - } - continue - } - if !ok { - if !sleep(ctx, pollBackoff) { - return - } - continue - } - - output, handleErr := h.Handle(ctx, task.Input) - if handleErr != nil { - _ = w.runner.Fail(ctx, task, handleErr.Error()) - continue - } - _ = w.runner.Complete(ctx, task, output) - } -} - -// sleep waits d or until ctx is cancelled; it returns false if ctx was cancelled, -// which keeps the poll loop responsive to Ctrl-C during idle waits. -func sleep(ctx context.Context, d time.Duration) bool { - t := time.NewTimer(d) - defer t.Stop() - select { - case <-ctx.Done(): - return false - case <-t.C: - return true - } -} diff --git a/internal/skillworker/worker_test.go b/internal/skillworker/worker_test.go deleted file mode 100644 index 6b52d8a..0000000 --- a/internal/skillworker/worker_test.go +++ /dev/null @@ -1,154 +0,0 @@ -/* - * Copyright 2026 Conductor Authors. - *

- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ - -package skillworker - -import ( - "context" - "encoding/json" - "errors" - "sync" - "testing" - "time" -) - -// handlerFunc adapts a function to ToolHandler. -type handlerFunc func(ctx context.Context, input json.RawMessage) (json.RawMessage, error) - -func (f handlerFunc) Handle(ctx context.Context, input json.RawMessage) (json.RawMessage, error) { - return f(ctx, input) -} - -// fakeRunner is a TaskRunner that returns queued tasks then reports empty polls. It -// records terminal updates and can cancel the loop once the queue is drained. -type fakeRunner struct { - mu sync.Mutex - queue []Task - pollCount int - completed []completedCall - failed []failedCall - stopAfterOne context.CancelFunc // cancel the loop after the first terminal update -} - -type completedCall struct { - task Task - output json.RawMessage -} - -type failedCall struct { - task Task - reason string -} - -func (r *fakeRunner) Poll(ctx context.Context, taskType string) (Task, bool, error) { - r.mu.Lock() - defer r.mu.Unlock() - r.pollCount++ - if len(r.queue) == 0 { - return Task{}, false, nil - } - t := r.queue[0] - r.queue = r.queue[1:] - return t, true, nil -} - -func (r *fakeRunner) Complete(ctx context.Context, t Task, output json.RawMessage) error { - r.mu.Lock() - r.completed = append(r.completed, completedCall{task: t, output: output}) - r.mu.Unlock() - if r.stopAfterOne != nil { - r.stopAfterOne() - } - return nil -} - -func (r *fakeRunner) Fail(ctx context.Context, t Task, reason string) error { - r.mu.Lock() - r.failed = append(r.failed, failedCall{task: t, reason: reason}) - r.mu.Unlock() - if r.stopAfterOne != nil { - r.stopAfterOne() - } - return nil -} - -func TestWorkerRunCompletesTask(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - fr := &fakeRunner{ - queue: []Task{{ID: "t1", WorkflowID: "w1", Input: json.RawMessage(`{"path":"a"}`)}}, - stopAfterOne: cancel, - } - var gotInput json.RawMessage - h := handlerFunc(func(_ context.Context, in json.RawMessage) (json.RawMessage, error) { - gotInput = in - return json.RawMessage(`"file body"`), nil - }) - - NewWorker(fr).Run(ctx, "demo__read_skill_file", h) - - if string(gotInput) != `{"path":"a"}` { - t.Errorf("handler input = %s", gotInput) - } - if len(fr.completed) != 1 || len(fr.failed) != 0 { - t.Fatalf("completed=%d failed=%d", len(fr.completed), len(fr.failed)) - } - got := fr.completed[0] - if got.task.ID != "t1" || string(got.output) != `"file body"` { - t.Errorf("completed call = %+v", got) - } -} - -func TestWorkerRunFailsTaskOnHandlerError(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - fr := &fakeRunner{ - queue: []Task{{ID: "t2", WorkflowID: "w2"}}, - stopAfterOne: cancel, - } - h := handlerFunc(func(context.Context, json.RawMessage) (json.RawMessage, error) { - return nil, errors.New("boom") - }) - - NewWorker(fr).Run(ctx, "demo__script", h) - - if len(fr.failed) != 1 || len(fr.completed) != 0 { - t.Fatalf("completed=%d failed=%d", len(fr.completed), len(fr.failed)) - } - if fr.failed[0].reason != "boom" || fr.failed[0].task.ID != "t2" { - t.Errorf("fail call = %+v", fr.failed[0]) - } -} - -func TestWorkerRunStopsOnContextCancel(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - fr := &fakeRunner{} // always reports empty polls - done := make(chan struct{}) - go func() { - NewWorker(fr).Run(ctx, "demo__x", handlerFunc(func(context.Context, json.RawMessage) (json.RawMessage, error) { - return nil, nil - })) - close(done) - }() - - cancel() - select { - case <-done: - case <-time.After(2 * time.Second): - t.Fatal("Run did not stop after context cancel") - } -} - -func TestTaskType(t *testing.T) { - if got := TaskType("demo", "read_skill_file"); got != "demo__read_skill_file" { - t.Errorf("TaskType = %q", got) - } -} From 3345ff589e79b14ce28f8faabf849c2dda48ecb0 Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Wed, 29 Jul 2026 18:30:27 -0300 Subject: [PATCH 4/7] Document the converged worker model, including skill workers Skill workers were undocumented: "skill" appeared zero times in README.md and CLAUDE.md, the agentspan docs have no skills section, and conductor-skills documents SDK-based workers instead. WORKER_SKILL.md covers the directory layout, the {skillName}__{tool} task types, the argv/stdout tool contract, and using a skill tool from a plain workflow with no agent involved. CLAUDE.md had no worker or skill commands at all, so an assistant working in this repo could not discover either. Both now have command tables, and the three result contracts are written down side by side. Flag corrections: worker js and worker remote now document --poll-timeout and --exec-timeout, with --timeout noted as deprecated. WORKER_STDIO.md's comparison table still called the command "Generic Workers (exec)", a name that predates the rename to stdio. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 59 +++++++++++++++++++ README.md | 17 +++++- WORKER_JS.md | 6 +- WORKER_SKILL.md | 148 ++++++++++++++++++++++++++++++++++++++++++++++++ WORKER_STDIO.md | 6 +- 5 files changed, 231 insertions(+), 5 deletions(-) create mode 100644 WORKER_SKILL.md diff --git a/CLAUDE.md b/CLAUDE.md index 3f29931..b1ec7fc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -187,6 +187,65 @@ Columns: NAME, VERSION, DESCRIPTION **Table Output (task list):** Columns: NAME, EXECUTABLE, DESCRIPTION, OWNER, TIMEOUT POLICY, TIMEOUT (s), RETRY COUNT, RESPONSE TIMEOUT (s) +### Worker Commands + +> **Note:** Workers are experimental. All flavours share one poll loop; they differ only in how user code is executed and in the result shape it returns. + +| Command | Description | Required Args | Optional Flags | Example | +|---------|-------------|---------------|----------------|---------| +| `worker stdio [args...]` | Run an external program per task; task JSON on stdin, result JSON on stdout | command | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--verbose` | `conductor worker stdio --type greet_task python3 worker.py` | +| `worker js ` | Run a JavaScript worker in the built-in interpreter | JS file | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout` | `conductor worker js --type greet_task worker.js` | +| `worker remote` | Run a worker downloaded from the Orkes job-runner registry (Orkes only) | None | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--refresh` | `conductor worker remote --type greet_task` | +| `worker list-remote` | List workers in the registry (Orkes only) | None | `--namespace` | `conductor worker list-remote` | + +**Flags:** +- `--type` - Task type to poll for (required for all worker commands) +- `--count` - Tasks polled per batch, executed in parallel (default 1). The next poll waits for the slowest task in the batch. +- `--poll-timeout` - Server-side long-poll wait in milliseconds (default 100) +- `--exec-timeout` - Per-task execution timeout in seconds (0 = none; default 100 for `remote`) +- `--timeout` - Deprecated alias for `--poll-timeout` +- `--verbose` - Print task and result JSON (`stdio` only) + +**Result contracts** differ per flavour: + +| Flavour | Worker returns | Failure carries | +|---------|----------------|-----------------| +| `stdio` | `{"status","output","logs","reason"}` on stdout | `reasonForIncompletion` + logs | +| `js` | `{status, body}` from the script; `$.task` holds the task | `output.error` | +| skill tools | bare stdout, wrapped as `{"result": ...}` | `reasonForIncompletion` | + +Workers exit cleanly on Ctrl-C/SIGTERM. Child processes receive `TASK_TYPE`, `TASK_ID`, `WORKFLOW_ID`, `EXECUTION_ID`, `POLL_DOMAIN`, and the CLI's own `CONDUCTOR_SERVER_URL` and credentials. + +### Skill Commands + +A skill is a directory with `SKILL.md` (frontmatter `name` required) plus `scripts/`. Each script is served as the Conductor task type `{skillName}__{tool}`, so a skill tool can be called by an agent **or** by a plain workflow task. + +| Command | Description | Required Args | Optional Flags | Example | +|---------|-------------|---------------|----------------|---------| +| `skill list` | List registered skills | None | `--all-versions`, `--json` | `conductor skill list` | +| `skill get [version]` | Get a registered skill | name | `--version` | `conductor skill get myskill` | +| `skill register ` | Package and register a local skill | path | | `conductor skill register ./myskill` | +| `skill load ` | Package a local skill and deploy it as an agent | path | | `conductor skill load ./myskill` | +| `skill pull [dest]` | Download and extract a skill package | name | `--version` | `conductor skill pull myskill` | +| `skill delete [version]` | Delete a registered skill version | name | `--version` | `conductor skill delete myskill` | +| `skill run ` | Start local tool workers, run the agent, stream output | path/name, prompt | `--model` (required), `--param`, `--version`, workspace flags | `conductor skill run ./myskill "say hi" --model gpt-4o` | +| `skill serve ` | Start local tool workers only, block until interrupted | path/name | `--version`, workspace flags | `conductor skill serve ./myskill` | + +**Workspace flags** (`run` and `serve`): `--workspace` (default `.`), `--no-workspace`, `--filesystem name=path` (repeatable), `--script-timeout` (default 300s), `--script-output-limit` (default 10 MiB). + +**Tool contract:** `inputParameters.command` becomes the script's argv; stdout becomes `{"result": ""}`; non-zero exit fails the task. Script language is chosen by extension (`.py .sh .js .mjs .ts .rb .go .bat .cmd`). + +Using a skill tool as a plain worker: + +```bash +conductor skill serve ./myskill & +# workflow task named "greetskill__greet" with inputParameters {"command": "Miguel"} +conductor workflow start --workflow skill_as_worker --input '{"name":"Miguel"}' --sync +# { "result": "Hello Miguel\n" } +``` + +See [WORKER_SKILL.md](./WORKER_SKILL.md), [WORKER_STDIO.md](./WORKER_STDIO.md), [WORKER_JS.md](./WORKER_JS.md). + ### Config Commands | Command | Description | Required Args | Optional Flags | Example | diff --git a/README.md b/README.md index 386cefd..8adef30 100644 --- a/README.md +++ b/README.md @@ -548,8 +548,8 @@ conductor worker [arguments] [flags] | Command | Description | |---------|-------------| | `stdio [args...]` | Run stdio worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--verbose`) | -| `js ` | Run JavaScript worker (`--type`, `--count`, `--worker-id`, `--domain`, `--timeout`) | -| `remote` | Run remote worker (`--type`, `--count`, `--worker-id`, `--domain`, `--refresh`) | +| `js ` | Run JavaScript worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`) | +| `remote` | Run remote worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--refresh`) | | `list-remote` | List remote workers (`--namespace`) | **Worker Options:** @@ -881,6 +881,19 @@ Execute tasks using **JavaScript** scripts with built-in utilities (HTTP, crypto 👉 **[Complete JavaScript Worker Documentation →](WORKER_JS.md)** +### Skill Workers + +A skill directory (`SKILL.md` plus `scripts/`) serves each of its scripts as a +Conductor task type via `conductor skill serve`. Scripts take their arguments from +`inputParameters.command` and return bare stdout, so there is no result envelope to +emit. Works with or without an agent. + +```bash +conductor skill serve ./myskill +``` + +👉 **[Complete Skill Worker Documentation →](WORKER_SKILL.md)** + **Quick example:** ```bash # Run a JavaScript worker diff --git a/WORKER_JS.md b/WORKER_JS.md index ec410fe..1bd6ea3 100644 --- a/WORKER_JS.md +++ b/WORKER_JS.md @@ -28,7 +28,9 @@ conductor worker js --type - `--count` - Number of tasks to poll in each batch (default: 1) - `--worker-id` - Worker ID for identification - `--domain` - Domain for task polling -- `--timeout` - Timeout in milliseconds (default: 100) +- `--poll-timeout` - Poll timeout in milliseconds (default: 100) +- `--exec-timeout` - Worker execution timeout in seconds (default: 0, no timeout) +- `--timeout` - Deprecated alias for `--poll-timeout` ### Example @@ -874,3 +876,5 @@ if (parsed.error) { | Custom Go functions | Modify `injectUtilities()` and rebuild | The JavaScript worker is designed for lightweight task processing with HTTP integration. For heavy processing or complex dependencies, consider calling external services that have full library support. + +See also [Stdio Workers](WORKER_STDIO.md) and [Skill Workers](WORKER_SKILL.md). diff --git a/WORKER_SKILL.md b/WORKER_SKILL.md new file mode 100644 index 0000000..a0c7d37 --- /dev/null +++ b/WORKER_SKILL.md @@ -0,0 +1,148 @@ +# Skill Workers + +A **skill** is a directory containing instructions and scripts. Registered with the server +it becomes an agent, but its tools still have to run on your machine — so the server +dispatches each tool call back to the CLI as a Conductor task, and the CLI runs it locally +and returns the result. + +That local half is a worker. It shares the poll loop with +[stdio workers](WORKER_STDIO.md) and [JavaScript workers](WORKER_JS.md), and it works +with or without an agent: because a skill tool is just a Conductor task type, an ordinary +workflow can call one directly. + +## Skill layout + +``` +myskill/ + SKILL.md # required; frontmatter must set `name` + scripts/ + greet.sh # each script becomes a tool +``` + +```markdown +--- +name: greetskill +description: Says hello +--- + +# Greet skill + +Instructions the agent reads. +``` + +```bash +#!/usr/bin/env bash +echo "Hello $1" +``` + +Script language is chosen by extension: `.py`, `.sh`, `.js`, `.mjs`, `.ts`, `.rb`, `.go`, +`.bat`, `.cmd`. Anything else is run with `bash`. + +## Serving the tools + +```bash +conductor skill serve ./myskill +# Serving workers for skill greetskill. Press Ctrl-C to stop. +``` + +`skill serve` starts one worker per tool and blocks. Use it when the skill is being run +somewhere else — from the UI, or by another process. `conductor skill run ` +starts the workers *and* runs the agent, stopping the workers when the run ends. + +## Task types + +Every tool is exposed as the task type: + +``` +{skillName}__{tool} +``` + +So `greetskill` with `scripts/greet.sh` serves `greetskill__greet`. Alongside the scripts, +these built-in tools are served too: + +| Tool | Task type | Purpose | +|---|---|---| +| `read_skill_file` | `{skill}__read_skill_file` | Read a file bundled with the skill | +| `list_workspace` | `{skill}__list_workspace` | List files in the workspace | +| `read_workspace_file` | `{skill}__read_workspace_file` | Read a workspace file | +| `search_workspace` | `{skill}__search_workspace` | Search the workspace | +| `git_status` | `{skill}__git_status` | Workspace git status | +| `git_diff` | `{skill}__git_diff` | Workspace git diff | + +The workspace tools are only served when a workspace is enabled; `--no-workspace` disables +them. + +## Tool contract + +Different from stdio workers, and simpler: + +**Input** — `inputParameters.command` is passed to the script as **arguments**, not on +stdin. Only that field reaches the script. + +**Output** — the script's **stdout** becomes the task output, wrapped as +`{"result": ""}`. There is no envelope to emit. + +**Failure** — a non-zero exit fails the task, with stderr in the failure reason. + +**Environment** — the skill root and the configured workspace roots are exported to the +script. + +## Using a skill tool from a plain workflow + +Nothing about this requires an agent. Point a `SIMPLE` task at the tool's task type: + +```json +{ + "name": "skill_as_worker", + "version": 1, + "tasks": [ + { + "name": "greetskill__greet", + "taskReferenceName": "g", + "type": "SIMPLE", + "inputParameters": { "command": "${workflow.input.name}" } + } + ] +} +``` + +```bash +conductor skill serve ./myskill & +conductor workflow start --workflow skill_as_worker --input '{"name":"Miguel"}' --sync +# { "result": "Hello Miguel\n" } +``` + +This makes a skill the lowest-ceremony way to run a script as a Conductor worker: no +result envelope, no SDK, and no protocol to implement. + +Two constraints to know before relying on it: + +- **The task type is fixed** as `{skill}__{tool}`, so an existing workflow cannot adopt a + skill tool without renaming its task. +- **Input is a single string.** Structured `inputData` does not reach the script; only + `command` does. Use a [stdio worker](WORKER_STDIO.md) when the task needs structured + input. + +## Flags + +| Flag | Applies to | Purpose | +|---|---|---| +| `--version` | run, serve | Skill version or checksum prefix | +| `--script-timeout` | run, serve | Per-script timeout in seconds (default 300) | +| `--script-output-limit` | run, serve | Max bytes captured from a script (default 10 MiB) | +| `--workspace` | run, serve | Workspace directory (default `.`) | +| `--no-workspace` | run, serve | Do not expose a workspace | +| `--filesystem name=path` | run, serve | Extra read-only root, repeatable | +| `--model` | run | Model for the agent (required for `run`) | +| `--param` | run | Skill parameter override, repeatable | + +## Comparison with other worker types + +| | Skill tools | [Stdio](WORKER_STDIO.md) | [JavaScript](WORKER_JS.md) | +|---|---|---|---| +| Task type | `{skill}__{tool}` | any | any | +| Input | `command` → argv | full task JSON on stdin | `$.task` | +| Output | bare stdout | `{status, output, logs, reason}` | `{status, body}` | +| Boilerplate | none | result envelope | result object | +| Structured input | no | yes | yes | +| Languages | by extension | any executable | JavaScript only | diff --git a/WORKER_STDIO.md b/WORKER_STDIO.md index c74cf12..316946a 100644 --- a/WORKER_STDIO.md +++ b/WORKER_STDIO.md @@ -260,9 +260,9 @@ func main() { 7. **Use environment variables**: Access `TASK_ID`, `WORKFLOW_ID` etc. when needed 8. **Exit with code 0**: Always exit with 0 and use status field for task outcome -## Comparison with JavaScript Workers +## Comparison with other worker types -| Feature | Generic Workers (exec) | JavaScript Workers (js) | +| Feature | Stdio Workers (stdio) | JavaScript Workers (js) | |---------|----------------------|------------------------| | Languages | Any (Python, Node, Go, etc.) | JavaScript only | | Dependencies | Full access to language ecosystem | Limited (Goja ES5.1+) | @@ -271,3 +271,5 @@ func main() { | HTTP Calls | Use language's HTTP library | Built-in `http` object | | File System | Full access | No access | | Best For | Complex logic, heavy dependencies | Lightweight tasks, quick scripts | + +See also [Skill Workers](WORKER_SKILL.md), which run a script with no result envelope at all. From 8e87ab8165fcb434451a3d03cb224d01b5cd7bc1 Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Thu, 30 Jul 2026 16:22:07 -0300 Subject: [PATCH 5/7] Address self-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three real regressions in the convergence, plus the gaps that let them through. Ctrl-C could be pressed once and then never again. signal.NotifyContext keeps its channel registered after firing, so the default disposition never returns and every later signal is swallowed. Combined with handlers that cannot be interrupted — a goja script has no vm.Interrupt wired, and a subprocess whose grandchild holds the captured pipes open blocks cmd.Run — a worker could become unkillable by anything short of SIGQUIT. Verified against a reproduction: NotifyContext survives two SIGTERMs, the replacement exits on the second. The first signal now asks the loop to drain and the second force-exits. Poll errors were silently swallowed. The four deleted loops each logged them; the shared loop discarded err, so a worker with a bad token printed its banner and then nothing, at any log level. The backoff had traded a noisy hot loop for a silent one. Restored, along with "Polled N task(s)" and the no-tasks debug line. The two places behaviour actually changed had no tests. GojaHandler and gojaResultToResult had none at all, and wrapResult lost the coverage it had when skillworker's runner test was deleted — the spec had named those as the guard for the undocumented skill contract. Added tests for the js result shapes (including an unknown status passing through and the output.error failure shape), concurrent Handle safety for both handlers, and the {"result": ...} envelope with its non-JSON and nil fallbacks. Smaller fixes: - worker js advertised --exec-timeout in three docs and discarded it; goja has no timeout mechanism, so the flag is gone rather than silently ignored - worker stdio accepted the hidden --timeout alias and ignored it, because execWorker read flags directly instead of going through workerPollFlags - --verbose printed Go field names and post-normalisation values, so a worker returning an unrecognised status could not see what it had actually sent - WORKER_SKILL.md named a task type that is never polled: the tool is list_workspace_files, not list_workspace - WORKER_SKILL.md implied stderr is separate; executeScript gives the script one buffer for both, so stderr lands inside {"result": ...} - README still claimed two worker types, and the new Skill Workers section had been inserted in front of the JavaScript quick example, orphaning it - dropped the now-unused TaskResult type from cmd/worker.go Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +- README.md | 17 +- WORKER_JS.md | 4 +- WORKER_SKILL.md | 8 +- cmd/worker.go | 80 +++++--- internal/skillworker/adapter_test.go | 166 ++++++++++++++++ internal/taskworker/goja_handler_test.go | 232 +++++++++++++++++++++++ internal/taskworker/stdio.go | 54 ++++-- internal/taskworker/taskworker.go | 25 ++- 9 files changed, 530 insertions(+), 60 deletions(-) create mode 100644 internal/skillworker/adapter_test.go create mode 100644 internal/taskworker/goja_handler_test.go diff --git a/CLAUDE.md b/CLAUDE.md index b1ec7fc..f20d49f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -194,7 +194,7 @@ Columns: NAME, EXECUTABLE, DESCRIPTION, OWNER, TIMEOUT POLICY, TIMEOUT (s), RETR | Command | Description | Required Args | Optional Flags | Example | |---------|-------------|---------------|----------------|---------| | `worker stdio [args...]` | Run an external program per task; task JSON on stdin, result JSON on stdout | command | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--verbose` | `conductor worker stdio --type greet_task python3 worker.py` | -| `worker js ` | Run a JavaScript worker in the built-in interpreter | JS file | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout` | `conductor worker js --type greet_task worker.js` | +| `worker js ` | Run a JavaScript worker in the built-in interpreter | JS file | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout` | `conductor worker js --type greet_task worker.js` | | `worker remote` | Run a worker downloaded from the Orkes job-runner registry (Orkes only) | None | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--refresh` | `conductor worker remote --type greet_task` | | `worker list-remote` | List workers in the registry (Orkes only) | None | `--namespace` | `conductor worker list-remote` | @@ -202,7 +202,7 @@ Columns: NAME, EXECUTABLE, DESCRIPTION, OWNER, TIMEOUT POLICY, TIMEOUT (s), RETR - `--type` - Task type to poll for (required for all worker commands) - `--count` - Tasks polled per batch, executed in parallel (default 1). The next poll waits for the slowest task in the batch. - `--poll-timeout` - Server-side long-poll wait in milliseconds (default 100) -- `--exec-timeout` - Per-task execution timeout in seconds (0 = none; default 100 for `remote`) +- `--exec-timeout` - Per-task execution timeout in seconds (`stdio` and `remote` only; 0 = none, default 100 for `remote`) - `--timeout` - Deprecated alias for `--poll-timeout` - `--verbose` - Print task and result JSON (`stdio` only) diff --git a/README.md b/README.md index 8adef30..de7606c 100644 --- a/README.md +++ b/README.md @@ -223,6 +223,7 @@ After installing, you'll get tab completion when typing `conductor `. * [Workers](#workers) * [Stdio Workers](#stdio-workers) * [JavaScript Workers (Built-in)](#javascript-workers--built-in-) + * [Skill Workers](#skill-workers) * [Remote Workers (Registry-based)](#remote-workers--registry-based-) * [Exit Codes](#exit-codes) * [Error Handling](#error-handling) @@ -548,7 +549,7 @@ conductor worker [arguments] [flags] | Command | Description | |---------|-------------| | `stdio [args...]` | Run stdio worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--verbose`) | -| `js ` | Run JavaScript worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`) | +| `js ` | Run JavaScript worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`) | | `remote` | Run remote worker (`--type`, `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--refresh`) | | `list-remote` | List remote workers (`--namespace`) | @@ -852,7 +853,7 @@ conductor --config /path/to/my-config.yaml workflow list ⚠️ **EXPERIMENTAL FEATURES** -The CLI supports two types of workers for processing Conductor tasks: +The CLI supports several types of workers for processing Conductor tasks: ### Stdio Workers @@ -879,6 +880,12 @@ Execute tasks using **JavaScript** scripts with built-in utilities (HTTP, crypto **Best for:** Prototyping, Lightweight tasks, quick scripts, HTTP integrations +**Quick example:** +```bash +# Run a JavaScript worker +conductor worker js --type greet_task worker.js +``` + 👉 **[Complete JavaScript Worker Documentation →](WORKER_JS.md)** ### Skill Workers @@ -894,12 +901,6 @@ conductor skill serve ./myskill 👉 **[Complete Skill Worker Documentation →](WORKER_SKILL.md)** -**Quick example:** -```bash -# Run a JavaScript worker -conductor worker js --type greet_task worker.js -``` - ### Remote Workers (Registry-based) ⚠️ **EXPERIMENTAL** - Download and execute workers directly from your Conductor Conductor instance without managing local files. diff --git a/WORKER_JS.md b/WORKER_JS.md index 1bd6ea3..a9ed854 100644 --- a/WORKER_JS.md +++ b/WORKER_JS.md @@ -29,9 +29,11 @@ conductor worker js --type - `--worker-id` - Worker ID for identification - `--domain` - Domain for task polling - `--poll-timeout` - Poll timeout in milliseconds (default: 100) -- `--exec-timeout` - Worker execution timeout in seconds (default: 0, no timeout) - `--timeout` - Deprecated alias for `--poll-timeout` +There is no execution timeout for JavaScript workers: scripts run in-process and the +interpreter has no interrupt wired, so a script that loops forever blocks its worker. + ### Example ```bash diff --git a/WORKER_SKILL.md b/WORKER_SKILL.md index a0c7d37..12f1ba2 100644 --- a/WORKER_SKILL.md +++ b/WORKER_SKILL.md @@ -63,7 +63,7 @@ these built-in tools are served too: | Tool | Task type | Purpose | |---|---|---| | `read_skill_file` | `{skill}__read_skill_file` | Read a file bundled with the skill | -| `list_workspace` | `{skill}__list_workspace` | List files in the workspace | +| `list_workspace_files` | `{skill}__list_workspace_files` | List files in the workspace | | `read_workspace_file` | `{skill}__read_workspace_file` | Read a workspace file | | `search_workspace` | `{skill}__search_workspace` | Search the workspace | | `git_status` | `{skill}__git_status` | Workspace git status | @@ -82,7 +82,11 @@ stdin. Only that field reaches the script. **Output** — the script's **stdout** becomes the task output, wrapped as `{"result": ""}`. There is no envelope to emit. -**Failure** — a non-zero exit fails the task, with stderr in the failure reason. +**Failure** — a non-zero exit fails the task, with the captured output in the failure reason. + +**stderr is merged into stdout.** `executeScript` gives the script a single buffer for both, +so anything a script logs to stderr ends up inside `{"result": …}` on success. Keep +diagnostics out of a script whose output you care about. **Environment** — the skill root and the configured workspace roots are exported to the script. diff --git a/cmd/worker.go b/cmd/worker.go index 1d1d579..537ffb8 100644 --- a/cmd/worker.go +++ b/cmd/worker.go @@ -14,6 +14,7 @@ package cmd import ( + "context" "encoding/json" "fmt" "io" @@ -115,11 +116,6 @@ The worker runs in continuous mode, polling for tasks and executing them in para } ) -type TaskResult struct { - Status string `json:"status"` - Body map[string]interface{} `json:"body"` -} - // WorkerCodeResponse represents the response from the job-runner worker-code API type WorkerCodeResponse struct { Id string `json:"id"` @@ -196,41 +192,34 @@ func execWorker(cmd *cobra.Command, args []string) error { workerCmd := args[0] workerArgs := args[1:] - workerId, _ := cmd.Flags().GetString("worker-id") - domain, _ := cmd.Flags().GetString("domain") - pollTimeout, _ := cmd.Flags().GetInt32("poll-timeout") - execTimeout, _ := cmd.Flags().GetInt32("exec-timeout") - count, _ := cmd.Flags().GetInt32("count") + pollOpts, execTimeout := workerPollFlags(cmd) verbose, _ := cmd.Flags().GetBool("verbose") fmt.Printf("Starting worker for task type: %s\n", taskType) fmt.Printf("Command: %s %v\n", workerCmd, workerArgs) - if workerId != "" { - fmt.Printf("Worker ID: %s\n", workerId) + if pollOpts.WorkerID != "" { + fmt.Printf("Worker ID: %s\n", pollOpts.WorkerID) } handler := taskworker.NewStdioHandler(taskworker.StdioOptions{ Command: workerCmd, Args: workerArgs, Env: workerChildEnv(), - Domain: domain, - ExecTimeout: time.Duration(execTimeout) * time.Second, + Domain: pollOpts.Domain, + ExecTimeout: execTimeout, Verbose: verbose, }) - return runWorkerLoop(cmd, taskType, handler, taskworker.RunnerOptions{ - WorkerID: workerId, - Domain: domain, - Count: count, - PollTimeoutMs: pollTimeout, - }) + return runWorkerLoop(cmd, taskType, handler, pollOpts) } // runWorkerLoop drives a handler with the shared poll loop until the user interrupts it. // Every worker flavour funnels through here, so the loop exists once rather than per // flavour. func runWorkerLoop(cmd *cobra.Command, taskType string, h taskworker.Handler, opts taskworker.RunnerOptions) error { - ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + ctx, cancel := context.WithCancel(cmd.Context()) + defer cancel() + stop := interruptWithEscalation(cancel) defer stop() runner := taskworker.NewConductorRunner(internal.GetTaskClient(), opts) @@ -238,6 +227,43 @@ func runWorkerLoop(cmd *cobra.Command, taskType string, h taskworker.Handler, op return nil } +// interruptWithEscalation cancels on the first interrupt and force-exits on the second. +// +// signal.NotifyContext alone is not enough here: it keeps the signal channel registered +// after firing once, so the default disposition never returns and further Ctrl-C presses +// are swallowed. A handler that cannot be interrupted — a goja script with no vm.Interrupt +// wired, or a subprocess whose grandchild is holding the captured pipes open — would then +// leave the worker unkillable by anything short of SIGQUIT. +// +// So the first signal asks the loop to drain, and a second one means the user is done +// waiting. +func interruptWithEscalation(cancel context.CancelFunc) (stop func()) { + signals := make(chan os.Signal, 2) + signal.Notify(signals, os.Interrupt, syscall.SIGTERM) + + done := make(chan struct{}) + go func() { + select { + case <-signals: + fmt.Fprintln(os.Stderr, "\nShutting down; press Ctrl-C again to exit immediately.") + cancel() + case <-done: + return + } + + select { + case <-signals: + os.Exit(130) + case <-done: + } + }() + + return func() { + signal.Stop(signals) + close(done) + } +} + // workerChildEnv builds the Conductor environment handed to worker subprocesses, so a // worker can call back into Conductor with the same server and credentials as the CLI. // @@ -704,9 +730,11 @@ func workerPollFlags(cmd *cobra.Command) (taskworker.RunnerOptions, time.Duratio // // The alias is hidden rather than removed so existing invocations keep working; it maps // to --poll-timeout only. See workerPollFlags and issue #91. -func addPollTimeoutFlags(cmd *cobra.Command, execTimeoutDefault int32) { +func addPollTimeoutFlags(cmd *cobra.Command, execTimeout bool, execTimeoutDefault int32) { cmd.Flags().Int32("poll-timeout", 100, "Poll timeout in milliseconds") - cmd.Flags().Int32("exec-timeout", execTimeoutDefault, "Worker execution timeout in seconds (0 = no timeout)") + if execTimeout { + cmd.Flags().Int32("exec-timeout", execTimeoutDefault, "Worker execution timeout in seconds (0 = no timeout)") + } cmd.Flags().Int32("timeout", 100, "Deprecated: use --poll-timeout") _ = cmd.Flags().MarkHidden("timeout") _ = cmd.Flags().MarkDeprecated("timeout", "use --poll-timeout instead") @@ -718,7 +746,7 @@ func init() { workerJsCmd.Flags().Int32("count", 1, "Number of tasks to poll in each batch") workerJsCmd.Flags().String("worker-id", "", "Worker ID") workerJsCmd.Flags().String("domain", "", "Domain") - addPollTimeoutFlags(workerJsCmd, 0) + addPollTimeoutFlags(workerJsCmd, false, 0) workerStdioCmd.Flags().String("type", "", "Task type to poll for (required)") workerStdioCmd.MarkFlagRequired("type") @@ -726,7 +754,7 @@ func init() { workerStdioCmd.Flags().String("domain", "", "Domain") workerStdioCmd.Flags().Int32("count", 1, "Number of tasks to poll in each batch") workerStdioCmd.Flags().Bool("verbose", false, "Print task and result JSON to stdout") - addPollTimeoutFlags(workerStdioCmd, 0) + addPollTimeoutFlags(workerStdioCmd, true, 0) workerRemoteCmd.Flags().String("type", "", "Task type to poll for (required)") workerRemoteCmd.MarkFlagRequired("type") @@ -737,7 +765,7 @@ func init() { // Remote workers previously derived their execution timeout from --timeout, whose // default was 100. Defaulting --exec-timeout to 100s keeps a hanging remote worker // bounded as it was before the two timeouts were separated. - addPollTimeoutFlags(workerRemoteCmd, 100) + addPollTimeoutFlags(workerRemoteCmd, true, 100) workerListRemoteCmd.Flags().String("namespace", "default", "Namespace to list workers from") diff --git a/internal/skillworker/adapter_test.go b/internal/skillworker/adapter_test.go new file mode 100644 index 0000000..ddac5b5 --- /dev/null +++ b/internal/skillworker/adapter_test.go @@ -0,0 +1,166 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package skillworker + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/conductor-oss/conductor-cli/internal/taskworker" +) + +// stubHandler returns a fixed output or error, standing in for a real tool. +type stubHandler struct { + output json.RawMessage + err error + gotIn json.RawMessage +} + +func (s *stubHandler) Handle(ctx context.Context, input json.RawMessage) (json.RawMessage, error) { + s.gotIn = input + return s.output, s.err +} + +func skillTask(raw string) taskworker.Task { + return taskworker.Task{ID: "t1", WorkflowID: "wf1", Raw: json.RawMessage(raw)} +} + +// TestAsTaskHandlerWrapsUnderResultKey pins the skill agent's output contract. It is the +// only contract of the three with no documentation outside the code, so this test is the +// specification. +func TestAsTaskHandlerWrapsUnderResultKey(t *testing.T) { + tests := []struct { + name string + output string + want interface{} + }{ + {name: "json string", output: `"Hello\n"`, want: "Hello\n"}, + {name: "json number", output: `42`, want: float64(42)}, + {name: "non-json carried through as a string", output: `not json at all`, want: "not json at all"}, + {name: "empty output becomes nil", output: ``, want: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := AsTaskHandler(&stubHandler{output: json.RawMessage(tt.output)}) + got := h.Handle(context.Background(), skillTask(`{"inputData":{}}`)) + + if got.Status != taskworker.StatusCompleted { + t.Errorf("Status = %q, want COMPLETED", got.Status) + } + if len(got.Output) != 1 { + t.Fatalf("Output = %v, want exactly one key", got.Output) + } + if got.Output["result"] != tt.want { + t.Errorf("Output[result] = %#v, want %#v", got.Output["result"], tt.want) + } + }) + } +} + +// TestAsTaskHandlerNilOutputYieldsNullResult pins that the key is always present, holding +// null, rather than the output map being empty. +func TestAsTaskHandlerNilOutputYieldsNullResult(t *testing.T) { + h := AsTaskHandler(&stubHandler{output: nil}) + got := h.Handle(context.Background(), skillTask(`{"inputData":{}}`)) + + value, present := got.Output["result"] + if !present { + t.Fatal(`Output has no "result" key; it must be present even when the output is empty`) + } + if value != nil { + t.Errorf("Output[result] = %#v, want nil", value) + } +} + +// TestAsTaskHandlerObjectOutputIsDecoded checks that structured output survives as +// structure rather than being stringified. +func TestAsTaskHandlerObjectOutputIsDecoded(t *testing.T) { + h := AsTaskHandler(&stubHandler{output: json.RawMessage(`{"files":["a.go"],"count":1}`)}) + got := h.Handle(context.Background(), skillTask(`{"inputData":{}}`)) + + inner, ok := got.Output["result"].(map[string]interface{}) + if !ok { + t.Fatalf("Output[result] = %#v, want a decoded object", got.Output["result"]) + } + if inner["count"] != float64(1) { + t.Errorf("result.count = %#v, want 1", inner["count"]) + } +} + +func TestAsTaskHandlerErrorFailsTheTask(t *testing.T) { + h := AsTaskHandler(&stubHandler{err: errors.New("tool exploded")}) + got := h.Handle(context.Background(), skillTask(`{"inputData":{}}`)) + + if got.Status != taskworker.StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if got.Reason != "tool exploded" { + t.Errorf("Reason = %q, want the handler error", got.Reason) + } + if got.Output != nil { + t.Errorf("Output = %v, want nil for a failure", got.Output) + } +} + +// TestAsTaskHandlerPassesOnlyInputData pins that a tool receives inputData rather than the +// whole task, which is how the tool handlers have always been written. +func TestAsTaskHandlerPassesOnlyInputData(t *testing.T) { + stub := &stubHandler{output: json.RawMessage(`"ok"`)} + h := AsTaskHandler(stub) + + h.Handle(context.Background(), skillTask(`{"taskId":"t1","inputData":{"command":"greet"}}`)) + + if string(stub.gotIn) != `{"command":"greet"}` { + t.Errorf("handler received %s, want only inputData", stub.gotIn) + } +} + +func TestAsTaskHandlerMalformedTaskFails(t *testing.T) { + h := AsTaskHandler(&stubHandler{output: json.RawMessage(`"ok"`)}) + got := h.Handle(context.Background(), skillTask(`not json`)) + + if got.Status != taskworker.StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } +} + +// TestRunnerOptionsMatchSkillContract pins the poll settings the skill agent expects: one +// task at a time, identifying as the CLI. +func TestRunnerOptionsMatchSkillContract(t *testing.T) { + opts := RunnerOptions() + + // Asserted against the literal too: the constant is the skill agent's wire contract, + // so renaming its value is a breaking change rather than a rename. + if opts.WorkerID != "conductor-cli" { + t.Errorf("WorkerID = %q, want conductor-cli", opts.WorkerID) + } + if opts.Count != 1 { + t.Errorf("Count = %d, want 1", opts.Count) + } + if opts.PollTimeoutMs != PollTimeoutMs { + t.Errorf("PollTimeoutMs = %d, want %d", opts.PollTimeoutMs, PollTimeoutMs) + } + if opts.UseTaskWorkerID { + t.Error("UseTaskWorkerID = true, want false — skill tools always report as conductor-cli") + } +} + +func TestTaskType(t *testing.T) { + if got := TaskType("greetskill", "greet"); got != "greetskill__greet" { + t.Errorf("TaskType() = %q, want greetskill__greet", got) + } +} diff --git a/internal/taskworker/goja_handler_test.go b/internal/taskworker/goja_handler_test.go new file mode 100644 index 0000000..778c66e --- /dev/null +++ b/internal/taskworker/goja_handler_test.go @@ -0,0 +1,232 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package taskworker + +import ( + "context" + "encoding/json" + "strings" + "sync" + "testing" +) + +func gojaTask() Task { + return Task{ + ID: "task-1", + WorkflowID: "wf-1", + Type: "greet", + WorkerID: "w-1", + Raw: json.RawMessage(`{"taskId":"task-1","taskType":"greet","inputData":{"name":"Miguel"},"pollCount":1}`), + } +} + +func handleScript(t *testing.T, script string) Result { + t.Helper() + h, err := NewGojaHandler(script, "test.js") + if err != nil { + t.Fatalf("NewGojaHandler() error = %v", err) + } + return h.Handle(context.Background(), gojaTask()) +} + +func TestGojaHandlerStatusAndBodyMapping(t *testing.T) { + got := handleScript(t, `(function () { + return { status: "COMPLETED", body: { message: "hi " + $.task.inputData.name } }; + })();`) + + if got.Status != StatusCompleted { + t.Errorf("Status = %q, want COMPLETED", got.Status) + } + if got.Output["message"] != "hi Miguel" { + t.Errorf("Output = %v, want message from $.task.inputData", got.Output) + } +} + +// TestGojaHandlerExposesWholeTask pins that $.task is the entire task, not just its +// input: WORKER_JS.md documents fields like pollCount and taskType on it. +func TestGojaHandlerExposesWholeTask(t *testing.T) { + got := handleScript(t, `(function () { + return { status: "COMPLETED", body: { type: $.task.taskType, polls: $.task.pollCount } }; + })();`) + + if got.Output["type"] != "greet" { + t.Errorf("$.task.taskType = %v, want greet", got.Output["type"]) + } + if got.Output["polls"] == nil { + t.Error("$.task.pollCount missing — $.task must be the whole task") + } +} + +// TestGojaHandlerPassesUnknownStatusThrough is the behaviour that forced Status to be an +// open string type: FAILED_WITH_TERMINAL_ERROR is documented, and the CLI must not +// second-guess statuses the server understands. +func TestGojaHandlerPassesUnknownStatusThrough(t *testing.T) { + for _, status := range []string{"FAILED_WITH_TERMINAL_ERROR", "IN_PROGRESS", "SOMETHING_NEW"} { + t.Run(status, func(t *testing.T) { + got := handleScript(t, `(function () { return { status: "`+status+`", body: {} }; })();`) + if string(got.Status) != status { + t.Errorf("Status = %q, want %q passed through unchanged", got.Status, status) + } + }) + } +} + +// TestGojaHandlerScriptErrorUsesErrorOutputKey pins the JavaScript failure shape. It +// differs from the stdio one, and a workflow reading ${task.output.error} depends on it. +func TestGojaHandlerScriptErrorUsesErrorOutputKey(t *testing.T) { + got := handleScript(t, `(function () { throw new Error("boom"); })();`) + + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + msg, ok := got.Output["error"].(string) + if !ok { + t.Fatalf(`Output["error"] = %#v, want a string — js failures report under "error"`, got.Output["error"]) + } + if !strings.Contains(msg, "boom") { + t.Errorf(`Output["error"] = %q, want it to contain the script error`, msg) + } + if got.Reason != "" { + t.Errorf("Reason = %q, want empty — js workers do not set ReasonForIncompletion", got.Reason) + } +} + +func TestGojaHandlerCompileErrorIsReportedAtConstruction(t *testing.T) { + if _, err := NewGojaHandler(`function ( {{{ bad syntax`, "bad.js"); err == nil { + t.Error("NewGojaHandler() on invalid JavaScript returned nil error") + } +} + +func TestGojaHandlerResultShapes(t *testing.T) { + tests := []struct { + name string + script string + wantStatus Status + check func(*testing.T, Result) + }{ + { + name: "no return value completes with empty output", + script: `(function () { var x = 1; })();`, + wantStatus: StatusCompleted, + check: func(t *testing.T, got Result) { + if len(got.Output) != 0 { + t.Errorf("Output = %v, want empty", got.Output) + } + }, + }, + { + name: "null completes with empty output", + script: `null;`, + wantStatus: StatusCompleted, + check: func(t *testing.T, got Result) { + if len(got.Output) != 0 { + t.Errorf("Output = %v, want empty", got.Output) + } + }, + }, + { + name: "object without a status completes and carries the value", + script: `({ greeting: "hello" });`, + wantStatus: StatusCompleted, + check: func(t *testing.T, got Result) { + if got.Output["result"] == nil { + t.Errorf(`Output = %v, want the value under "result"`, got.Output) + } + }, + }, + { + name: "bare string completes and carries the value", + script: `"just a string";`, + wantStatus: StatusCompleted, + check: func(t *testing.T, got Result) { + if got.Output["result"] != "just a string" { + t.Errorf(`Output["result"] = %v, want the string`, got.Output["result"]) + } + }, + }, + { + name: "status with no body yields an empty output map", + script: `({ status: "COMPLETED" });`, + wantStatus: StatusCompleted, + check: func(t *testing.T, got Result) { + if got.Output == nil { + t.Error("Output = nil, want an empty map") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := handleScript(t, tt.script) + if got.Status != tt.wantStatus { + t.Errorf("Status = %q, want %q", got.Status, tt.wantStatus) + } + tt.check(t, got) + }) + } +} + +// TestGojaHandlerConcurrentUse guards the Handler concurrency contract. The compiled +// program is shared, so each task must get its own Runtime — goja Runtimes are not safe +// for concurrent use, and a batch poll runs tasks in parallel. +func TestGojaHandlerConcurrentUse(t *testing.T) { + h, err := NewGojaHandler(`(function () { + return { status: "COMPLETED", body: { id: $.task.taskId } }; + })();`, "test.js") + if err != nil { + t.Fatalf("NewGojaHandler() error = %v", err) + } + + const n = 16 + var wg sync.WaitGroup + results := make([]Result, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + id := string(rune('a' + i)) + results[i] = h.Handle(context.Background(), Task{ + ID: id, + Raw: json.RawMessage(`{"taskId":"` + id + `"}`), + }) + }(i) + } + wg.Wait() + + for i, got := range results { + want := string(rune('a' + i)) + if got.Status != StatusCompleted { + t.Errorf("result %d status = %q, want COMPLETED", i, got.Status) + } + if got.Output["id"] != want { + t.Errorf("result %d id = %v, want %q — concurrent Handle calls shared a Runtime", i, got.Output["id"], want) + } + } +} + +func TestGojaHandlerMalformedRawFails(t *testing.T) { + h, err := NewGojaHandler(`({ status: "COMPLETED" });`, "test.js") + if err != nil { + t.Fatalf("NewGojaHandler() error = %v", err) + } + + got := h.Handle(context.Background(), Task{ID: "t", Raw: json.RawMessage(`not json`)}) + if got.Status != StatusFailed { + t.Errorf("Status = %q, want FAILED", got.Status) + } + if got.Output["error"] == nil { + t.Error(`Output["error"] missing for an unparseable task`) + } +} diff --git a/internal/taskworker/stdio.go b/internal/taskworker/stdio.go index 720688c..1e12ad3 100644 --- a/internal/taskworker/stdio.go +++ b/internal/taskworker/stdio.go @@ -100,23 +100,25 @@ func (h *StdioHandler) Handle(ctx context.Context, t Task) Result { result := h.runAndParse(cmd, &stdout, &stderr) - if h.opts.Verbose { - resultJSON, _ := json.MarshalIndent(result, "", " ") - if result.Status == StatusFailed { - fmt.Println("=== Task Result (Error) ===") - fmt.Println(string(resultJSON)) - fmt.Println("===========================") - } else { - fmt.Println("=== Task Result ===") - fmt.Println(string(resultJSON)) - fmt.Println("===================") - } - } - - log.Infof("Task %s completed with status: %s", t.ID, result.Status) + log.Infof("Task %s handled with status: %s", t.ID, result.Status) return result } +// printResultBanner reports a result under --verbose. The banner distinguishes failures +// so they stand out in a stream of task output. +func printResultBanner(status string, result Result) { + resultJSON, _ := json.MarshalIndent(result, "", " ") + if Status(status) == StatusFailed { + fmt.Println("=== Task Result (Error) ===") + fmt.Println(string(resultJSON)) + fmt.Println("===========================") + return + } + fmt.Println("=== Task Result ===") + fmt.Println(string(resultJSON)) + fmt.Println("===================") +} + // runAndParse executes the child and turns its outcome into a Result. func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) Result { if err := cmd.Run(); err != nil { @@ -125,11 +127,15 @@ func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) if stderrOutput != "" { log.Errorf("Worker stderr:\n%s", stderrOutput) } - return Result{ + failure := Result{ Status: StatusFailed, Reason: fmt.Sprintf("worker execution failed: %v", err), Logs: []string{stderrOutput}, } + if h.opts.Verbose { + printResultBanner(string(StatusFailed), failure) + } + return failure } var parsed stdioResult @@ -137,11 +143,27 @@ func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) stdoutOutput := stdout.String() log.Errorf("Failed to parse worker output as JSON: %v", err) log.Errorf("Worker stdout:\n%s", stdoutOutput) - return Result{ + failure := Result{ Status: StatusFailed, Reason: fmt.Sprintf("invalid worker stdout JSON: %v", err), Logs: []string{stdoutOutput}, } + if h.opts.Verbose { + printResultBanner(string(StatusFailed), failure) + } + return failure + } + + // Reported before normalisation, so a worker that returned an unrecognised status + // sees what it actually sent rather than the rewritten failure — which is the whole + // point of asking for verbose output. + if h.opts.Verbose { + printResultBanner(parsed.Status, Result{ + Status: Status(parsed.Status), + Output: parsed.Output, + Logs: parsed.Logs, + Reason: parsed.Reason, + }) } return normalizeStdioResult(parsed) diff --git a/internal/taskworker/taskworker.go b/internal/taskworker/taskworker.go index 6db39e5..c89d215 100644 --- a/internal/taskworker/taskworker.go +++ b/internal/taskworker/taskworker.go @@ -85,11 +85,13 @@ func (t Task) InputData() (json.RawMessage, error) { // flavours shape failures differently — JavaScript workers report the message under an // "error" output key, stdio workers use ReasonForIncompletion plus logs — and those // differences are observable by workflows. +// The json tags exist because --verbose prints a Result back to the user, and that +// output was previously the stdio worker's own lowercase, omitempty-tagged shape. type Result struct { - Status Status - Output map[string]interface{} - Logs []string - Reason string + Status Status `json:"status"` + Output map[string]interface{} `json:"output,omitempty"` + Logs []string `json:"logs,omitempty"` + Reason string `json:"reason,omitempty"` } // Failure builds a Result for the common shape: FAILED with a reason and no output. @@ -161,13 +163,26 @@ func (w *Worker) Run(ctx context.Context, taskType string, h Handler) { } polled, err := w.runner.Poll(ctx, taskType) - if err != nil || len(polled) == 0 { + if err != nil { + // Logged every time rather than once: a persistent failure here (bad + // credentials, unreachable server) is the single most common reason a + // worker appears to do nothing, and the backoff keeps the volume sane. + log.Errorf("Error polling tasks: %v", err) if !sleep(ctx, w.cfg.PollBackoff) { return } continue } + if len(polled) == 0 { + log.Debug("No tasks available") + if !sleep(ctx, w.cfg.PollBackoff) { + return + } + continue + } + + log.Infof("Polled %d task(s)", len(polled)) w.runBatch(ctx, polled, h) } } From e4d21db2649460421a39b2ef5e68de1915baa805 Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Wed, 5 Aug 2026 15:15:02 -0300 Subject: [PATCH 6/7] Reconcile worker and skill docs with main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rebase onto main brought in #99's CLAUDE.md, which documents workers and skills independently of the sections this branch added — leaving two "Worker Commands" and two "Skill Commands" sections. Keeps #99's versions, which cover more commands and flags, and folds in what only this branch had: the shared-loop note, the three result contracts side by side, the shutdown and child-environment behaviour, the {skillName}__{tool} task types with their argv/stdout contract, and the WORKER_SKILL.md links. Also corrects #99's flag documentation to match this branch: worker js and worker remote take --poll-timeout rather than --timeout, worker js has no --exec-timeout because a goja script cannot be interrupted, and remote's --exec-timeout defaults to 100s. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 97 +++++++++++++++++++------------------------------------ 1 file changed, 33 insertions(+), 64 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f20d49f..3f68e93 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -187,65 +187,6 @@ Columns: NAME, VERSION, DESCRIPTION **Table Output (task list):** Columns: NAME, EXECUTABLE, DESCRIPTION, OWNER, TIMEOUT POLICY, TIMEOUT (s), RETRY COUNT, RESPONSE TIMEOUT (s) -### Worker Commands - -> **Note:** Workers are experimental. All flavours share one poll loop; they differ only in how user code is executed and in the result shape it returns. - -| Command | Description | Required Args | Optional Flags | Example | -|---------|-------------|---------------|----------------|---------| -| `worker stdio [args...]` | Run an external program per task; task JSON on stdin, result JSON on stdout | command | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--verbose` | `conductor worker stdio --type greet_task python3 worker.py` | -| `worker js ` | Run a JavaScript worker in the built-in interpreter | JS file | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout` | `conductor worker js --type greet_task worker.js` | -| `worker remote` | Run a worker downloaded from the Orkes job-runner registry (Orkes only) | None | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--refresh` | `conductor worker remote --type greet_task` | -| `worker list-remote` | List workers in the registry (Orkes only) | None | `--namespace` | `conductor worker list-remote` | - -**Flags:** -- `--type` - Task type to poll for (required for all worker commands) -- `--count` - Tasks polled per batch, executed in parallel (default 1). The next poll waits for the slowest task in the batch. -- `--poll-timeout` - Server-side long-poll wait in milliseconds (default 100) -- `--exec-timeout` - Per-task execution timeout in seconds (`stdio` and `remote` only; 0 = none, default 100 for `remote`) -- `--timeout` - Deprecated alias for `--poll-timeout` -- `--verbose` - Print task and result JSON (`stdio` only) - -**Result contracts** differ per flavour: - -| Flavour | Worker returns | Failure carries | -|---------|----------------|-----------------| -| `stdio` | `{"status","output","logs","reason"}` on stdout | `reasonForIncompletion` + logs | -| `js` | `{status, body}` from the script; `$.task` holds the task | `output.error` | -| skill tools | bare stdout, wrapped as `{"result": ...}` | `reasonForIncompletion` | - -Workers exit cleanly on Ctrl-C/SIGTERM. Child processes receive `TASK_TYPE`, `TASK_ID`, `WORKFLOW_ID`, `EXECUTION_ID`, `POLL_DOMAIN`, and the CLI's own `CONDUCTOR_SERVER_URL` and credentials. - -### Skill Commands - -A skill is a directory with `SKILL.md` (frontmatter `name` required) plus `scripts/`. Each script is served as the Conductor task type `{skillName}__{tool}`, so a skill tool can be called by an agent **or** by a plain workflow task. - -| Command | Description | Required Args | Optional Flags | Example | -|---------|-------------|---------------|----------------|---------| -| `skill list` | List registered skills | None | `--all-versions`, `--json` | `conductor skill list` | -| `skill get [version]` | Get a registered skill | name | `--version` | `conductor skill get myskill` | -| `skill register ` | Package and register a local skill | path | | `conductor skill register ./myskill` | -| `skill load ` | Package a local skill and deploy it as an agent | path | | `conductor skill load ./myskill` | -| `skill pull [dest]` | Download and extract a skill package | name | `--version` | `conductor skill pull myskill` | -| `skill delete [version]` | Delete a registered skill version | name | `--version` | `conductor skill delete myskill` | -| `skill run ` | Start local tool workers, run the agent, stream output | path/name, prompt | `--model` (required), `--param`, `--version`, workspace flags | `conductor skill run ./myskill "say hi" --model gpt-4o` | -| `skill serve ` | Start local tool workers only, block until interrupted | path/name | `--version`, workspace flags | `conductor skill serve ./myskill` | - -**Workspace flags** (`run` and `serve`): `--workspace` (default `.`), `--no-workspace`, `--filesystem name=path` (repeatable), `--script-timeout` (default 300s), `--script-output-limit` (default 10 MiB). - -**Tool contract:** `inputParameters.command` becomes the script's argv; stdout becomes `{"result": ""}`; non-zero exit fails the task. Script language is chosen by extension (`.py .sh .js .mjs .ts .rb .go .bat .cmd`). - -Using a skill tool as a plain worker: - -```bash -conductor skill serve ./myskill & -# workflow task named "greetskill__greet" with inputParameters {"command": "Miguel"} -conductor workflow start --workflow skill_as_worker --input '{"name":"Miguel"}' --sync -# { "result": "Hello Miguel\n" } -``` - -See [WORKER_SKILL.md](./WORKER_SKILL.md), [WORKER_STDIO.md](./WORKER_STDIO.md), [WORKER_JS.md](./WORKER_JS.md). - ### Config Commands | Command | Description | Required Args | Optional Flags | Example | @@ -486,15 +427,26 @@ Package local skill directories (a directory containing `SKILL.md`) and run them **`load` vs `run`:** `load` only publishes the agent (run it later with `agent run --name `); `run` starts local tool workers, launches the agent, and streams the execution. `serve` starts only the workers so the skill can be driven from elsewhere (e.g. the UI). +**Tool task types.** Each script in `scripts/` plus the built-in tools are served as the +Conductor task type `{skillName}__{tool}` — `read_skill_file`, and with a workspace enabled +`list_workspace_files`, `read_workspace_file`, `search_workspace`, `git_status`, `git_diff`. +`inputParameters.command` becomes the script's argv, stdout becomes `{"result": ""}`, +and a non-zero exit fails the task. Script language is chosen by extension +(`.py .sh .js .mjs .ts .rb .go .bat .cmd`). + +Because a tool is just a task type, a plain workflow can call one with no agent involved — +point a `SIMPLE` task at `{skillName}__{tool}` while `skill serve` is running. See +[WORKER_SKILL.md](./WORKER_SKILL.md). + ### Worker Commands Run task workers that poll Conductor and execute work locally. | Command | Description | Required Args | Optional Flags | Example | |---------|-------------|---------------|----------------|---------| -| `worker js ` | Run a JavaScript worker (EXPERIMENTAL) | JS file | `--type` (required), `--count`, `--worker-id`, `--domain`, `--timeout` | `conductor worker js worker.js --type my_task` | +| `worker js ` | Run a JavaScript worker (EXPERIMENTAL) | JS file | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout` | `conductor worker js worker.js --type my_task` | | `worker stdio [args...]` | Poll tasks and execute a command via stdin/stdout | command | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--verbose` | `conductor worker stdio ./handler.sh --type my_task` | -| `worker remote` | Run a worker from the job-runner registry (EXPERIMENTAL, Orkes only) | None | `--type` (required), `--count`, `--worker-id`, `--domain`, `--timeout`, `--refresh` | `conductor worker remote --type my_task` | +| `worker remote` | Run a worker from the job-runner registry (EXPERIMENTAL, Orkes only) | None | `--type` (required), `--count`, `--worker-id`, `--domain`, `--poll-timeout`, `--exec-timeout`, `--refresh` | `conductor worker remote --type my_task` | | `worker list-remote` | List workers in the job-runner registry (EXPERIMENTAL, Orkes only) | None | `--namespace` | `conductor worker list-remote` | **Flags:** @@ -502,13 +454,30 @@ Run task workers that poll Conductor and execute work locally. - `--count` - Number of tasks to poll in each batch (default: 1) - `--worker-id` - Worker ID reported to the server - `--domain` - Task domain -- `--timeout` / `--poll-timeout` - Poll timeout in milliseconds (default: 100) -- `--exec-timeout` - Execution timeout in seconds for `stdio` (default: 0 = no timeout) +- `--poll-timeout` - Server-side long-poll wait in milliseconds (default: 100) +- `--exec-timeout` - Per-task execution timeout in seconds. `stdio` and `remote` only — a + JavaScript worker runs in-process with no interrupt, so there is nothing to time out. + Default 0 (no timeout) for `stdio`, 100 for `remote`. +- `--timeout` - Deprecated hidden alias for `--poll-timeout` - `--verbose` - Print task and result JSON to stdout (`stdio` command) - `--refresh` - Force refresh the worker from the registry, ignoring cache - `--namespace` - Registry namespace to list workers from (default: `default`) -See [WORKER_JS.md](./WORKER_JS.md) and [WORKER_STDIO.md](./WORKER_STDIO.md) for the worker protocols. +All flavours share one poll loop; they differ only in how user code runs and in the result +shape it returns: + +| Flavour | Worker returns | Failure carries | +|---------|----------------|-----------------| +| `stdio` | `{"status","output","logs","reason"}` on stdout | `reasonForIncompletion` + logs | +| `js` | `{status, body}` from the script; `$.task` holds the task | `output.error` | +| skill tools | bare stdout, wrapped as `{"result": ...}` | `reasonForIncompletion` | + +Workers exit on Ctrl-C/SIGTERM once the in-flight batch finishes; a second signal exits +immediately. Child processes receive `TASK_TYPE`, `TASK_ID`, `WORKFLOW_ID`, `EXECUTION_ID`, +`POLL_DOMAIN`, and the CLI's own `CONDUCTOR_SERVER_URL` and credentials. + +See [WORKER_JS.md](./WORKER_JS.md), [WORKER_STDIO.md](./WORKER_STDIO.md) and +[WORKER_SKILL.md](./WORKER_SKILL.md) for the worker protocols. ### Development Commands From 133c74578512a6bca8a09bb834bf25e28f1b7ad2 Mon Sep 17 00:00:00 2001 From: Miguel Prieto Date: Wed, 5 Aug 2026 15:54:09 -0300 Subject: [PATCH 7/7] Let shutdown drain in-flight tasks instead of failing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review found that graceful shutdown was reporting failures the worker inflicted on itself. StdioHandler derived the child's context from the loop's, so the first Ctrl-C SIGKILLed every running child; runAndParse turned that into FAILED / "worker execution failed: signal: killed", and because results are delivered on a context that outlives cancellation, the server received it. That is worse than the behaviour it replaced. Previously Ctrl-C killed the CLI before any update, leaving the task IN_PROGRESS for the server to requeue after responseTimeoutSeconds. Now a clean shutdown consumed one of the task's retries, and failed the workflow outright when retryCount was 0. It also contradicted both the loop's own doc comment and CLAUDE.md, which promise that Run returns once the in-flight batch finishes. The child is now detached from the loop context, so a running task completes and reports its real result. Its execution timeout still applies. A task that will never finish is covered by the second interrupt, which exits the process. Verified end to end: SIGTERM while a 3s task is in flight now yields COMPLETED with the worker's own output, where it previously yielded FAILED. Also from the same review: - skill run and skill serve still used signal.NotifyContext, the pattern this branch replaced precisely because it swallows every signal after the first. Both now use interruptWithEscalation, so a stream or tool script that ignores cancellation cannot leave the process unkillable. - workerPollFlags had no tests, though it is where the #91 fix lives. Added coverage for the --timeout alias precedence, the exec-timeout default that preserves remote's old effective behaviour, and the flag surface each command exposes. - "Polled N task(s)" moves to Debug. skill run starts one loop per tool type and streams agent output to the same terminal, so an Info line per poll buried the stream. Poll errors stay at Error — a silently idle worker is the failure that logging exists to surface. - printResultBanner took a status parameter that duplicated Result.Status. - Corrected the addPollTimeoutFlags comment: the alias is registered on every worker subcommand, including stdio, which never shipped with --timeout. - Documented that InputData yields "null" for an empty-but-non-nil input map, where marshalling the map directly gave "{}". Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 +- cmd/skill_run.go | 14 ++- cmd/worker.go | 6 +- cmd/worker_flags_test.go | 183 ++++++++++++++++++++++++++++++ internal/taskworker/stdio.go | 22 ++-- internal/taskworker/stdio_test.go | 39 +++++-- internal/taskworker/taskworker.go | 12 +- 7 files changed, 255 insertions(+), 27 deletions(-) create mode 100644 cmd/worker_flags_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 3f68e93..78d5731 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -472,8 +472,10 @@ shape it returns: | `js` | `{status, body}` from the script; `$.task` holds the task | `output.error` | | skill tools | bare stdout, wrapped as `{"result": ...}` | `reasonForIncompletion` | -Workers exit on Ctrl-C/SIGTERM once the in-flight batch finishes; a second signal exits -immediately. Child processes receive `TASK_TYPE`, `TASK_ID`, `WORKFLOW_ID`, `EXECUTION_ID`, +Workers exit on Ctrl-C/SIGTERM once the in-flight batch finishes — a running task is left +to complete and report its real result rather than being killed, which would report a +failure the worker inflicted on itself and consume one of the task's retries. A second +signal exits immediately. Child processes receive `TASK_TYPE`, `TASK_ID`, `WORKFLOW_ID`, `EXECUTION_ID`, `POLL_DOMAIN`, and the CLI's own `CONDUCTOR_SERVER_URL` and credentials. See [WORKER_JS.md](./WORKER_JS.md), [WORKER_STDIO.md](./WORKER_STDIO.md) and diff --git a/cmd/skill_run.go b/cmd/skill_run.go index 9ee810d..96c27f8 100644 --- a/cmd/skill_run.go +++ b/cmd/skill_run.go @@ -17,11 +17,8 @@ import ( "context" "encoding/json" "fmt" - "os" - "os/signal" "path/filepath" "strings" - "syscall" "time" "github.com/spf13/cobra" @@ -120,8 +117,11 @@ func runSkillRun(cmd *cobra.Command, args []string) error { // One signal-aware context governs both the workers and the stream; cancelling // it (Ctrl-C) stops everything. Workers are also cancelled when the execution - // ends normally. - ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + // ends normally. A second interrupt exits outright, so a stream or tool script + // that ignores cancellation cannot leave the process unkillable. + ctx, cancel := context.WithCancel(cmd.Context()) + defer cancel() + stop := interruptWithEscalation(cancel) defer stop() workerCtx, cancelWorkers := context.WithCancel(ctx) defer cancelWorkers() @@ -153,7 +153,9 @@ func runSkillServe(cmd *cobra.Command, args []string) error { return err } - ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + ctx, cancel := context.WithCancel(cmd.Context()) + defer cancel() + stop := interruptWithEscalation(cancel) defer stop() startSkillWorkers(ctx, buildSkillWorkerRegistry(cfg, local, ws, scriptOptions(), skillWorkspaceFileLimit)) diff --git a/cmd/worker.go b/cmd/worker.go index 537ffb8..42e2405 100644 --- a/cmd/worker.go +++ b/cmd/worker.go @@ -725,8 +725,10 @@ func workerPollFlags(cmd *cobra.Command) (taskworker.RunnerOptions, time.Duratio return opts, time.Duration(execSeconds) * time.Second } -// addPollTimeoutFlags registers the two timeout flags on a worker subcommand, plus the -// deprecated --timeout alias that `worker js` and `worker remote` shipped with. +// addPollTimeoutFlags registers the timeout flags on a worker subcommand, plus a hidden +// deprecated --timeout alias. `worker js` and `worker remote` shipped with --timeout; +// `worker stdio` did not, but it accepts the alias too so that one spelling works +// everywhere rather than the alias being another per-command difference. // // The alias is hidden rather than removed so existing invocations keep working; it maps // to --poll-timeout only. See workerPollFlags and issue #91. diff --git a/cmd/worker_flags_test.go b/cmd/worker_flags_test.go new file mode 100644 index 0000000..590a63f --- /dev/null +++ b/cmd/worker_flags_test.go @@ -0,0 +1,183 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package cmd + +import ( + "testing" + "time" + + "github.com/spf13/cobra" +) + +// workerFlagCmd builds a throwaway command carrying the same flags a worker subcommand +// registers, so the flag plumbing can be tested without running a worker. +func workerFlagCmd(t *testing.T, execTimeout bool, execTimeoutDefault int32, args ...string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{Use: "fake"} + cmd.Flags().String("worker-id", "", "") + cmd.Flags().String("domain", "", "") + cmd.Flags().Int32("count", 1, "") + addPollTimeoutFlags(cmd, execTimeout, execTimeoutDefault) + + if err := cmd.ParseFlags(args); err != nil { + t.Fatalf("ParseFlags(%v) error = %v", args, err) + } + return cmd +} + +// TestWorkerPollFlagsTimeoutAlias is the regression test for issue #91: `worker remote` +// fed a single --timeout value to both the poll wait (milliseconds) and the execution +// budget (seconds), so one number meant two different things in two different units. +func TestWorkerPollFlagsTimeoutAlias(t *testing.T) { + tests := []struct { + name string + args []string + wantPollMs int32 + wantExecSecs time.Duration + }{ + { + name: "no flags uses defaults", + args: nil, + wantPollMs: 100, + wantExecSecs: 0, + }, + { + name: "poll-timeout alone", + args: []string{"--poll-timeout", "500"}, + wantPollMs: 500, + wantExecSecs: 0, + }, + { + name: "deprecated timeout alias maps to poll only", + args: []string{"--timeout", "4000"}, + wantPollMs: 4000, + wantExecSecs: 0, + }, + { + name: "poll-timeout wins when both are given", + args: []string{"--timeout", "4000", "--poll-timeout", "250"}, + wantPollMs: 250, + wantExecSecs: 0, + }, + { + name: "exec-timeout is independent of the poll wait", + args: []string{"--poll-timeout", "250", "--exec-timeout", "30"}, + wantPollMs: 250, + wantExecSecs: 30 * time.Second, + }, + { + // The #91 bug: one value must not become both a millisecond poll wait and a + // second-denominated execution budget. + name: "timeout alias does not also set the exec budget", + args: []string{"--timeout", "100"}, + wantPollMs: 100, + wantExecSecs: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := workerFlagCmd(t, true, 0, tt.args...) + opts, execTimeout := workerPollFlags(cmd) + + if opts.PollTimeoutMs != tt.wantPollMs { + t.Errorf("PollTimeoutMs = %d, want %d", opts.PollTimeoutMs, tt.wantPollMs) + } + if execTimeout != tt.wantExecSecs { + t.Errorf("execTimeout = %v, want %v", execTimeout, tt.wantExecSecs) + } + }) + } +} + +// TestWorkerPollFlagsRemoteExecTimeoutDefault pins the default that preserves the old +// effective behaviour: --timeout 100 previously reached the child as a 100 second kill +// timer, so defaulting remote's --exec-timeout to 0 would let a hanging worker run forever. +func TestWorkerPollFlagsRemoteExecTimeoutDefault(t *testing.T) { + cmd := workerFlagCmd(t, true, 100) + _, execTimeout := workerPollFlags(cmd) + + if execTimeout != 100*time.Second { + t.Errorf("execTimeout = %v, want 100s", execTimeout) + } +} + +// TestWorkerPollFlagsWithoutExecTimeout covers `worker js`, which registers no +// --exec-timeout because a goja script cannot be interrupted. Reading the flag must not +// panic on the command that lacks it. +func TestWorkerPollFlagsWithoutExecTimeout(t *testing.T) { + cmd := workerFlagCmd(t, false, 0, "--poll-timeout", "300") + opts, execTimeout := workerPollFlags(cmd) + + if opts.PollTimeoutMs != 300 { + t.Errorf("PollTimeoutMs = %d, want 300", opts.PollTimeoutMs) + } + if execTimeout != 0 { + t.Errorf("execTimeout = %v, want 0 when the flag is not registered", execTimeout) + } +} + +func TestWorkerPollFlagsPassesThroughIdentity(t *testing.T) { + cmd := workerFlagCmd(t, true, 0, "--worker-id", "w1", "--domain", "prod", "--count", "5") + opts, _ := workerPollFlags(cmd) + + if opts.WorkerID != "w1" { + t.Errorf("WorkerID = %q, want w1", opts.WorkerID) + } + if opts.Domain != "prod" { + t.Errorf("Domain = %q, want prod", opts.Domain) + } + if opts.Count != 5 { + t.Errorf("Count = %d, want 5", opts.Count) + } + if opts.UseTaskWorkerID { + t.Error("UseTaskWorkerID = true, want false — only JavaScript workers set it") + } +} + +// TestJsRunnerOptionsReportsTaskWorkerID pins that JavaScript workers report the polled +// task's own worker id, which is observable on the task result. +func TestJsRunnerOptionsReportsTaskWorkerID(t *testing.T) { + cmd := workerFlagCmd(t, false, 0, "--worker-id", "ignored-for-js") + opts, _ := workerPollFlags(cmd) + + if got := jsRunnerOptions(opts); !got.UseTaskWorkerID { + t.Error("jsRunnerOptions() did not set UseTaskWorkerID") + } +} + +// TestAddPollTimeoutFlagsRegistration checks the flag surface each worker command exposes: +// --timeout must be hidden everywhere, and --exec-timeout absent where it cannot be honoured. +func TestAddPollTimeoutFlagsRegistration(t *testing.T) { + withExec := workerFlagCmd(t, true, 0) + if withExec.Flags().Lookup("exec-timeout") == nil { + t.Error("--exec-timeout not registered when requested") + } + + withoutExec := workerFlagCmd(t, false, 0) + if withoutExec.Flags().Lookup("exec-timeout") != nil { + t.Error("--exec-timeout registered on a command that cannot honour it") + } + + alias := withExec.Flags().Lookup("timeout") + if alias == nil { + t.Fatal("--timeout alias not registered") + } + if !alias.Hidden { + t.Error("--timeout is not hidden; the deprecated alias should not appear in help") + } + if alias.Deprecated == "" { + t.Error("--timeout is not marked deprecated") + } +} diff --git a/internal/taskworker/stdio.go b/internal/taskworker/stdio.go index 1e12ad3..b635c81 100644 --- a/internal/taskworker/stdio.go +++ b/internal/taskworker/stdio.go @@ -72,13 +72,21 @@ func (h *StdioHandler) Handle(ctx context.Context, t Task) Result { fmt.Println("==================") } + // The child is deliberately detached from the loop's cancellation. A task already + // running should finish and report its real result, which is what Run promises + // ("returns once the in-flight batch finishes"). Killing it on Ctrl-C instead makes + // the worker report a FAILED it inflicted on itself — and because results are + // delivered on a context that outlives cancellation, the server sees that failure + // and consumes one of the task's retries. A child that will not finish is handled by + // the second interrupt, which exits the process outright. + execCtx := context.WithoutCancel(ctx) if h.opts.ExecTimeout > 0 { var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, h.opts.ExecTimeout) + execCtx, cancel = context.WithTimeout(execCtx, h.opts.ExecTimeout) defer cancel() } - cmd := exec.CommandContext(ctx, h.opts.Command, h.opts.Args...) + cmd := exec.CommandContext(execCtx, h.opts.Command, h.opts.Args...) cmd.Env = append(cmd.Environ(), "TASK_TYPE="+t.Type, "TASK_ID="+t.ID, @@ -106,9 +114,9 @@ func (h *StdioHandler) Handle(ctx context.Context, t Task) Result { // printResultBanner reports a result under --verbose. The banner distinguishes failures // so they stand out in a stream of task output. -func printResultBanner(status string, result Result) { +func printResultBanner(result Result) { resultJSON, _ := json.MarshalIndent(result, "", " ") - if Status(status) == StatusFailed { + if result.Status == StatusFailed { fmt.Println("=== Task Result (Error) ===") fmt.Println(string(resultJSON)) fmt.Println("===========================") @@ -133,7 +141,7 @@ func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) Logs: []string{stderrOutput}, } if h.opts.Verbose { - printResultBanner(string(StatusFailed), failure) + printResultBanner(failure) } return failure } @@ -149,7 +157,7 @@ func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) Logs: []string{stdoutOutput}, } if h.opts.Verbose { - printResultBanner(string(StatusFailed), failure) + printResultBanner(failure) } return failure } @@ -158,7 +166,7 @@ func (h *StdioHandler) runAndParse(cmd *exec.Cmd, stdout, stderr *bytes.Buffer) // sees what it actually sent rather than the rewritten failure — which is the whole // point of asking for verbose output. if h.opts.Verbose { - printResultBanner(parsed.Status, Result{ + printResultBanner(Result{ Status: Status(parsed.Status), Output: parsed.Output, Logs: parsed.Logs, diff --git a/internal/taskworker/stdio_test.go b/internal/taskworker/stdio_test.go index 3f09a83..56bd81e 100644 --- a/internal/taskworker/stdio_test.go +++ b/internal/taskworker/stdio_test.go @@ -153,14 +153,39 @@ func TestStdioHandlerExecTimeoutKillsChild(t *testing.T) { } } -func TestStdioHandlerCancelledContextStopsChild(t *testing.T) { - h := NewStdioHandler(StdioOptions{Command: "sleep", Args: []string{"30"}}) +// TestStdioHandlerCancelledContextLetsTaskFinish pins that shutdown does not fail an +// in-flight task. Killing the child on Ctrl-C made the worker report +// "worker execution failed: signal: killed" — a failure it inflicted on itself, which the +// server then counts against the task's retries. The child is detached from the loop +// context so it finishes and reports its real result. +func TestStdioHandlerCancelledContextLetsTaskFinish(t *testing.T) { + h := NewStdioHandler(shWorker(`sleep 0.3; echo '{"status":"COMPLETED","output":{"done":true}}'`)) ctx, cancel := context.WithCancel(context.Background()) - go func() { - time.Sleep(50 * time.Millisecond) - cancel() - }() + cancel() // already shutting down when the task starts + + got := h.Handle(ctx, stdioTask()) + + if got.Status != StatusCompleted { + t.Errorf("Status = %q, want COMPLETED — shutdown must not fail an in-flight task", got.Status) + } + if strings.Contains(got.Reason, "killed") { + t.Errorf("Reason = %q — the child was killed by shutdown", got.Reason) + } + if got.Output["done"] != true { + t.Errorf("Output = %v, want the worker's real result", got.Output) + } +} + +// TestStdioHandlerExecTimeoutStillAppliesAfterCancel guards the other half: detaching the +// child from cancellation must not also detach it from its execution timeout. +func TestStdioHandlerExecTimeoutStillAppliesAfterCancel(t *testing.T) { + opts := StdioOptions{Command: "sleep", Args: []string{"30"}} + opts.ExecTimeout = 100 * time.Millisecond + h := NewStdioHandler(opts) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() start := time.Now() got := h.Handle(ctx, stdioTask()) @@ -169,7 +194,7 @@ func TestStdioHandlerCancelledContextStopsChild(t *testing.T) { t.Errorf("Status = %q, want FAILED", got.Status) } if elapsed := time.Since(start); elapsed > 5*time.Second { - t.Errorf("took %v — cancelling the context did not stop the child", elapsed) + t.Errorf("took %v — the exec timeout stopped applying once the child was detached", elapsed) } } diff --git a/internal/taskworker/taskworker.go b/internal/taskworker/taskworker.go index c89d215..35474a7 100644 --- a/internal/taskworker/taskworker.go +++ b/internal/taskworker/taskworker.go @@ -63,8 +63,11 @@ type Task struct { } // InputData returns just the task's inputData, for handlers that want the input rather -// than the whole task. A task with no inputData yields "null", matching what the skill -// worker produced previously by marshalling a nil map. +// than the whole task. A task with no inputData yields "null". +// +// model.Task.InputData is tagged omitempty, so an empty-but-non-nil map is absent from +// Raw and also yields "null" here, where marshalling the map directly would have given +// "{}". Handlers decode into structs, so both produce the same zero values. func (t Task) InputData() (json.RawMessage, error) { var envelope struct { InputData json.RawMessage `json:"inputData"` @@ -182,7 +185,10 @@ func (w *Worker) Run(ctx context.Context, taskType string, h Handler) { continue } - log.Infof("Polled %d task(s)", len(polled)) + // Debug, not Info: skill run starts one loop per tool type and streams agent output + // to the same terminal, so an Info line here buries the stream. Poll *errors* stay + // at Error — a silently idle worker is the failure this logging exists to surface. + log.Debugf("Polled %d task(s)", len(polled)) w.runBatch(ctx, polled, h) } }