Skip to content

fix(process): release completed command output under Bun - #139965

Merged
steipete merged 2 commits into
mainfrom
codex/bun-owned-stdio-output
Sep 6, 2026
Merged

fix(process): release completed command output under Bun#139965
steipete merged 2 commits into
mainfrom
codex/bun-owned-stdio-output

Conversation

@steipete

@steipete steipete commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where service-managed commands finish under Bun but their result remains pending until retained descendants exit.

Why This Change Was Made

Give Bun's private anchor ownership of its output streams and release the relay's inherited duplicates after successful spawn. Preserve Node's original standard-stream behavior, which its Linux process census requires. Forward stdin EOF explicitly because Bun's default pipe end check initializes extra output writers.

User Impact

Completed command output reaches real EOF while descendant cleanup remains available. Root-result delivery, decoder flushing, lineage authority, input error handling, backpressure, and subsequent cleanup retain their existing contracts.

Evidence

  • The original two retained-descendant cases fail on the Bun baseline and pass after repair.
  • All 148 selected process tests pass on Linux Node 24.19.0, Linux Bun 1.4.2, macOS Node 26.8.1, and macOS Bun 1.4.2.
  • The first revision failed Linux CI: closing Node's standard descriptors caused its later native process census to abort before sending a closing receipt. A minimal native probe and the unchanged owned-stdio test reproduced that defect. The correction preserves Node's standard-stream semantics and limits explicit descriptor ownership to Bun.
  • Fresh complete changed-file gate, full repository build, and independent P0–P2 review pass. No tracked tests, assertions, timeout limits, or host cleanup gates changed.
  • Linux proof: Node Testbox, Bun Testbox. Both command wrappers exited successfully against the verified final candidate tree; both leases were stopped afterward.
  • Fresh exact-head CI passed after the Linux correction. This follows fix(process): start supervised commands under Bun #139555 and does not claim the entire repository's Bun suite is green.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added size: XS maintainer Maintainer-authored PR labels Sep 6, 2026
@steipete
steipete marked this pull request as ready for review September 6, 2026 08:55
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 5:50 AM ET / 09:50 UTC.

ClawSweeper review

What this changes

The PR releases completed command output under Bun while preserving Node standard streams and explicitly forwarding stdin EOF.

Merge readiness

✅ Ready for maintainer review

This remains a useful fix absent from inspected current main. The corrected Bun-only descriptor handling addresses the previous Linux Node concern; no concrete blocking defect remains in the introduced patch.

Priority: P2
Reviewed head: 5ed0bc0c12e8fc4e3111f6ec0be5b5a00e41e4b1

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused owner-level repair with relevant existing regression coverage and a correction that resolves the prior Node descriptor concern.
Proof confidence 🌊 off-meta tidepool Not applicable: The supplied MEMBER association exempts this PR from the external-contributor proof gate, and no material authority change triggers an exception. The body reports production subprocess lifecycle validation on Node and Bun, but linked Testbox logs were inaccessible to this reviewer.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The supplied MEMBER association exempts this PR from the external-contributor proof gate, and no material authority change triggers an exception. The body reports production subprocess lifecycle validation on Node and Bun, but linked Testbox logs were inaccessible to this reviewer.
Evidence reviewed 8 items Introduced change and corrected runtime split: The complete host-pinned introduction contains only the anchor and relay changes. At the reviewed head, explicit fd 1/2 ownership is conditional on Bun; Node retains process.stdout/process.stderr. The relay releases duplicate writers only after anchor spawn.
Current main still needs the repair: Inspected current-main source still pipelines command output into global standard streams and uses default stdin pipe ending. It does not contain the proposed Bun output-lifetime repair.
Result completion remains separate from cleanup: The host waits for the root result and both output streams to end. Cleanup independently requires its closing receipt and observed process-group disappearance; the introduced patch does not weaken these gates.
Findings None None.
Security None None.

How this fits together

OpenClaw's process supervisor launches service-managed commands through a relay and a process-group anchor. Command results require completed output, while a separate control channel retains responsibility for cleaning up descendants.

