From fa2f7c8ed6e576006d14ec6ec989ac2d6f1a5e91 Mon Sep 17 00:00:00 2001 From: Dennison Date: Fri, 31 Jul 2026 22:24:57 +0200 Subject: [PATCH] fix(workflow): preserve child exit after initial write failure (#1076) --- docs/logs/engineering-log.md | 32 +++ docs/logs/long-term-thinking-log.md | 27 ++ docs/logs/observational-log.md | 22 ++ docs/logs/system-log.md | 26 ++ ...-workflow-initial-write-exit-impact-map.md | 110 ++++++++ ...e-1076-workflow-initial-write-exit-plan.md | 124 +++++++++ docs/plans/INDEX.md | 2 + docs/plans/active-plan.md | 9 + internal/workflow/source.go | 83 ++++-- .../workflow/source_outcome_internal_test.go | 263 +++++++++++++++--- 10 files changed, 632 insertions(+), 66 deletions(-) create mode 100644 docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-impact-map.md create mode 100644 docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-plan.md diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index d5611092..94c3c48f 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,37 @@ # Engineering Log +## 2026-07-31 (Workflow Initial Write Exit Arbitration — Issue #1076) + +- Symptom: hosted `test-race` run `30660042116` reported only + `write |1: broken pipe` for a source-workflow child that wrote + `child stderr diagnostic` and exited status 7. +- Cause: the first `enc.Encode(start)` error returns directly after killing the + process group, before stdin close, `cmd.Wait`, bounded stderr collection, and + `resolveSourceWorkflowOutcome`. +- TDD contract: hold the parent after `cmd.Start` until a FIFO plus OS-released + advisory lock prove the real child wrote stderr and exited; require child-exit + diagnostics and a reaped PID. Extend the pure resolver table for initial-write + precedence and its standalone-error control before production edits. +- First red: the exited-child fixture returned raw EPIPE instead of exit status + 7 plus stderr; the standalone resolver control returned missing-result. +- Review red: a live child closed stdin and remained active; cleanup killed and + reaped it, but the resulting `signal: killed` wait error incorrectly masked + the initial EPIPE. +- Fix: capture the initial-write error, retain process-group cleanup, + skip protocol serving, then enter the same close/wait/arbitration path used by + every other started-child outcome. Record when this path successfully requests + SIGKILL and classify that matching wait status as cleanup, while natural exit + status 7 remains primary with bounded stderr. +- Attribution boundary: a matching SIGKILL after this cleanup request cannot be + distinguished from an identical concurrent signal without broader WNOWAIT or + process-supervision machinery; EPIPE is intentionally primary in that narrow + ambiguous case. Natural exit statuses remain unambiguous. +- Green evidence: both lifecycle branches and the resolver plus real timeout + passed; focused normal/race x100 passed in 84.986s/90.588s; workflow + normal/race passed in 13.719s/16.534s; and `make test-race` passed. Full + non-PTY regression passed normal, full race, and coverage at 85.6% with zero + uncovered functions. Parent-run hosted gates remain. + ## 2026-07-31 (Runner Dispatcher Shutdown Isolation — Issue #1068) - Symptom: `go test -race ./internal/harness -count=5` failed four of five diff --git a/docs/logs/long-term-thinking-log.md b/docs/logs/long-term-thinking-log.md index c560cd4f..4abb7509 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -1,5 +1,32 @@ # Long-Term Thinking Log +## 2026-07-31 (Workflow Initial Write Exit Arbitration — Issue #1076) + +- Command intent: repair the separate hosted race failure where the initial + workflow `start` write returns EPIPE before the already-started child is + waited and its exit evidence is resolved. +- User intent: land one strict red-green baseline repair before rebasing #1070, + preserving actionable workflow diagnostics and a genuinely green race gate. +- Success definition: a deterministic real-child test proves exit-before-write, + exact reaping, and bounded stderr; resolver controls preserve deadline, + semantic protocol, process-exit, standalone write, close, and success order; + focused through full regression gates pass. +- Non-goals: #1070 terminal publication, #973 invalid-protocol/nil-map defects, + protocol redesign, retries, timeout changes, or unbounded stderr. +- Guardrails: one shared outcome arbiter, one wait per started child, process + group cleanup on write/protocol failure, strict TDD, and no push or merge. +- Review outcome: a second deterministic red proved that cleanup SIGKILL could + mask a standalone initial EPIPE while the child was still live. The outcome + now distinguishes a parent-requested SIGKILL from a natural child exit. +- Implementation outcome: initial write failure skips protocol serving but + still terminates the process group, closes stdin, waits exactly once, and + enters the shared arbiter. Exit 7 plus bounded stderr remains primary; a + cleanup-caused SIGKILL does not replace the earlier write error. +- Verification outcome: both lifecycle branches and resolver/stderr controls + pass normal/race x100, complete workflow normal/race passes, and + `make test-race` passes. The accepted non-PTY full regression passes normal, + full race, and coverage at 85.6% with zero uncovered functions. + ## 2026-07-31 (Runner Dispatcher Shutdown Isolation — Issue #1068) - Command intent: independently classify and repair the 4/5 aggregate race diff --git a/docs/logs/observational-log.md b/docs/logs/observational-log.md index 69617900..e751c84a 100644 --- a/docs/logs/observational-log.md +++ b/docs/logs/observational-log.md @@ -2,6 +2,28 @@ Use this file for observations about system behavior without immediately prescribing code changes. +## 2026-07-31 (Source-Workflow Initial Write Lifecycle) + +- Lifecycle observation: a successful `cmd.Start` transfers child ownership to + the parent even if the first protocol write fails; returning before + `cmd.Wait` loses both reaping and the primary exit evidence. +- Ordering observation: an initial EPIPE can be a consequence of the child + already exiting, so it cannot by itself classify the workflow failure. +- Testing observation: a FIFO establishes that the child reached its terminal + path, while an advisory lock released only when the process exits lets the + parent prove exit-before-write without sleeps or probabilistic scheduling. +- Scope observation: #1064 starts arbitration only after protocol serving; this + earlier lifecycle branch must feed that same resolver rather than create a + second outcome policy. +- Cleanup observation: a live child that already closed stdin turns the initial + write into EPIPE; terminating and reaping it then produces `signal: killed`. + That parent-requested cleanup status must not be presented as an independent + workflow failure. +- Attribution observation: `kill(-pgid, SIGKILL)` success records a cleanup + request, not exclusive signal provenance. After requesting the same signal, + EPIPE remains the truthful ordered error for a SIGKILL wait; natural exit 7 + remains distinguishable and primary. + ## 2026-07-31 (Runner Dispatcher Identity Under Parallel Load) - Aggregate observation: the original full-package race command reproduced the diff --git a/docs/logs/system-log.md b/docs/logs/system-log.md index 08dc4d65..8f6b6680 100644 --- a/docs/logs/system-log.md +++ b/docs/logs/system-log.md @@ -1,5 +1,31 @@ # System Log +## 2026-07-31 (Source-Workflow Initial Write Lifecycle) + +- System/component: `internal/workflow.SourceManager.runSourceWorkflow`, the + initial parent-to-child `start` write, process-group cleanup, and + `sourceWorkflowOutcome`. +- Ownership/order: every successfully started child remains parent-owned until + one close/wait path reaps it. Terminal resolution remains deadline, semantic + protocol, cleanup-attributed initial-write failure, natural non-zero process + exit with bounded stderr, standalone initial-write error, later close error, + then missing result or success. +- Inputs/outputs: add the already-observed initial-write error to the internal + outcome record; no API, CLI, protocol, persistence, config, or client schema + changes. +- Reliability/security boundary: write/protocol failures still terminate the + process group, wait occurs exactly once, and stderr remains limited to + `maxWorkflowStderrBytes`. +- Termination attribution: the runtime records whether initial-write cleanup + successfully requested process-group SIGKILL and whether `cmd.Wait` observed + that signal. That wait status is cleanup evidence rather than a new primary + failure; natural exit statuses and other signals retain process-failure + precedence. Exact concurrent same-signal provenance is outside this narrow + lifecycle contract. +- Rollback boundary: revert if timeout/protocol precedence, standalone + transport errors, process reaping, successful results, or stderr bounds + change. + ## 2026-07-31 (Runner Dispatcher Shutdown Identity) - System/component: bounded `internal/harness.Runner`, `poolDispatcher`, diff --git a/docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-impact-map.md b/docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-impact-map.md new file mode 100644 index 00000000..20ed8b6a --- /dev/null +++ b/docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-impact-map.md @@ -0,0 +1,110 @@ +# Cross-Surface Impact Map: Issue #1076 Workflow Initial Write Exit + +## Task + +- Task / issue: #1076, initial workflow `start` write masks child exit stderr. +- Plan: `2026-07-31-issue-1076-workflow-initial-write-exit-plan.md`. +- Owner: Codex. +- Status: implemented and fully verified locally; closing PR and hosted checks + pending parent promotion. + +## Current Ownership, Callers, and Data Flow + +- Entry point: `SourceManager.runSourceWorkflow` after `cmd.Start` and before + `serveProtocol`. +- Owning source of truth: `runSourceWorkflow` owns stdin/stdout pipes, bounded + stderr capture, process-group cleanup, close, wait, and the terminal signals + passed to `resolveSourceWorkflowOutcome`. +- Callers and consumers: source bundles register a `Script` with `Engine`; the + resulting error is stored on failed workflow runs and displayed unchanged by + API, CLI, TUI, web, and macOS consumers. +- Similar abstractions searched: `rg -n "bounded stderr|child stderr|broken + pipe|RunWorkflow|Stderr|sourceWorkflowOutcome|cmd.Wait|StdinPipe" + internal/workflow docs/plans docs/logs`. +- Duplication conclusion: extend the existing outcome record and resolver; do + not add another arbiter or client-specific error handling. + +## Config, API, CLI, and Tools + +- User-facing config/defaults/env/files: none after the source and issue search; + the existing workflow timeout remains the lifecycle bound. +- Endpoints, requests, responses, CLI commands, tools, and wire formats: no + schema or routing change. +- Error-state change: a non-zero child exit plus bounded stderr outranks the + initial transport-write symptom. A standalone initial-write error remains + visible. + +## Persistence and Compatibility + +- Schemas, migrations, caches, generated data, or ownership changes: none. +- Compatibility: successful results, missing-result errors, timeouts, semantic + protocol failures, later stdin-close errors, and stderr bounds remain. +- Mixed-version behavior: none; arbitration is local to one workflow process. + +## Lifecycle, Security, and Reliability + +- Concurrency/lifecycle: child exit races the first parent write. Every started + child must follow one bounded cleanup path and be waited exactly once. +- Cancellation/retries/cleanup: initial-write and protocol errors both retain + process-group termination; retries are not added. +- Authentication, authorization, permissions, secrets, and privacy: none after + data-flow search. Child stderr remains capped at `maxWorkflowStderrBytes`. +- Failure/recovery: deadline and semantic protocol remain first. A natural + non-zero process exit, including exit 7, retains bounded stderr and beats the + initial write. When the initial write itself requests process-group SIGKILL, + that cleanup wait signal does not mask the earlier write error. Later close, + missing result, and success contracts remain ordered afterward. + +## Product and Integration Surfaces + +- Server/runtime: corrected source-workflow failure provenance and child reaping. +- TUI/web/macOS/other clients: no code change; they receive the corrected stored + error through existing APIs. +- Provider/model/tool catalog and routing: none after repository search. +- External systems and automation: hosted race checks become stable for this + scheduling path. +- UX, keyboard, focus, accessibility, and motion: none; no client presentation + contract changes. + +## Deployment and Operations + +- Migration/order/flags: separate baseline PR lands before #1070 is rebased; + no flag or data migration. +- Observability: preserve child exit status and bounded stderr instead of raw + EPIPE-only diagnostics. +- Rollback: revert if timeout/protocol precedence changes, any child is not + reaped exactly once, successful workflows regress, or stderr becomes unbounded. +- Attribution boundary: SIGKILL observed after this path successfully requests + the same signal is cleanup evidence; distinguishing an indistinguishable + concurrent SIGKILL would require broader pre-reap process supervision outside + #1076. Natural exit statuses are not ambiguous and remain primary. +- Runbooks/operator docs: no operator procedure changes. + +## Regression Tests + +- First expected red: a real child writes stderr and exits 7 before the initial + parent write is released; pre-fix code returns raw EPIPE and skips wait. +- New acceptance controls: initial-write plus natural wait-error precedence, + standalone initial-write visibility, and live-child cleanup SIGKILL causality. +- Edge/failure/lifecycle/security: deadline, semantic protocol, close-only, + missing result, success, exact reaping, process-group cleanup, and bounded + stderr. +- Real-path proof: focused integration normal/race x100, complete workflow + normal/race, `make test-race`, unchanged full regression, then hosted checks. +- Exact targeted command: `go test ./internal/workflow -run + '^(TestSourceManagerRunWorkflowInitialStartWriteReapsChildExit|TestResolveSourceWorkflowOutcomePrecedence)$' + -count=1` and its race/stress variants. + +## Documentation and Handoff + +- Specs/public docs before code: this plan/map; no public docs because no new + user-facing capability is introduced. +- Implementation logs/indexes after code: engineering, observational, system, + long-term, plans index, and active-plan tracker. +- Training/onboarding/release notes: none; parent handoff records exact commit, + commands, results, and the required #1070 rebase ordering. + +## Warning Check + +- Every cross-surface heading is resolved. Unaffected surfaces include the + repository search and data-flow rationale rather than blank `None` claims. diff --git a/docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-plan.md b/docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-plan.md new file mode 100644 index 00000000..950a3d52 --- /dev/null +++ b/docs/plans/2026-07-31-issue-1076-workflow-initial-write-exit-plan.md @@ -0,0 +1,124 @@ +# Plan: Preserve Child Exit Across the Initial Workflow Write + +## Context + +- Governing GitHub issue: #1076. +- Problem: `SourceManager.runSourceWorkflow` returns immediately when encoding + the initial `start` message fails, so it skips `cmd.Wait` and the existing + terminal-outcome resolver. A child that already exited non-zero can therefore + be reported only as `broken pipe`, without its bounded stderr diagnostic. +- User impact: real source-workflow failures lose their actionable child exit + and stderr evidence, and the scheduling-dependent path intermittently rejects + the accepted hosted race baseline. +- Constraints: ship separately before rebasing #1070; preserve timeout and + semantic protocol-error precedence, standalone initial-write errors, stderr + bounds, successful results, exact-once wait/reap, and process-group cleanup. + +## Scope + +- In scope: one deterministic child-exit-before-initial-write integration + regression, outcome-resolver controls, and the smallest change that routes an + initial write failure through bounded cleanup, wait, and shared arbitration. +- Out of scope: protocol redesign, retries or sleeps, ignoring pipe failures, + timeout changes, stderr-bound changes, #973's invalid-protocol/nil-map defects, + and every #1070 terminal-publication file. + +## Documentation Contract + +- Feature status: implemented and fully verified locally; closing PR and hosted + checks pending parent promotion. +- Public docs affected: none; this corrects existing runtime failure provenance. +- Spec docs before code: this plan and its linked impact map. +- Implementation notes after code: engineering, observational, system, and + long-term logs plus the plans index and active-plan tracker. + +## Test Plan (TDD) + +- First integration red: hold execution after `cmd.Start`; a real child writes + stderr and exits status 7; an OS-released advisory lock proves exit before the + initial write proceeds. Require process-exit diagnostics, bounded stderr, and + a reaped PID instead of raw EPIPE. +- Resolver red: add `initialWriteErr` cases proving deadline and semantic + protocol errors remain earlier, non-zero wait plus bounded stderr beats the + write error, and a standalone write error remains visible. +- Cleanup-causality red: a live child closes stdin and remains alive; require + the initial EPIPE to remain primary when that failure triggers process-group + SIGKILL, while still reaping the child exactly once. +- Controls: later close-only errors, missing result, successful result, stderr + truncation, and the existing real-child timeout/protocol paths remain pinned. +- Focused stress: lifecycle integration and resolver normal/race at + `-count=100`. +- Package gates: complete `internal/workflow` normal and race. +- Repository gates: `make test-race`, then unchanged foreground + `./scripts/test-regression.sh` with Command Line Tools, `/private/tmp`, and an + isolated Go cache. +- Hosted gates: closing PR `test-fast` and `test-race`; then rebase #1070 and + rerun its complete accepted gates. + +## Cross-Surface Impact Map + +- See `2026-07-31-issue-1076-workflow-initial-write-exit-impact-map.md`. + +## Implementation Checklist + +- [x] Link contract-complete bug #1076 before implementation. +- [x] Record current ownership, callers, sources of truth, and search evidence. +- [x] Write this plan and the impact map before production code. +- [x] Add and capture the deterministic failing lifecycle regression. +- [x] Add and capture failing resolver controls. +- [x] Add and capture the live-child cleanup-causality regression found in review. +- [x] Route the initial write failure through cleanup, exact-once wait, and the + existing resolver. +- [x] Confirm focused stress, workflow package, CI-equivalent race, and full + repository regression gates. +- [x] Update logs and documentation status with current exact evidence. +- [x] Commit locally with #1076 linkage; do not push or merge. +- [ ] Parent opens one closing PR with `Closes #1076` and confirms hosted gates. +- [ ] Parent merges the baseline repair, rebases #1070, and reruns its gates. + +## Risks and Mitigations + +- Risk: cleanup-induced errors could hide timeout, semantic protocol, or true + process-exit evidence. +- Mitigation: pin the complete precedence table and retain one shared resolver. +- Risk: the lifecycle regression could still depend on scheduler timing. +- Mitigation: coordinate with a FIFO and an advisory lock released only by + process exit; do not use sleeps or probabilistic repetition for the red. +- Risk: fixing the early return could leak or double-wait a child. +- Mitigation: keep one linear close/wait path, assert the real PID is reaped, + and stress normal/race execution. +- Risk: after the initial EPIPE requests SIGKILL, that same wait signal cannot + prove whether this cleanup or an indistinguishable concurrent actor delivered + it. +- Mitigation: classify SIGKILL after a successful cleanup request as cleanup + evidence and preserve the earlier EPIPE; any natural exit status, including + exit 7, remains primary. Exact pre-kill signal provenance would require a + broader WNOWAIT/process-supervision redesign excluded by #1076. +- Risk: diagnostics could expose unbounded child output. +- Mitigation: retain `limitedWriter` and `boundedString` and keep the existing + truncation control green. + +## Verification Evidence + +- First semantic red: the deterministic exited-child test returned + `write |1: broken pipe` instead of exit status 7 plus bounded stderr; the + standalone resolver control returned `exited without a result`. +- Review red: the live-child test returned + `workflow "initial-write-live-child" exited: signal: killed` instead of the + initial EPIPE; its resolver control failed the same precedence contract. +- Focused green: both real lifecycle branches, the full resolver table, and the + real CommandContext timeout passed together. Both child PID assertions proved + the process was reaped. +- Focused stress: both lifecycle branches plus resolver/stderr controls passed + normal `-count=100` in 84.986s and race `-count=100` in 90.588s. +- Package: complete `internal/workflow/...` passed normal in 13.719s and race + in 16.534s. +- CI-equivalent race: `make test-race` passed every configured package; + `internal/workflow` passed in 23.720s. +- Full repository regression: the unchanged foreground non-PTY + `./scripts/test-regression.sh` passed normal, full race, and coverage; + `coveragegate: PASS (total=85.6%, min=80.0%, zero-functions=0)`. +- Launch evidence: tmux/redirect attempts were excluded because macOS + `security` opened the controlling terminal. The accepted run used the managed + non-PTY foreground process with Command Line Tools, `/private/tmp`, and the + isolated Go cache. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index d148a830..dc294af5 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -2,6 +2,8 @@ - `2026-07-31-issue-1077-d0-logo-plan.md` — Issue #1077 D0 macOS app logo geometry, shared SwiftUI source, and verification status. - `2026-07-31-issue-1077-d0-logo-impact-map.md` — Cross-surface impact map for Issue #1077. +- `2026-07-31-issue-1076-workflow-initial-write-exit-plan.md` — Issue #1076 plan for preserving child exit diagnostics across the initial workflow write. +- `2026-07-31-issue-1076-workflow-initial-write-exit-impact-map.md` — Cross-surface impact map for Issue #1076 lifecycle and error arbitration. - `2026-07-31-issue-1068-dispatcher-shutdown-isolation-plan.md` — Issue #1068 plan for deterministic instance-scoped Runner dispatcher shutdown verification. - `2026-07-31-issue-1068-dispatcher-shutdown-isolation-impact-map.md` — Cross-surface impact map for Issue #1068 lifecycle isolation. - `2026-07-31-issue-1062-provider-key-matrix-health-wait-plan.md` — Issue #1062 plan for a contention-tolerant provider API-key matrix startup wait. diff --git a/docs/plans/active-plan.md b/docs/plans/active-plan.md index a6f92245..5a814acc 100644 --- a/docs/plans/active-plan.md +++ b/docs/plans/active-plan.md @@ -1,5 +1,13 @@ # Active Plan +Current status: Issue #1076 isolates the source-workflow lifecycle race where a +child exits before the initial `start` write and the early EPIPE return skips +wait plus bounded stderr arbitration. Natural-exit and live-child cleanup reds +are green, focused normal/race x100, workflow normal/race, and `make test-race` +pass. The unchanged full regression also passes at 85.6% coverage with zero +uncovered functions, and the repair is committed locally. Parent promotion +remains; this ships separately before #1070 is rebased. + Current status: Issue #1077 D0 macOS app logo is implemented on current `origin/main`; focused and complete Swift tests plus the GoCode build pass. The repository regression gate remains blocked by two unrelated real-keychain tests @@ -30,6 +38,7 @@ Remaining work before merge is final verification and any requested review/cleanup. Current active plans: +- `2026-07-31-issue-1076-workflow-initial-write-exit-plan.md` - `2026-07-31-issue-1068-dispatcher-shutdown-isolation-plan.md` - `2026-07-30-issue-1052-provider-key-capture-sync-plan.md` - `2026-07-30-issue-1023-feedback-intake-plan.md` diff --git a/internal/workflow/source.go b/internal/workflow/source.go index 5775e3e6..898cbb3c 100644 --- a/internal/workflow/source.go +++ b/internal/workflow/source.go @@ -464,6 +464,13 @@ func (m *SourceManager) build(ctx context.Context, bundle *SourceBundle) error { } func (m *SourceManager) runSourceWorkflow(ctx *Context, bundle *SourceBundle) (any, error) { + return m.runSourceWorkflowWithBeforeInitialWrite(ctx, bundle, nil) +} + +// runSourceWorkflowWithBeforeInitialWrite exposes only the lifecycle boundary +// needed to make child-exit-before-write ordering deterministic in package +// tests. Production calls pass nil and retain the exact run path. +func (m *SourceManager) runSourceWorkflowWithBeforeInitialWrite(ctx *Context, bundle *SourceBundle, beforeInitialWrite func(*exec.Cmd)) (any, error) { if err := m.build(ctx.ctx, bundle); err != nil { return nil, err } @@ -490,18 +497,26 @@ func (m *SourceManager) runSourceWorkflow(ctx *Context, bundle *SourceBundle) (a if err := cmd.Start(); err != nil { return nil, err } + if beforeInitialWrite != nil { + beforeInitialWrite(cmd) + } enc := json.NewEncoder(stdin) - if err := enc.Encode(protocolResponse{Type: "start", Result: mustRaw(ctx.Args)}); err != nil { - _ = killProcessGroup(cmd) - return nil, err + initialWriteErr := enc.Encode(protocolResponse{Type: "start", Result: mustRaw(ctx.Args)}) + initialWriteKillRequested := false + var result any + var protocolErr error + if initialWriteErr != nil { + initialWriteKillRequested = killProcessGroup(cmd) == nil + } else { + result, protocolErr = m.serveProtocol(runCtx, ctx, stdout, enc) + if protocolErr != nil { + _ = killProcessGroup(cmd) + } } - result, protocolErr := m.serveProtocol(runCtx, ctx, stdout, enc) - if protocolErr != nil { - _ = killProcessGroup(cmd) - } closeErr := stdin.Close() waitErr := cmd.Wait() + initialWriteKillCausedWait := initialWriteKillRequested && processWaitWasKilled(waitErr) deadlineExceeded := runCtx.Err() == context.DeadlineExceeded if deadlineExceeded { _ = killProcessGroup(cmd) @@ -510,30 +525,35 @@ func (m *SourceManager) runSourceWorkflow(ctx *Context, bundle *SourceBundle) (a _ = killProcessGroup(cmd) } return resolveSourceWorkflowOutcome(sourceWorkflowOutcome{ - result: result, - workflowName: bundle.Manifest.Name, - timeout: timeout, - deadlineExceeded: deadlineExceeded, - protocolErr: protocolErr, - closeErr: closeErr, - waitErr: waitErr, - stderr: stderr.String(), + result: result, + workflowName: bundle.Manifest.Name, + timeout: timeout, + deadlineExceeded: deadlineExceeded, + protocolErr: protocolErr, + initialWriteErr: initialWriteErr, + initialWriteKillCausedWait: initialWriteKillCausedWait, + closeErr: closeErr, + waitErr: waitErr, + stderr: stderr.String(), }) } type sourceWorkflowOutcome struct { - result any - workflowName string - timeout time.Duration - deadlineExceeded bool - protocolErr error - closeErr error - waitErr error - stderr string + result any + workflowName string + timeout time.Duration + deadlineExceeded bool + protocolErr error + initialWriteErr error + initialWriteKillCausedWait bool + closeErr error + waitErr error + stderr string } // resolveSourceWorkflowOutcome keeps primary execution failures ahead of -// process-cleanup failures while preserving a standalone cleanup error. +// transport and cleanup failures, without reporting a parent-requested kill as +// if it were an independent child failure. func resolveSourceWorkflowOutcome(outcome sourceWorkflowOutcome) (any, error) { if outcome.deadlineExceeded { return nil, fmt.Errorf("workflow %q timed out after %s", outcome.workflowName, outcome.timeout) @@ -541,9 +561,15 @@ func resolveSourceWorkflowOutcome(outcome sourceWorkflowOutcome) (any, error) { if outcome.protocolErr != nil { return nil, outcome.protocolErr } + if outcome.initialWriteErr != nil && outcome.initialWriteKillCausedWait { + return nil, outcome.initialWriteErr + } if outcome.waitErr != nil { return nil, fmt.Errorf("workflow %q exited: %w: %s", outcome.workflowName, outcome.waitErr, boundedString(outcome.stderr, maxWorkflowStderrBytes)) } + if outcome.initialWriteErr != nil { + return nil, outcome.initialWriteErr + } if outcome.closeErr != nil { return nil, outcome.closeErr } @@ -553,6 +579,15 @@ func resolveSourceWorkflowOutcome(outcome sourceWorkflowOutcome) (any, error) { return outcome.result, nil } +func processWaitWasKilled(err error) bool { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ProcessState == nil { + return false + } + status, ok := exitErr.ProcessState.Sys().(syscall.WaitStatus) + return ok && status.Signaled() && status.Signal() == syscall.SIGKILL +} + type protocolMessage struct { ID string `json:"id,omitempty"` Type string `json:"type"` diff --git a/internal/workflow/source_outcome_internal_test.go b/internal/workflow/source_outcome_internal_test.go index 1a6f84ff..4ead6aa2 100644 --- a/internal/workflow/source_outcome_internal_test.go +++ b/internal/workflow/source_outcome_internal_test.go @@ -1,7 +1,13 @@ package workflow import ( + "context" "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" "strings" "syscall" "testing" @@ -15,44 +21,65 @@ func TestResolveSourceWorkflowOutcomePrecedence(t *testing.T) { waitErr := errors.New("exit status 7") protocolErr := errors.New("malformed protocol") + initialWriteErr := errors.New("initial start write failed") result := map[string]any{"ok": true} tests := []struct { - name string - result any - deadlineExceeded bool - protocolErr error - closeErr error - waitErr error - wantResult any - wantError string - wantErrorIs error + name string + result any + deadlineExceeded bool + protocolErr error + initialWriteErr error + initialWriteKillCausedWait bool + closeErr error + waitErr error + wantResult any + wantError string + wantErrorIs error }{ { - name: "deadline precedes protocol process and close errors", - result: result, - deadlineExceeded: true, - protocolErr: protocolErr, - closeErr: syscall.EPIPE, - waitErr: waitErr, - wantError: `workflow "test-workflow" timed out after 3s`, + name: "deadline precedes protocol process transport and cleanup errors", + result: result, + deadlineExceeded: true, + protocolErr: protocolErr, + initialWriteErr: initialWriteErr, + initialWriteKillCausedWait: true, + closeErr: syscall.EPIPE, + waitErr: waitErr, + wantError: `workflow "test-workflow" timed out after 3s`, }, { - name: "protocol precedes process and close errors", - result: result, - protocolErr: protocolErr, - closeErr: syscall.EPIPE, - waitErr: waitErr, - wantError: protocolErr.Error(), - wantErrorIs: protocolErr, + name: "initial start write precedes its cleanup kill", + initialWriteErr: initialWriteErr, + initialWriteKillCausedWait: true, + waitErr: errors.New("signal: killed"), + wantError: initialWriteErr.Error(), + wantErrorIs: initialWriteErr, }, { - name: "process exit precedes stdin close and includes stderr", - result: result, - closeErr: syscall.EPIPE, - waitErr: waitErr, - wantError: `workflow "test-workflow" exited: exit status 7: child stderr diagnostic`, - wantErrorIs: waitErr, + name: "protocol precedes process transport and cleanup errors", + result: result, + protocolErr: protocolErr, + initialWriteErr: initialWriteErr, + closeErr: syscall.EPIPE, + waitErr: waitErr, + wantError: protocolErr.Error(), + wantErrorIs: protocolErr, + }, + { + name: "process exit precedes initial write and stdin close and includes stderr", + result: result, + initialWriteErr: initialWriteErr, + closeErr: syscall.EPIPE, + waitErr: waitErr, + wantError: `workflow "test-workflow" exited: exit status 7: child stderr diagnostic`, + wantErrorIs: waitErr, + }, + { + name: "clean child still surfaces initial start write error", + initialWriteErr: initialWriteErr, + wantError: initialWriteErr.Error(), + wantErrorIs: initialWriteErr, }, { name: "successful child still surfaces stdin close error", @@ -75,14 +102,16 @@ func TestResolveSourceWorkflowOutcomePrecedence(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := resolveSourceWorkflowOutcome(sourceWorkflowOutcome{ - result: tt.result, - workflowName: "test-workflow", - timeout: 3 * time.Second, - deadlineExceeded: tt.deadlineExceeded, - protocolErr: tt.protocolErr, - closeErr: tt.closeErr, - waitErr: tt.waitErr, - stderr: "child stderr diagnostic", + result: tt.result, + workflowName: "test-workflow", + timeout: 3 * time.Second, + deadlineExceeded: tt.deadlineExceeded, + protocolErr: tt.protocolErr, + initialWriteErr: tt.initialWriteErr, + initialWriteKillCausedWait: tt.initialWriteKillCausedWait, + closeErr: tt.closeErr, + waitErr: tt.waitErr, + stderr: "child stderr diagnostic", }) require.Equal(t, tt.wantResult, got) if tt.wantError == "" { @@ -102,14 +131,164 @@ func TestResolveSourceWorkflowOutcomeBoundsProcessStderr(t *testing.T) { stderr := strings.Repeat("x", maxWorkflowStderrBytes) + "secret-tail" _, err := resolveSourceWorkflowOutcome(sourceWorkflowOutcome{ - workflowName: "test-workflow", - timeout: time.Second, - closeErr: syscall.EPIPE, - waitErr: errors.New("exit status 7"), - stderr: stderr, + workflowName: "test-workflow", + timeout: time.Second, + initialWriteErr: errors.New("initial start write failed"), + closeErr: syscall.EPIPE, + waitErr: errors.New("exit status 7"), + stderr: stderr, }) require.Error(t, err) require.Contains(t, err.Error(), "...[truncated]") require.NotContains(t, err.Error(), "secret-tail") require.LessOrEqual(t, len(err.Error()), maxWorkflowStderrBytes+128) } + +func TestSourceManagerRunWorkflowInitialStartWriteReapsChildExit(t *testing.T) { + root := t.TempDir() + readyPath := filepath.Join(root, "child-ready.fifo") + lockPath := filepath.Join(root, "child-exit.lock") + require.NoError(t, syscall.Mkfifo(readyPath, 0o600)) + + manager, err := NewSourceManager(SourceManagerOptions{ + Engine: NewEngine(EngineOptions{}), + WorkflowDirs: []string{filepath.Join(root, "global"), filepath.Join(root, "workspace")}, + CacheDir: filepath.Join(root, "cache"), + ModuleRoot: findModuleRoot(), + }) + require.NoError(t, err) + + source := fmt.Sprintf(`package main + +import ( + "fmt" + "os" + "syscall" +) + +func main() { + lock, err := os.OpenFile(%q, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + panic(err) + } + if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_EX); err != nil { + panic(err) + } + fmt.Fprint(os.Stderr, "child stderr diagnostic") + ready, err := os.OpenFile(%q, os.O_WRONLY, 0) + if err != nil { + panic(err) + } + if _, err := ready.Write([]byte{1}); err != nil { + panic(err) + } + if err := ready.Close(); err != nil { + panic(err) + } + os.Exit(7) +} +`, lockPath, readyPath) + bundle, err := manager.CreateWorkflow(context.Background(), CreateWorkflowRequest{ + Name: "initial-write-exit", + Description: "Exits before the initial start write.", + Source: source, + Scope: "workspace", + }) + require.NoError(t, err) + + childPID := 0 + result, err := manager.runSourceWorkflowWithBeforeInitialWrite( + &Context{ctx: context.Background(), Args: map[string]any{"input": "value"}}, + bundle, + func(cmd *exec.Cmd) { + childPID = cmd.Process.Pid + ready, openErr := os.OpenFile(readyPath, os.O_RDONLY, 0) + require.NoError(t, openErr) + defer ready.Close() + var signal [1]byte + _, readErr := io.ReadFull(ready, signal[:]) + require.NoError(t, readErr) + + lock, lockErr := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) + require.NoError(t, lockErr) + defer lock.Close() + require.NoError(t, syscall.Flock(int(lock.Fd()), syscall.LOCK_EX)) + defer syscall.Flock(int(lock.Fd()), syscall.LOCK_UN) //nolint:errcheck + }, + ) + require.Nil(t, result) + require.ErrorContains(t, err, `workflow "initial-write-exit" exited: exit status 7`) + require.ErrorContains(t, err, "child stderr diagnostic") + require.NotErrorIs(t, err, syscall.EPIPE) + require.Positive(t, childPID) + require.ErrorIs(t, syscall.Kill(childPID, 0), syscall.ESRCH, "child process must be reaped") +} + +func TestSourceManagerRunWorkflowInitialStartWriteCleansLiveChildAndPreservesWriteError(t *testing.T) { + root := t.TempDir() + readyPath := filepath.Join(root, "child-ready.fifo") + require.NoError(t, syscall.Mkfifo(readyPath, 0o600)) + + manager, err := NewSourceManager(SourceManagerOptions{ + Engine: NewEngine(EngineOptions{}), + WorkflowDirs: []string{filepath.Join(root, "global"), filepath.Join(root, "workspace")}, + CacheDir: filepath.Join(root, "cache"), + ModuleRoot: findModuleRoot(), + }) + require.NoError(t, err) + + source := fmt.Sprintf(`package main + +import ( + "os" + "syscall" + "time" +) + +func main() { + if err := syscall.Close(0); err != nil { + panic(err) + } + ready, err := os.OpenFile(%q, os.O_WRONLY, 0) + if err != nil { + panic(err) + } + if _, err := ready.Write([]byte{1}); err != nil { + panic(err) + } + if err := ready.Close(); err != nil { + panic(err) + } + for { + time.Sleep(time.Hour) + } +} +`, readyPath) + bundle, err := manager.CreateWorkflow(context.Background(), CreateWorkflowRequest{ + Name: "initial-write-live-child", + Description: "Closes stdin but remains live.", + Source: source, + Scope: "workspace", + }) + require.NoError(t, err) + + childPID := 0 + result, err := manager.runSourceWorkflowWithBeforeInitialWrite( + &Context{ctx: context.Background(), Args: map[string]any{"input": "value"}}, + bundle, + func(cmd *exec.Cmd) { + childPID = cmd.Process.Pid + ready, openErr := os.OpenFile(readyPath, os.O_RDONLY, 0) + require.NoError(t, openErr) + defer ready.Close() + var signal [1]byte + _, readErr := io.ReadFull(ready, signal[:]) + require.NoError(t, readErr) + }, + ) + require.Nil(t, result) + require.ErrorIs(t, err, syscall.EPIPE) + require.NotContains(t, err.Error(), "signal: killed") + require.Positive(t, childPID) + require.ErrorIs(t, syscall.Kill(childPID, 0), syscall.ESRCH, "child process must be reaped") +}