Skip to content

Add process stop cleanup regression coverage#2377

Open
johnynek wants to merge 9 commits into
mainfrom
agent/small/2376-add-cross-backend-process-stop-and-cleanup-regression-coverage
Open

Add process stop cleanup regression coverage#2377
johnynek wants to merge 9 commits into
mainfrom
agent/small/2376-add-cross-backend-process-stop-and-cleanup-regression-coverage

Conversation

@johnynek

@johnynek johnynek commented May 4, 2026

Copy link
Copy Markdown
Owner

Adds cross-backend regression coverage for the portable process stop/status contract and managed with_process cleanup behavior. The shared JVM/Python process test now exercises stable status observations, timeout non-consumption, already-exited stop behavior, terminate/kill post-stop stability, low-level stdio ownership through explicit close, and helper-owned versus caller-owned handle cleanup. C/libuv tests add matching low-level coverage for stable recorded status and ensure poll, zero-duration wait_timeout, terminate, and kill do not implicitly close returned pipe handles.

Fixes #2376

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces comprehensive regression coverage for the portable process stop and status contract across the C/libuv, JVM, and Python backends. Key changes include the addition of test helpers to verify that process exit status remains stable across repeated observations and new test cases ensuring that low-level operations like poll, terminate, and kill do not implicitly close caller-owned stdio pipe handles. Feedback suggests strengthening these ownership tests by using non-empty payloads during write operations, as zero-byte writes might be optimized away by certain runtime implementations, potentially failing to detect if a handle was incorrectly closed.

stdin_write_ok <- recover(
(
_ <- (match stdin_h:
case Some(h): write_utf8(h, "")

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.

medium

Writing an empty string to stdin_h may be a no-op in some backend implementations and might not actually verify that the handle remains open and valid. To robustly prove that the low-level stop functions did not close the returned handles, consider writing a non-empty string. Note that a flush operation should be avoided for this purpose as it is intended to be a no-op for performance on certain file descriptors.

          case Some(h): write_utf8(h, " ")
References
  1. For file descriptors without user-space buffers (e.g., libuv's uv_file), a flush operation should be a no-op for performance.

Comment thread c_runtime/test.c
0,
"IO/Core kill before stdio close should return StopSent for the running child");
return ___bsts_g_Bosatsu_l_Prog_l_flat__map(
___bsts_g_Bosatsu_l_IO_l_Core_l_write__bytes(slots[1], io_core_bytes_value(NULL, 0)),

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.

medium

Similar to the Bosatsu test case, writing zero bytes (io_core_bytes_value(NULL, 0)) might be optimized away by the runtime or the underlying libuv implementation, potentially failing to detect if the handle was incorrectly closed. Using a non-empty payload, as done in the terminate test case at line 1373, would provide a stronger regression check for handle ownership.

      ___bsts_g_Bosatsu_l_IO_l_Core_l_write__bytes(slots[1], io_core_bytes_value((const uint8_t*)" ", 1)),

@codecov

codecov Bot commented May 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.21%. Comparing base (a4aa784) to head (919c80f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2377      +/-   ##
==========================================
+ Coverage   85.18%   85.21%   +0.03%     
==========================================
  Files         196      196              
  Lines       49379    49379              
  Branches    12196    12196              
==========================================
+ Hits        42062    42078      +16     
+ Misses       7317     7301      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@johnynek

johnynek commented May 4, 2026

Copy link
Copy Markdown
Owner Author

Blocked before ready-to-push: I made the requested non-empty write changes and uncovered a JVM ownership issue where terminate via java.lang.Process.destroy() closes the child stdin stream. I patched JVM terminate to use ProcessHandle.destroy() and changed the Bosatsu ownership fixture to prove a non-empty post-terminate write before killing the child for cleanup; the C kill-path test now writes a one-byte payload. However, a required-tests run started before the final cleanup patch hung in the sandbox, and this environment does not permit process listing or pkill, so I could not obtain a clean passing scripts/test_basic.sh result in this turn. Because the required gate did not complete, this is not ready to push.

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.

Add cross-backend process stop and cleanup regression coverage

1 participant