flowchart LR
  A[Command request] --> B[Supervisor host]
  B --> C[Relay process]
  C --> D[Process-group anchor]
  D --> E[Command and descendants]
  E --> F[Output streams]
  F --> B
  D --> G[Cleanup control channel]
  G --> B
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source +25. Total +25 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 2 36 11 +25
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 36 11 +25

Review metrics

None.

Technical review

Best possible solution:

Keep output-descriptor ownership runtime-specific while preserving real output EOF, decoder flushing, and independent descendant cleanup.

Do we have a high-confidence way to reproduce the issue?

Yes: the existing retained-descendant and incomplete UTF-8 lifecycle cases provide concrete reproduction paths, supported by the supplied baseline failure report; this read-only review did not execute them.

Is this the best way to solve the issue?

Yes: the patch repairs stream ownership at the relay and anchor, preserves Node's established behavior, and leaves host completion and cleanup checks intact.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 2dbbb33887ae.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove merge-risk: 🚨 availability: Current PR review selected no merge-risk labels.

Label justifications:

  • P2: This repairs a bounded Bun command-completion failure without evidence of a broader current runtime outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The supplied MEMBER association exempts this PR from the external-contributor proof gate, and no material authority change triggers an exception. The body reports production subprocess lifecycle validation on Node and Bun, but linked Testbox logs were inaccessible to this reviewer.

Evidence

What I checked:

  • Introduced change and corrected runtime split: The complete host-pinned introduction contains only the anchor and relay changes. At the reviewed head, explicit fd 1/2 ownership is conditional on Bun; Node retains process.stdout/process.stderr. The relay releases duplicate writers only after anchor spawn. (src/process/supervisor/service-child-group-anchor.ts:314, 5ed0bc0c12e8)
  • Current main still needs the repair: Inspected current-main source still pipelines command output into global standard streams and uses default stdin pipe ending. It does not contain the proposed Bun output-lifetime repair. (src/process/supervisor/service-child-group-anchor.ts:312, 2dbbb33887ae)
  • Result completion remains separate from cleanup: The host waits for the root result and both output streams to end. Cleanup independently requires its closing receipt and observed process-group disappearance; the introduced patch does not weaken these gates. (src/process/supervisor/service-child-relay-host.ts:239, 5ed0bc0c12e8)
  • Existing regression scenarios: The retained-descendant test requires a completed result while the descendant is alive and extinction remains pending. The incomplete UTF-8 case additionally verifies decoded and raw output before result delivery. Both exercise real subprocesses through production adapters; neither test is changed by this PR. (src/process/supervisor/adapters/child.service-lifecycle.test.ts:282, 5ed0bc0c12e8)
  • Previous review concern addressed: The supplied previous review requested investigation of Linux descriptor-lifetime failures. The captured PR body reports reproducing Node's native census abort, restoring Node standard-stream semantics, and passing 148 selected tests across Linux/macOS with Node and Bun. The inspected correction matches that explanation. Captured context sourceRevision: bb0898406061282964ccd7a9cbc0efc42d7f70fe8cde1ad72667fa639210f313. (5ed0bc0c12e8)
  • Proof inspection limit: The body links Linux validation at https://github.com/openclaw/openclaw/actions/runs/34024136404 and https://github.com/openclaw/openclaw/actions/runs/34024764081. GitHub API access failed and browser retrieval returned cache misses, so their logs were not independently inspected. This is a reviewer access limitation, not missing contributor evidence.

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-06T09:11:12.672Z sha 2406418 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-06T09:21:13.530Z sha 2406418 :: blocked before merge. :: none

@steipete
steipete marked this pull request as draft September 6, 2026 09:15
@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Sep 6, 2026
@steipete
steipete marked this pull request as ready for review September 6, 2026 09:37
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Sep 6, 2026
@steipete
steipete merged commit 825eba5 into main Sep 6, 2026
221 of 222 checks passed
@steipete
steipete deleted the codex/bun-owned-stdio-output branch September 6, 2026 10:10
@steipete

steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 7, 2026
@steipete
fix(process): release completed command output under Bun (openclaw#13…
49e4421 
…9965)

* fix(process): release completed command output under Bun

* fix(process): preserve Node standard descriptors during cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete