Add process stop cleanup regression coverage#2377
Conversation
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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
- For file descriptors without user-space buffers (e.g., libuv's
uv_file), a flush operation should be a no-op for performance.
| 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)), |
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
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 |
Adds cross-backend regression coverage for the portable process stop/status contract and managed
with_processcleanup 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 ensurepoll, zero-durationwait_timeout,terminate, andkilldo not implicitly close returned pipe handles.Fixes #2376