Skip to content

[Bug]: Workflow initial start write masks child exit stderr #1076

Description

@dennisonbertram

Work type

Bug / regression

Observed behavior

PR #1070 hosted test-race run 30660042116 failed
TestSourceManagerRunWorkflowIncludesBoundedStderrOnProcessExit. The workflow
child wrote child stderr diagnostic and exited with status 7, but the stored
workflow error was only write |1: broken pipe.

The failure occurred in internal/workflow while the #1070 harness, server,
TUI, and fast-check packages passed. #1070 does not change workflow code.

Expected behavior

If a workflow child exits before the parent can encode the initial start
message, the manager must still reap the child and resolve the primary outcome.
A non-zero child exit must win over the initial pipe-write EPIPE and must include
bounded child stderr. If the child exits successfully and the initial write is
the only failure, that write failure must remain visible. Timeout and semantic
protocol-error precedence must remain unchanged.

Reproduction

Hosted reproduction:

  1. Check out exact PR Fix terminal run publication atomicity #1070 head
    5b4eb8535b4762fd87f663148c1b4e5b9fe35e06.
  2. Run make test-race under GitHub's hosted runner.
  3. Observe run 30660042116, job 91253830642.
  4. internal/workflow/source_test.go:292 expected
    child stderr diagnostic but received write |1: broken pipe.

The scheduling-dependent path is visible in runSourceWorkflow: failure from
the first enc.Encode(start) returns immediately before stdin.Close,
cmd.Wait, and resolveSourceWorkflowOutcome.

The permanent red must make this ordering deterministic by holding the parent
between successful cmd.Start and the first protocol write until the child has
written stderr and exited.

User and operational impact

Real source-workflow failures can be reported as an unhelpful transport cleanup
symptom instead of the child process exit and its bounded diagnostic. This
obscures operator diagnosis and intermittently rejects the accepted CI baseline,
blocking the cron/callback reliability promotion chain. No data corruption or
credential exposure is known; stderr must remain bounded by the existing limit.

Suspected seam and search evidence

Owning seam: SourceManager.runSourceWorkflow in
internal/workflow/source.go, immediately after cmd.Start and during the
initial enc.Encode(protocolResponse{Type: "start"}).

Search evidence:

Current hypothesis: initial transport-write failure needs to enter the same
bounded process-reaping and outcome-resolution path as later teardown failures,
with child exit ahead of the transport symptom.

Blast-radius impact map

  • Callers and data flow: Go-authored source workflows launched by
    SourceManager; initial parent-to-child protocol write; process wait; stderr;
    terminal workflow error.
  • Config/env/defaults: none after search; existing workflow timeout remains the
    bound.
  • API/CLI/wire formats/tools: no schema change; user-visible workflow error
    becomes the primary child-exit diagnostic.
  • Persistence/schema/cache: no schema or migration; corrected error is stored
    through the existing run failure path.
  • Concurrency/lifecycle/cancellation/retries: affected; child exit can race the
    first parent write. Child must always be reaped, timeout honored, and process
    group cleaned without goroutine leaks.
  • Security/auth/permissions/privacy: preserve maxWorkflowStderrBytes; never
    expose unbounded stderr.
  • TUI/web/macOS/other clients: no client code change; clients display the more
    truthful stored error.
  • Provider/model/tool catalog: none after search.
  • Deployment/observability/runbooks: hosted race stability and clearer runtime
    diagnostics; no deployment migration.
  • Compatibility: successful workflows, standalone start-write failures,
    timeouts, semantic protocol errors, later stdin-close errors, and result
    handling must retain their contracts.
  • Existing tests/fixtures: source_test.go exit/stderr integration tests and
    source_outcome_internal_test.go precedence table require extension.
  • Documentation: issue-specific plan/impact map plus engineering,
    observational, system, and long-term logs and indexes.

Regression test first

