Skip to content

fix(test): preserve unlimited test children after stale deadlines - #44613

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/unlimited-test-child-lifetime
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/unlimited-test-child-lifetime

Conversation

@steipete

@steipete steipete commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

A completed test can leave a file-level deadline armed. If it fires while a later test has timeout: 0, the process reaper compares that test’s EPOCH timestamp with the current clock and kills its live child, even though the test has no deadline.

Exclude the no-deadline sentinel before checking expiration, matching ExecutionEntry::evaluate_timeout. The regression keeps a child alive across a completed finite deadline and checks its OS liveness from an unlimited test. Normal timeout cleanup remains unchanged.

Official Bun 13a98b0d reproduces the failure three out of three times with ESRCH. The same source bug reproduces on native Windows x64. The shared owner fix and regression pass native Linux, macOS, and Windows qualification in openclaw#133; the fork and upstream have identical baseline blobs for both changed files. The upstream-specific release build at 104a1c67d3 passes: the maintained regression passes in 198 ms and the full test-runner file passes 25 tests in 910 ms (16 existing skips). P2 Codex autoreview is scoped-clean. No fork-specific test policy or source-position expectation changes are included.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 2ee9d6d7-f227-42e2-a723-c0b74e022410
📥 Commits

Reviewing files that changed from the base of the PR and between 13a98b0 and 104a1c6.

📒 Files selected for processing (2)
  • src/runtime/test_runner/Execution.rs
  • test/js/bun/test/test-test.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

The timeout check now skips process-killing checks when an active entry has an epoch deadline. A nested-runner regression test checks that a child process remains alive during a zero-timeout test.

Changes

Timeout handling

Layer / File(s) Summary
Timeout check and regression test
src/runtime/test_runner/Execution.rs, test/js/bun/test/test-test.test.ts
The timeout check excludes epoch deadlines. The nested-runner test verifies that a child process remains alive after a finite-deadline test and during a zero-timeout test.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 104a1

No actionable merge-blocking risk is identified; this change is ready for normal checks.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserving child processes for tests with unlimited timeouts when a stale deadline expires.
Description check ✅ Passed The description explains the problem, the fix, the regression test, and verification results. It covers both template topics, though it does not use the required section headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

steipete added a commit to openclaw/bun that referenced this pull request Oct 6, 2026
…133)

The test runner can kill an unlimited test’s live child when a completed test’s stale deadline fires. Its process-reaping check treats the no-deadline/EPOCH timestamp as expired. Exclude that sentinel, matching the existing entry timeout check, and cover the completed-deadline/live-child sequence with a deterministic regression.

The deferred compile-cache idle test now preserves its ten-second persistence-progress guard and bounds module loading, pipe flushes, child exit, and cleanup individually. The complete 2,000-module workload, late-module handshake, 2,002-file assertion, idle-generation window, and signal-exit budget remain unchanged.

The exact Windows binary from [run 37386790583](https://github.com/openclaw/bun/actions/runs/37386790583) reproduces the old 30-second aggregate timeout 3/3 under a controlled CPU quota while files keep appearing. An independent driver completes all 2,002 entries in 64.7 seconds; the proposed test passes under that quota in 52.6/52.2 seconds. Deliberately stalled persistence, startup, and exit still fail at their ten-second guards. That hosted failure retained no progress timeline, so its specific runner-level trigger remains unknown. This is distinct from #53’s Linux idle-accounting fix and #130’s Windows execute-access scan fix.

The child-lifetime regression fails 3/3 on old Mac and Windows x64 fork binaries and official upstream Bun `13a98b0d`. Exact-source Windows pairs fail 3/3 before and pass 3/3 with the owner fix. Two stale hook-error column expectations were corrected from 15 to 11 after the pre-change fork reproduced all five failures and Node 24 confirmed column 11; exact source-position and error-attribution checks remain intact.

Final validation applies to `29cbbbf595` on main `10b749212a`:

- P2 Codex autoreview and the final pre-merge review are scoped-clean.
- Local release build passes, with 25 test-runner tests and 214 module tests passing; all 12 Rust targets pass.
- [Native CI](https://github.com/openclaw/bun/actions/runs/37402121441): Linux 16/16 and Darwin 13/13, first attempt. Its merge tree exactly matches the reviewed candidate.
- [Windows CI](https://github.com/openclaw/bun/actions/runs/37402131975): x64 and ARM64 each 31/31, both native smoke jobs and manifest pass on the first attempt. The immutable workflow is separate from runtime source; native revisions, WebKit identity, archive/executable/profile hashes, and PE architectures are verified for this exact source commit.
- OpenClaw consumer tests pass 7 + 22 before and after this exact commit, with the before binary’s tree matching current main and isolated HOME/state/configuration/temp directories.

Format, JavaScript/source lint, Clippy, Miri, and lol-html tests pass. Mordant reports the existing `bare_bool_args` finding in `src/resolver/package_json.rs`; its source, baseline, configuration, toolchain, and explicitly advisory workflow setting are unchanged from main. The initial issue-finder run lacked authentication; no credentials or CI settings were changed.

The shared owner fix is also proposed in oven-sh#44613 with a passing upstream-specific build and full test-runner suite. The separate Windows rename fix remains in oven-sh#44601.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant