From d3de014723f3260477e75a729844754ea0d1c487 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 7 Apr 2026 10:30:40 -0400 Subject: [PATCH] plans --- ...26-04-06-object-backed-artifact-uploads.md | 75 ++++++ ...process-run-workers-signal-control-plan.md | 237 ++++++++++++++++++ 2 files changed, 312 insertions(+) create mode 100644 docs/plans/2026-04-06-object-backed-artifact-uploads.md create mode 100644 docs/plans/2026-04-06-subprocess-run-workers-signal-control-plan.md diff --git a/docs/plans/2026-04-06-object-backed-artifact-uploads.md b/docs/plans/2026-04-06-object-backed-artifact-uploads.md new file mode 100644 index 000000000..cd1a15a83 --- /dev/null +++ b/docs/plans/2026-04-06-object-backed-artifact-uploads.md @@ -0,0 +1,75 @@ +# Object-Backed Artifact Uploads + +## Summary +- Make `ArtifactStore` the durable source of truth for all stage artifacts, backed by a configurable object store that can be local filesystem or S3. +- Keep the server as the only durable artifact writer. Worker subprocesses upload artifacts to the server over the existing stage-artifacts POST route; they do not write `ArtifactStore` directly and do not use the run scratch directory as IPC. +- Ship both upload modes on `POST /api/v1/runs/{id}/stages/{stageId}/artifacts`: + - `application/octet-stream` for single-file upload + - `multipart/form-data` for batch upload on the same path +- Default artifact-upload failure policy is non-fatal: after bounded retries, the worker emits a warning notice and the run continues. +- Multipart uploads are a strict wire format: the `manifest` part must arrive first so the server can validate the batch before accepting file bytes. + +## Key Changes +- Add artifact storage configuration separate from the main run-store path: + - default backend: local object store rooted under the existing storage directory + - optional backend: S3 with bucket, region, prefix, optional endpoint override, and path-style toggle +- Add an artifact-storage capability marker to new runs so the server can distinguish object-backed runs from legacy disk-backed runs when deciding whether local fallback is allowed. +- Refactor server startup so `ArtifactStore` is constructed from artifact-storage config instead of being hardwired to `LocalFileSystem` in `serve.rs`. +- Extend `ArtifactStore` with streaming writes: + - add `put_stream(...)` that writes directly to the configured object store using `object_store::buffered::BufWriter` + - use the same deterministic object key layout as today so retries are idempotent +- Replace the buffered server artifact upload handler with a streaming implementation: + - `application/octet-stream`: requires `filename` query param, streams request body into `ArtifactStore` + - `multipart/form-data`: requires a JSON `manifest` part first, followed by file parts; the manifest is the canonical source of artifact paths and optional checksums/content types +- Add multipart request types to the OpenAPI spec: + - `ArtifactBatchUploadManifest` + - `ArtifactBatchUploadEntry` + - one entry per file part, keyed by part name and relative artifact path +- Validate both octet-stream `filename` query params and multipart manifest paths with the existing relative-path rules; reject traversal, empty segments, duplicate manifest paths, duplicate part names, missing parts, and unexpected parts. +- Compute and verify `sha256` during upload when provided by the client. If omitted, accept the upload without checksum enforcement. Native backend checksum features such as S3-specific checksum headers are optional optimizations, not part of the v1 contract. +- Enforce explicit server-side limits: + - maximum single artifact size + - maximum artifacts per multipart request + - maximum total multipart request bytes + - reject uploads that exceed these limits before durable commit when possible, and abort active multipart writes when the limit is crossed mid-stream +- For multipart requests, return non-2xx on the first failed file and leave already-written objects in place. Retries are safe because object keys are deterministic; v1 does not attempt cross-object rollback. +- Treat concurrent uploads to the same `{run_id, stage_id, path}` as idempotent-safe retries. Deterministic object keys mean duplicate concurrent uploads may race, but the final durable object must be equivalent regardless of which writer wins. +- Update worker subprocess behavior: + - after a stage captures artifacts locally, the worker uploads them to the server over the internal artifact route + - single-file uploads may use raw octet-stream; batch upload should use multipart when multiple artifacts exist for the stage + - the worker emits `artifact.captured` only after the server confirms durable upload + - on repeated upload failure, the worker emits a warning-style run notice and continues +- Update read paths so `ArtifactStore` is the primary source for list/download. Keep the current run-scratch fallback only for legacy runs created before this change; new runs should not rely on local disk artifacts being present on the server. +- Extend the worker spawn contract so the server provides: + - internal server address or Unix-socket target + - a short-lived bearer token scoped to artifact upload routes for that run +- Accept that interrupted multipart uploads may leave orphaned objects in v1. Record this as operational debt and add a later cleanup pass or age-based GC policy for abandoned artifact objects. + +## Public Interfaces +- `POST /api/v1/runs/{id}/stages/{stageId}/artifacts` accepts both `application/octet-stream` and `multipart/form-data`. +- Multipart wire format: + - one `manifest` JSON part first + - one file part per manifest entry + - manifest fields: `part`, `path`, optional `sha256`, optional `expected_bytes`, optional `content_type` +- Settings gain artifact object-store configuration, with local as the default and S3 as an explicit opt-in backend. +- New runs record an artifact-storage capability/version marker so read paths can determine whether disk fallback is valid. + +## Test Plan +- Server integration: single-file octet-stream upload stores objects in local object store and returns `204`. +- Server integration: multipart batch upload with manifest stores every artifact under the expected object keys and returns `204`. +- Validation: invalid artifact path, invalid octet-stream `filename`, duplicate path, duplicate part name, missing file part, unexpected file part, malformed manifest, and manifest-not-first all return `400`. +- Integrity: checksum mismatch returns error and does not mark the artifact as captured. +- Limits: oversized single artifact, oversized batch byte total, and too many multipart entries all fail with the configured limit response and abort the active upload. +- Retry behavior: partial multipart failure can be retried safely and produces the correct final artifact set. +- Concurrency: concurrent uploads to the same run/stage/path converge safely to one correct durable object. +- Read path: list/download returns artifacts from `ArtifactStore` for new runs and still falls back to disk for legacy runs. +- Worker integration: worker uploads captured artifacts through the server route and emits `artifact.captured` only after success. +- Failure behavior: repeated upload failure emits a warning notice and the run completes without failing. +- Backend coverage: large artifact upload works against an S3-compatible backend such as MinIO and uses streaming/multipart object-store writes without buffering the full file in memory. + +## Assumptions +- Scope is limited to `ArtifactStore` stage artifacts. Large run blobs written by `RunDatabase::write_blob()` are unchanged. +- New runs are expected to have durable artifacts in object storage; the server-side disk fallback exists only for backward compatibility with older runs. +- The server remains the sole durable artifact writer and owns all artifact object-store credentials. +- Non-fatal artifact-upload failure is the chosen default for v1; if strict durability becomes required later, it can be added as a separate policy change. +- The artifact object key remains scoped by run id, stage id, and relative artifact path, so same-key retries are naturally idempotent. diff --git a/docs/plans/2026-04-06-subprocess-run-workers-signal-control-plan.md b/docs/plans/2026-04-06-subprocess-run-workers-signal-control-plan.md new file mode 100644 index 000000000..b456225bd --- /dev/null +++ b/docs/plans/2026-04-06-subprocess-run-workers-signal-control-plan.md @@ -0,0 +1,237 @@ +# Subprocess Run Workers And Signal Control + +## Summary +- Move workflow execution out of `fabro server` into one hidden worker subprocess per run. +- Use Unix signals for lifecycle control, but keep the server as the sole durable writer for run events, projections, and summaries. +- Stream canonical worker events back to the server over the worker stdout pipe so the event log, SSE, and API status stay coherent without multi-process append races. + +## Scope Boundaries +In scope: +- replace in-process run execution with supervised worker subprocesses +- repurpose hidden `fabro __runner` into hidden `fabro __run-worker` +- use signals for `cancel`, `pause`, and `unpause` +- make the server the only writer to a run's durable event stream +- define a worker spawn contract, stdout/stderr contract, and control priority rules +- add request and effect events for run control +- phase delivery so subprocess cancel lands before pause and procline polish + +Out of scope: +- non-Unix parity for worker supervision +- crash-time worker reattachment in the first delivery +- changing the current public control routes away from `/runs/{id}/cancel`, `/pause`, and `/unpause` +- changing terminal cancellation away from durable `status=failed` with `status_reason=cancelled` + +## Problem Frame +The intended architecture is server-supervised subprocess workers, not server-local async tasks. The current plan also assumed both server and worker could append to the same run event stream, but the current `RunDatabase` writer is process-local: sequence allocation, event cache, and watch fanout are held in per-process memory. That makes multi-process writes and cross-process `watch_events_from()` unsafe as a foundation for this refactor. + +The revised plan therefore needs to solve four things explicitly: + +- control delivery without shared in-process primitives +- durable event ordering without multi-process writers +- server observation of worker state transitions +- clear priority and override rules when multiple control requests arrive + +## Key Decisions +- Hidden worker command + - Rename hidden `fabro __runner` to hidden `fabro __run-worker`. + - `__run-worker` executes a single run locally and does not call back into the server API. +- Single-writer rule + - The server is the only process that appends durable run events, updates run projections, and drives SSE. + - The worker must not open a write-capable `RunDatabase`. + - The worker may open the run store read-only for manifest and checkpoint reads only. +- Worker-to-server event path + - Worker stdout is reserved for newline-delimited canonical `RunEvent` JSON objects. + - The worker builds canonical events once, using the existing event model, and writes them to stdout. + - The server reads stdout, validates each event, appends it to the run store, and fans it out to SSE and any in-process listeners. + - This replaces the current assumption that the worker writes directly to the run store. +- Worker stderr and logs + - Worker stderr is not part of the event stream. + - The server captures worker stderr, writes it to a per-run log file under the run scratch directory, and mirrors lines into server tracing with the run id attached. +- Worker spawn contract + - The server spawns `fabro __run-worker --run-id --mode --storage-dir `. + - If the active config path is required to reproduce local runtime behavior, pass `--config ` as well. + - The worker does not need a server address because it does not talk HTTP to the server. +- Control delivery + - `cancel` sends `SIGTERM` to the worker process. + - After the grace timeout, the server sends `SIGKILL` to the worker process group. + - `pause` sends `SIGUSR1`. + - `unpause` sends `SIGUSR2`. + - Do not use `SIGSTOP` / `SIGCONT` for API pause and unpause. +- Control priority and conflict rules + - Priority is `cancel > pause > unpause`. + - `pending_control` remains a single value and later accepted requests overwrite lower-priority pending requests. + - `cancel` is accepted from `submitted`, `queued`, `starting`, `running`, or `paused`. + - A `cancel` request overwrites a pending `pause` or `unpause`. + - `pause` is accepted only when observed status is `running` and `pending_control` is `null`. + - `unpause` is accepted only when observed status is `paused` and `pending_control` is `null`. + - A lower-priority request while a higher-priority request is pending returns `409`. + - If `pause` is pending and `cancel` arrives before the worker reaches a safe point, the worker must skip `run.paused` and terminate through the normal cancel path. + - If the run is already paused and `cancel` arrives, the worker must exit the pause wait and cancel immediately. +- Signal handling in the worker + - `SIGTERM` requests cooperative cancellation. + - `SIGUSR1` requests cooperative pause. + - `SIGUSR2` requests cooperative unpause. + - Pause takes effect only at safe points such as between stages, before retry sleeps, before entering or resuming human waits, and at existing cancellation checkpoints. +- Event naming + - Use `run.cancel.requested`, `run.pause.requested`, and `run.unpause.requested` for accepted control requests. + - Use `run.paused` and `run.unpaused` for observed worker transitions. + - Keep terminal cancellation on `run.failed` with `reason=cancelled`. + - Avoid `run.resumed` because `resume` already means resume-from-checkpoint elsewhere in the system. +- Restart behavior in the first delivery + - Do not attempt PID-based worker reattachment in the first delivery. + - On graceful server shutdown, terminate active workers before exit. + - On server startup, any non-terminal run with stale worker metadata from a prior server process is marked interrupted or terminated and its worker metadata is cleared. + - Robust crash-time reattachment is deferred to a later phase and must include process identity verification before any signal delivery. +- API shape + - Keep the current control routes and verbs. + - Extend `RunStatusResponse` with `status_reason` and `pending_control`. + - Extend durable `StoreRunSummary` with `pending_control`. + - Control endpoints return the current observed status plus `pending_control`; they do not report the requested action as completed until the worker emits the corresponding effect event. +- Process titles + - Server titles: + - `fabro server boot` + - `fabro server unix:/path/to/socket` + - `fabro server tcp:127.0.0.1:3000` + - `fabro server stopping` + - Worker titles use the existing 12-character short run-id convention: + - `fabro start` + - `fabro resume` + - `fabro init` + - `fabro running` + - `fabro waiting` + - `fabro paused` + - `fabro cancelling` + - `fabro succeeded` + - `fabro failed` + - `fabro cancelled` + +## Delivery Plan +### Phase 1: Subprocess execution and signal cancel +- Spawn one worker subprocess per started run. +- Make the server the sole event-store writer. +- Stream canonical worker events over stdout into the server. +- Route worker stderr into per-run log files and server tracing. +- Implement signal-based `cancel` only. +- Do not support crash-time worker reattachment in this phase. + +### Phase 2: Control request events and API status enrichment +- Add `run.cancel.requested` and `pending_control`. +- Extend `RunStatusResponse` and `StoreRunSummary` with `status_reason` and `pending_control`. +- Update status projections so accepted cancel requests surface immediately without pretending the run has already terminated. + +### Phase 3: Pause and unpause +- Add `SIGUSR1` and `SIGUSR2` handling. +- Add `run.pause.requested`, `run.unpause.requested`, `run.paused`, and `run.unpaused`. +- Implement the priority and overwrite rules defined above. + +### Phase 4: Procline polish and optional crash-time recovery +- Add title helpers and short-id process titles. +- If crash-time worker recovery is still wanted, design it as a separate pass with explicit worker identity verification before any PID-based signaling. + +## Implementation Changes +### 1. Server supervision and worker pipes +- Replace the current `tokio::spawn(execute_run(...))` path in `lib/crates/fabro-server/src/server.rs` with worker subprocess spawning. +- Introduce a server-side supervisor record for live runs that stores: + - observed status + - created time + - local error text + - worker PID + - worker PGID + - worker mode (`start` or `resume`) + - current `pending_control` + - handles for worker stdout and stderr tasks +- On worker spawn: + - set process-group isolation + - capture stdout and stderr + - start one task that parses stdout into canonical `RunEvent`s and appends them to the run store + - start one task that drains stderr into a per-run log file and tracing + - set observed status to `starting` +- On worker exit: + - if the worker already produced a terminal run event, clear live worker metadata only + - if no terminal run event was appended, append a terminal failure with `reason=terminated` + +### 2. Workflow engine event sink refactor +- Refactor run execution so the worker path no longer depends on direct run-store writes from inside `fabro-workflow`. +- Introduce a run-event sink abstraction used by workflow execution, retro, pull-request creation, and any remaining bypass paths that currently append directly to the run store. +- For worker execution, the sink serializes canonical `RunEvent` JSON lines to stdout. +- Preserve the current event-strategy rule that the canonical `RunEvent` is built exactly once and reused for all downstream sinks. + +### 3. Worker execution and safe-point control +- Rewrite `lib/crates/fabro-cli/src/commands/run/runner.rs` into the `__run-worker` entrypoint that: + - loads the run manifest and checkpoint state read-only from storage + - executes `operations::start` or `operations::resume` + - owns the workflow `Emitter` + - translates OS signals into local cooperative control flags +- At safe points: + - if cancel is requested, terminate through the existing cancellation path + - if pause is requested and cancel is not pending, emit `run.paused`, block until unpause or cancel, then emit `run.unpaused` when execution continues +- Make cancel override a paused state immediately. + +### 4. Event model and projection updates +- Extend `fabro-workflow` event types and `fabro-types` `EventBody` with: + - `run.cancel.requested` + - `run.pause.requested` + - `run.unpause.requested` + - `run.paused` + - `run.unpaused` +- Keep request events server-originated and effect events worker-originated. +- Update `lib/crates/fabro-store/src/run_state.rs` so projections: + - track `pending_control` + - do not let request events overwrite observed status + - apply `run.paused` and `run.unpaused` to observed status + - continue projecting terminal cancellation as `status=failed` and `status_reason=cancelled` +- Extend durable `RunSummary` with `pending_control`. + +### 5. API and client updates +- Update `docs/api-reference/fabro-api.yaml` with: + - a new `RunControlAction` schema using `cancel`, `pause`, and `unpause` + - `pending_control` on `RunStatusResponse` + - `status_reason` on `RunStatusResponse` + - `pending_control` on `StoreRunSummary` +- Define control endpoint behavior as: + - validate whether the action is currently allowed + - apply the priority and overwrite rules above + - append the corresponding request event + - update `pending_control` + - send the signal if a live worker exists + - return the current observed status response +- Regenerate both generated API clients after the OpenAPI change. + +### 6. Process title cleanup +- Keep using `fabro_proc::title_init()` and `fabro_proc::title_set()`. +- Add helpers for server title updates by bind and lifecycle phase, and worker title updates by short run id and worker phase. +- Keep procline verification lightweight; do not build brittle exact-string end-to-end assertions around process titles. + +## Test Plan +- Single-writer and event-path tests + - worker execution path opens the run store read-only only + - worker stdout emits valid newline-delimited canonical `RunEvent` payloads + - server appends worker-streamed events in order and SSE reflects the appended stream + - worker stderr is captured into the per-run log file +- Cancel tests + - starting a queued run spawns a worker subprocess and records PID and PGID + - `POST /runs/{id}/cancel` appends `run.cancel.requested`, sets `pending_control=cancel`, sends `SIGTERM`, and later converges to durable `failed` with `status_reason=cancelled` + - cancelling a submitted or queued run reaches durable `failed/cancelled` without spawning a worker + - an unresponsive worker is escalated from worker `SIGTERM` to process-group `SIGKILL` +- Pause and unpause tests + - `POST /runs/{id}/pause` on a running worker appends `run.pause.requested`, sets `pending_control=pause`, and later projects `paused` + - `POST /runs/{id}/unpause` on a paused worker appends `run.unpause.requested`, sets `pending_control=unpause`, and later projects `running` + - pause followed by cancel before a safe point never produces `run.paused` + - cancel while paused exits the pause wait and converges to durable `failed/cancelled` + - lower-priority actions while a higher-priority action is pending return `409` and append no request event +- Startup behavior tests + - graceful server shutdown terminates active workers + - startup clears stale worker metadata and marks prior non-terminal runs interrupted or terminated +- Verification commands + - `cargo nextest run -p fabro-server` + - `cargo nextest run -p fabro-workflow` + - `cargo nextest run -p fabro-store` + - `cargo nextest run -p fabro-cli` + - `cargo fmt --check --all` + - `cargo clippy --workspace -- -D warnings` + +## Assumptions +- This refactor is Unix-first and may explicitly reject or defer non-Unix worker supervision behavior. +- The first delivery optimizes for correct subprocess supervision and durable event ordering, not crash-time worker survival across server restarts. +- `pause` and `unpause` are cooperative safe-point transitions, not immediate OS-level stop and continue. +- The short run id remains the first 12 characters of the ULID, matching current CLI presentation.