Support per-node-type activity worker routing (dedicated Docker images/tools per node type) #141

Activity

1. tbrandenburg commented on Sep 14, 2026

   

   tbrandenburg

   on Sep 14, 2026

   Author

   More actions

   Open Question 1 — Worker secrets/config

   Verdict: Yes — each specialized worker gets its own .env/.env.example and its own env.ts, following the pattern already established by apps/backend and apps/execution-worker today.

   Justification:

   This repo's existing convention is one deployable unit = one env contract, not a shared/global config:

   • apps/backend/.env.example and apps/execution-worker/.env.example are separate files with overlapping-but-distinct keys (OPENROUTER_API_KEY, AI_MODEL appear in both, independently), and the comment in apps/backend/.env.example:16 is explicit: "The execution worker keeps its own key for running workflows" — this is the precedent question already answered once, for a different key.
   • apps/execution-worker/src/env.ts implements requireEnv/envOr as a local, fail-fast module scoped to that package — not imported from apps/backend or any shared @workflow-builder/* package. There's no shared env module in the monorepo today; each app owns its full contract.
   • deploy/ai-studio/docker-compose.yml reinforces this at the ops layer: each service gets its own environment: block wired at the compose level, with unit-specific required-vs-optional treatment (e.g. OPENROUTER_API_KEY: ${OPENROUTER_API_KEY:?set ...} for backend).

   A specialized worker (e.g. apps/execution-worker-coding-agent) will need its own credentials (its own model/API key for its dedicated image, possibly a different provider or scoped token) that the main worker has no reason to hold or fail-fast-check for. Making it depend on or extend the main worker's env.ts would either force irrelevant vars into the main worker's contract or create an implicit coupling between two independently-deployed images — contradicting the "each Docker image is a separate deployable unit" premise of this issue itself.

   Recommendation: Repeat the pattern exactly — new apps/<name>/ directory, own package.json, own .env.example, own env.ts with requireEnv/envOr. Do not add conditional/optional secrets to the shared execution-worker env module. taskQueue routing determines which worker picks up an activity; it says nothing about how that worker is configured, and this repo's existing convention already treats those as orthogonal.
2. tbrandenburg commented on Sep 14, 2026

   

   tbrandenburg

   on Sep 14, 2026

   Author

   More actions

   Open Question 2 — Heartbeats vs. timeout

   Verdict: Yes — heartbeats, and they should be a first-class ActivityProfile field, not bolted on separately.

   Why timeout alone is insufficient here:

   DEFAULT_NODE_ACTIVITY_PROFILE already picks a generous startToCloseTimeout with a low retry cap specifically to bound cost on partial failures (activity-profiles.ts:25-30). That reasoning holds for an LLM call that either finishes or errors quickly. A CLI-agent subprocess is a different failure mode: it can hang — wedged on stdin, a stuck child process, a network partition mid-stream — without erroring and without finishing. With only startToCloseTimeout, Temporal has no signal that the activity is dead until the full timeout elapses, so a 30-minute profile means up to 30 minutes wasted per attempt before a single retry fires, multiplied by retry.maximumAttempts. A heartbeatTimeout lets the activity self-report liveness on a much shorter cadence (e.g. every 30s) so a stalled worker slot is reclaimed and retried in seconds, not the entire timeout window — while startToCloseTimeout still bounds the case where it's alive but genuinely slow.

   Fits the existing pattern:

   node-activity-options.ts:37-53 and profile-validation.ts already treat a profile as a closed, whole-object shape (PROFILE_KEYS, RETRY_KEYS) explicitly to avoid silently inheriting Temporal defaults (README:96). heartbeatTimeout should follow the same rule — optional, but validated the same way as startToCloseTimeout (same DurationString + protobuf-Duration bounds check) — rather than becoming an implicit default that only long-running node types remember to set.

   Concrete change:

   // activity-profiles.ts
   export type ActivityProfile = {
     startToCloseTimeout: DurationString;
     heartbeatTimeout?: DurationString; // required for activities that call Context.current().heartbeat()
     retry: { maximumAttempts: number };
   };

   Add heartbeatTimeout to PROFILE_KEYS in profile-validation.ts, pass it through in resolveFromValidatedProfiles, and document in the README that any node executor doing long-running subprocess/stream work must call heartbeat() on an interval shorter than its profile's heartbeatTimeout — otherwise Temporal cancels it as stalled.
3. tbrandenburg commented on Sep 14, 2026

   

   tbrandenburg

   on Sep 14, 2026

   Author

   More actions

   Open Question 3 — Event shape for CLI output

   Verdict: reuse the existing event pipeline as-is (option a) — no new event type needed for the MVP.

   Why:

   • execution_events.payload_json is already an untyped jsonb column (apps/backend/src/db/schema.ts:55) and emitEvent(executionId, type, payload?: unknown, nodeId?) (apps/execution-worker/src/database.ts:15) accepts any JSON-serializable payload. A captured stdout/stderr blob fits without a migration.
   • The graph runner's event model is strictly binary per node today: node_started → one of node_completed / node_failed / node_skipped (packages/execution-core/src/graph-runner.ts:312-321). There is no existing notion of a partial/intermediate event for a single node — adding one would be a new concept threaded through the runner, the SSE formatEvent (apps/backend/src/routes/executions.ts:237), and the frontend execution-log renderer, not just the worker.
   • Precedent: ai-agent.ts already returns the entire result as one { output: { response: ... } } payload on completion (apps/execution-worker/src/activities/ai-agent.ts:59), and errors are logged in one shot via logger?.error('llm call failed', {...}) (:63) rather than streamed. A CLI-agent activity should follow the same convention: buffer stdout/stderr for the run and attach it to the single node_completed/node_failed payload.

   Recommendation:
   Ship the CLI-agent node with buffered output on node_completed/node_failed (zero schema change, zero new SSE/UI work) as the MVP for this issue's routing work. Track true incremental streaming (a node_progress/partial event type, new SSE handling, log-panel append-as-you-go UI) as an explicit, separate follow-up once per-node-type worker routing has landed — same incremental-scope pattern the replay-test backlog already uses for "scenarios still worth adding." Don't couple the routing change to a streaming-UX change; they're independently valuable and independently risky.
4. tbrandenburg commented on Sep 14, 2026

   

   tbrandenburg

   on Sep 14, 2026

   Author

   More actions

   Open Question 4 — Replica configurability

   Verdict: No — don't add a static replica-count config field; use docker compose up --scale.

   Why:

   • deploy/ai-studio/docker-compose.yml has zero deploy:/replicas blocks today — backend and worker are both defined as strictly single-instance services (restart: unless-stopped only, no scaling primitive at all). There's no existing pattern to extend, only one to introduce.
   • The worker service (docker-compose.yml:99-115) already differs from backend purely by command:, sharing one ai-studio-runtime image built from a single runtime target (Dockerfile:19-30, comment: "backend + worker, command chosen per compose service"). That's this repo's established precedent for "N behaviors, one image" — it argues for adding new specialized worker services (new command:/task-queue per node-type group), not a new config schema for how many of each to run.
   • Docker Compose already ships the mechanism for this: docker compose up --scale worker-coding-agent=3 --scale worker-default=1. That's free, needs zero code or schema changes, and works today with plain docker compose (no Swarm/deploy: block required for scaling itself, just for resource limits if ever needed).
   • No Kubernetes/Nomad/Helm manifests exist anywhere in the repo — introducing a bespoke replica-count config or orchestration layer would go against this project's stated KISS/YAGNI stance ("don't overengineer," "justify every line of code you write").

   Recommendation: When this issue lands, give each specialized worker its own compose service (own command:/task-queue env, same shared ai-studio-runtime image), and document docker compose up --scale <service>=N per service as the scaling story. Revisit only if/when the project actually adopts a real orchestrator beyond docker-compose — it doesn't today.
5. piotrblaszczyk commented on Sep 16, 2026

   

   piotrblaszczyk

   on Sep 16, 2026

   Contributor

   More actions

   Replied across all three in #148. Short version: a direction we want and it's on our roadmap. Keeping this open as the design record.

   👍React with 👍1Reacted by Tom Brandenburg
6. piotrblaszczyk

   added

   roadmap

   on Sep 16, 2026