Add moonrun async fs wasm host#1772
Conversation
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
4 times, most recently
from
6f76597 to
a8a8ad2
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8a8ad21d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
from
a8a8ad2 to
039f74f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 039f74f4d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
2 times, most recently
from
e42c1ec to
1669171
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1669171910
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
2 times, most recently
from
d8d20e0 to
9336c4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9336c4ff3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
from
9336c4f to
56aa701
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56aa7019c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
from
56aa701 to
4aa437a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aa437a91d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let state = self.state.lock().unwrap(); | ||
| state.processes.get(process)?.clone() |
There was a problem hiding this comment.
When async code follows the normal spawn_process → make_wait_for_process_job path, this clones the HostProcess and leaves the original entry in state.processes; I checked the registered process imports and there is no matching free/close-process API, so every spawned process keeps a live table slot even after the wait job consumes the child. Long-running wasm programs that spawn many processes will leak these handles until the 65,536-slot limit is reached; moving/removing the process into the wait job would let the slot be reused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 567af62e3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[cfg(windows)] | ||
| { | ||
| let _ = files; | ||
| Err(AsyncHostError::NotSupported) |
There was a problem hiding this comment.
On Windows, moonbit_v0.pipe is registered as a native-mapped import (registry.rs maps it to moonbitlang_async_pipe), but this host path always returns NotSupported. Any wasm async code that creates pipes on Windows, such as process stdio setup before spawn_process, will fail with ENOSYS even though the runtime otherwise advertises Windows support for the async host; this should either create host file handles for a Windows pipe or be declared unsupported consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddf60d73ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f449505c51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let len = usize::try_from(len).map_err(|_| AsyncHostError::Fault)?; | ||
| Ok(OsString::from_wide(&buffer[..len])) |
There was a problem hiding this comment.
On Windows systems where the selected temp directory is longer than MAX_PATH, GetTempPath2W returns the required buffer length instead of filling this fixed-size buffer; this code then slices buffer[..len] with len > buffer.len() and panics, so async @fs.tmpdir/get_tmp_path can crash moonrun rather than reporting an error. Retry with a resized buffer when the returned length exceeds the current capacity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5eaa5ab10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unsupported unsupported::i32 => "os_string/c_buffer_as_string", | ||
| native = Some("moonbitlang_async_c_buffer_as_string"), | ||
| sources = [moonbit_async:"src/internal/os_string/stub.c"]; |
There was a problem hiding this comment.
When wasm async code needs to turn a native OS-string buffer into a MoonBit String (for example tmpdir or directory-entry names), it imports moonbit_v0.os_string/c_buffer_as_string; this registry entry still wires that native ABI symbol to the generic unsupported stub, so those call sites receive ENOSYS/-1 instead of decoded text even though a ported decoder exists under async_sys/internal/os_string/stub.rs. Register an async_api wrapper for the decoder instead of marking this native import unsupported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ed65f4a89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state: Arc::clone(&self.state), | ||
| }; | ||
| thread_pool::run_host_job(&mut job, &mut files); | ||
| thread_pool::complete_guest_job(&mut job, memory)?; |
There was a problem hiding this comment.
When a synchronous run_job has an invalid guest output range (for example a read/readdir/file-time job whose destination is out of bounds), complete_guest_job returns an error here before the taken job is put back into the table. Because take leaves the slot reserved, the original job handle becomes permanently unusable and cannot be freed, so repeated bad buffers can leak job slots until the table is exhausted; ensure the job is reinserted even when copy-out fails.
Useful? React with 👍 / 👎.
| .ptr | ||
| .checked_add(dst.offset) | ||
| .ok_or(AsyncHostError::Fault)?; | ||
| memory.write(GuestRange::new(dst_offset, len)?, result)?; |
There was a problem hiding this comment.
For file-time jobs, the host records the caller-provided out_len in dst.len, but this copy-out uses result.len() instead. If wasm passes a too-small timestamp buffer that is still within linear memory, moonrun writes the full 48-byte record past the logical buffer and corrupts adjacent guest data instead of reporting an error; validate result.len() <= dst.len before writing.
Useful? React with 👍 / 👎.
| windows_sys::Win32::Foundation::SetHandleInformation( | ||
| handle as _, | ||
| windows_sys::Win32::Foundation::HANDLE_FLAG_INHERIT, | ||
| windows_sys::Win32::Foundation::HANDLE_FLAG_INHERIT, | ||
| ) |
There was a problem hiding this comment.
On Windows, when a Moonrun file handle is passed as child stdio, this marks the parent handle inheritable and never clears it after CreateProcessW. Because this spawn path always uses inheritable handles, later child processes can inherit earlier stdio files/pipes unintentionally, keeping them open and delaying EOF/deletion; reset the inheritance flag after the spawn attempt or duplicate temporary inheritable handles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73efd6105f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let max_jobs = usize::try_from(max_jobs).map_err(|_| AsyncHostError::Fault)?; | ||
| let mut state = self.state.lock().unwrap(); | ||
| let mut output = vec![0; max_jobs]; | ||
| let bytes = thread_pool::fetch_completion(&mut state.completions, &mut output)?; |
There was a problem hiding this comment.
When fetch_completion is called with an invalid or too-small guest output buffer, this line drains IDs from state.completions before any destination writes are validated. The later memory.write_i32_le error then returns to the guest after those completion IDs have been popped, so the corresponding async jobs are never reported again and their waiting coroutines can hang. Validate the whole destination range first or requeue the IDs on copy-out failure.
Useful? React with 👍 / 👎.
| if len < 2 { | ||
| return Err(AsyncHostError::Fault); | ||
| } | ||
| let fds = context.host.pipe()?; |
There was a problem hiding this comment.
If the guest passes an out-of-bounds dst to moonbit_v0.pipe, this creates and inserts both host pipe handles before attempting to write them back to wasm memory. A subsequent memory.write_i32_le failure leaves the handles in the host table with no values returned to the guest, so they cannot be closed and repeated bad buffers can exhaust file descriptors/handle slots. Validate the output range before creating the pipe or remove the inserted handles on failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc8d88fdf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut status = 0; | ||
| let ret = unsafe { libc::waitpid(self.pid, &mut status, 0) }; | ||
| if ret == self.pid { | ||
| Ok(libc::WEXITSTATUS(status)) |
There was a problem hiding this comment.
When a Unix child is terminated by a signal (for example sh -c 'kill -TERM $$'), waitpid still returns the pid but the status is not an exited status; reading WEXITSTATUS in that case reports a bogus exit code (often 0 on Linux). This makes make_wait_for_process_job report a signaled process as a successful exit, so check WIFEXITED/WIFSIGNALED and map signaled statuses distinctly before returning.
Useful? React with 👍 / 👎.
|
|
||
| #[cfg(windows)] | ||
| fn push_windows_arg(out: &mut String, arg: &str) { | ||
| let need_quote = arg.chars().any(|ch| matches!(ch, ' ' | '\t' | '"')); |
There was a problem hiding this comment.
When a spawned Windows process receives an empty argument, this condition leaves need_quote false and push_windows_arg appends nothing, so an argv like ['cmd', '', 'next'] loses the empty element in the command line passed to CreateProcessW. Empty arguments need to be emitted as "" to preserve the child's argc/argv.
Useful? React with 👍 / 👎.
peter-jerry-ye
force-pushed
the
codex/wasm-async-fs-host
branch
from
1e82d9f to
7fe1601
Compare
1 participant
Why
This adds the moonrun-side host runtime needed for the wasm backend of
moonbitlang/asyncto execute async filesystem programs without compiler changes.What Changed
moonbit_v0async host import surface for core filesystem, process, timer, errno, fd, and worker operations.Correctness
The implementation preserves the semantic C-stub boundary: MoonBit wasm calls
moonbit_v0imports with explicit pointer/length arguments, while moonrun owns native resources through checked host handle tables. Worker jobs reserve job handles while running so completions cannot be lost through handle reuse, and directory/process/file APIs use bounded guest-memory reads and writes.