Test file and test name:

  • Add a deterministic integration regression that pauses the parent after
    cmd.Start, allows a fixture child to write stderr and exit 7, then releases
    the initial start write.
  • Extend TestResolveSourceWorkflowOutcomePrecedence with initial-write error
    plus non-zero wait error and stderr, and with a standalone initial-write error
    control.

Red command:

go test ./internal/workflow -run 'Test.*InitialStartWrite.*ChildExit|TestResolveSourceWorkflowOutcomePrecedence' -count=1

Expected failure: pre-fix integration returns write |1: broken pipe and omits
the child stderr diagnostic because the process is not waited through the
resolver.

False-positive controls: prove the fixture reached child exit before releasing
the parent; assert exact non-zero exit precedence and bounded stderr; preserve a
successful-child/standalone-write failure; keep timeout, semantic protocol,
later close-error, and successful-result cases.

Fix boundaries

In scope: route initial start-write failure through bounded child cleanup/wait
and the existing outcome arbiter; add only the smallest deterministic lifecycle
test seam if required; extend the existing sourceWorkflowOutcome abstraction.

Out of scope: protocol redesign, sleeps/retries, ignoring all pipe errors,
changing workflow timeouts, changing stderr bounds, or modifying #1070's
terminal-publication code.

No parallel outcome abstraction is needed: repair runSourceWorkflow and
resolveSourceWorkflowOutcome so all launch outcomes share one precedence
contract.

Diagnostic and observability evidence

  • Hosted failure: Actions run 30660042116, job 91253830642.
  • Exact error: write |1: broken pipe does not contain
    child stderr diagnostic at internal/workflow/source_test.go:292.
  • Companion test-fast run 30660042065 passed, confirming a
    scheduling-sensitive race path.
  • Post-fix evidence must retain the exact child exit status and bounded stderr
    while eliminating false EPIPE-only results under focused normal/race stress.

Verification plan

  1. Deterministic red for child exit before the initial start write.
  2. Resolver table red/green for initial-write + wait-error precedence and the
    standalone initial-write control.
  3. Focused integration and outcome tests normal/race at -count=100.
  4. go test ./internal/workflow/... and
    go test -race ./internal/workflow/....
  5. make test-race.
  6. Unchanged foreground ./scripts/test-regression.sh with normal, full race,
    coverage >= 80%, and zero uncovered functions.
  7. Independent cheap-agent exact-head review.
  8. Hosted test-fast and test-race on the closing PR.
  9. Rebase Fix terminal run publication atomicity #1070 onto the repaired main and rerun its full local and hosted
    gates.

Rollout and rollback

Ship as a separate PR before rebasing #1070. No data migration or repair.
Monitor workflow failure messages and hosted race stability. Revert if semantic
protocol errors or timeouts lose precedence, successful workflows regress, a
child is not reaped, stderr becomes unbounded, or standalone transport errors
are hidden.

Documentation and handoff

Create an issue-specific plan and impact map. Update docs/plans/INDEX.md,
docs/logs/engineering-log.md, docs/logs/long-term-thinking-log.md,
docs/logs/observational-log.md, and docs/logs/system-log.md. Record exact red,
green, stress, full-regression, review, hosted, merge, and #1070 rebase evidence.

Definition of done

  • Deterministic child-exit-before-start-write reproduction is red pre-fix.
  • The child is always reaped through a bounded lifecycle path.
  • Non-zero exit and bounded stderr beat initial EPIPE.
  • Standalone initial-write errors remain visible.
  • Timeout, semantic protocol, close-error, and success contracts remain.
  • Focused normal/race stress and complete regression are green.
  • Independent cheap exact-head review has no unresolved findings.
  • Hosted test-fast and test-race are green on the closing PR.
  • Required plan, impact map, logs, indexes, rollout, and rollback are current.
  • Fix terminal run publication atomicity #1070 is rebased onto merged main and its gates rerun.

Process acknowledgement

  • I will preserve the failing regression test, update this issue if the root
    cause changes, and record any newly discovered bug as its own issue.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions