Repository navigation
fix(omp): wait for process fixtures before reading procfs - #1869
gezerui888 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Readiness signaling and process probes server/omp-process.test.ts |
A helper waits up to three seconds for the expected stdout byte and cancels and releases the reader. Both tests pipe stdout and wait for readiness before probing. The empty-argument test keeps its existing argument values. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- devswha/ovis#1854 reports that the empty-arguments test can probe
/procbefore the child starts. The readiness wait addresses this scenario.
Possibly related PRs
- devswha/ovis#1807 also waits for process startup before checking process information, but uses a different fixture and API.
Suggested reviewers: devswha, christso
Merge Risk: 
7af7c
-
No actionable merge-blocking risk is established; the readiness change is mergeable.
Pre-merge checks: 
-
Check name Status Explanation Description check The description includes the required Change and Validation sections. It explains the readiness-signal fix, references issue #1854, and reports targeted tests, repeated runs, and broader check results…Title check The title clearly and concisely describes the main change: waiting for process fixtures before reading procfs. Linked Issues check Issue #1854requires the two live-process tests to wait until the child starts before reading/proc/<pid>. The PR addswaitForReady, pipes child stdout, and waits for the.readiness byte before…Out of Scope Changes check The diff is limited to server/omp-process.test.ts. It adds readiness handling and updates the two process fixtures needed for issue#1854. It preserves process cleanup and existing assertions. No un…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Full details: Description check
-
Explanation
The description includes the required Change and Validation sections. It explains the readiness-signal fix, references issue
#1854, and reports targeted tests, repeated runs, and broader check results.
Full details: Linked Issues check
-
Explanation
Issue
#1854requires the two live-process tests to wait until the child starts before reading/proc/<pid>. The PR addswaitForReady, pipes child stdout, and waits for the.readiness byte before each probe. The helper has a 3-second deadline and cleanup. The empty-argument test retains its empty arguments and assertions. The reported test validation supports the automated-test requirement.
Full details: Out of Scope Changes check
-
Explanation
The diff is limited to
server/omp-process.test.ts. It adds readiness handling and updates the two process fixtures needed for issue#1854. It preserves process cleanup and existing assertions. No unrelated behavior or files are changed.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
…o arrive (#1883) ## Change A signed-in device (a watch-only one included) could hold every one of the 16 read slots of `POST /api/connection-diagnostics` by sending each report body a byte at a time: the read had a size bound but no time bound, so every other report got 429 until those reads ended (#1425, item 1). - `server/client-connection-log.ts`: each body now has one 10 s deadline for the whole read, not a per-chunk timeout. A timer covers a body that stops arriving; a clock check after each chunk covers one that keeps trickling. Past it the request gets **408 `trace_timeout`**, the reader is cancelled, and the slot is released by the existing `finally` in `handle()`. - `shared/protocol.ts`: the route's comment lists the new 408. - Item 4: `src/lib/connectionTrace.test.ts` closes every trace a test opens (a later test's `resumeConnectionTraces()` reached earlier tests' traces), and the new `src/lib/api-signin.test.ts` covers `authenticate` and `pairDevice`: request shape, reporting resumed after a successful sign-in, and not resumed after a refused one. Items 2 and 3 are left as they are, as the issue describes them: the report quota is charged only after validation so malformed posts cannot starve real ones (each parse is at most 8 KiB, 16 at once), and a 4xx refusal drops that incident's batch by design (`connectionTrace.ts`, tested); the 408 follows that rule and the next incident still posts. This adds a status code to an HTTP route's contract, so it needs a real review. Fixes #1425 ## Verification - `server/client-connection-log.test.ts`: "ends a body that keeps trickling in once the deadline for the whole body passes" (fake clock, no sleeps) and "gives every read slot back when the bodies holding them stop arriving" (16 stalled bodies, 1 ms injected deadline). On origin/main they fail (413 / a 5 s test timeout); on this branch they pass. - `src/lib/api-signin.test.ts`: removing `resumeConnectionTraces()` from `api.ts` fails the two "resume" cases; resuming before the `ok` check fails the two "refused" cases. - Live, against `createServer({ port: 0 })` with 16 raw-TCP senders at one byte per second, rotating `X-Forwarded-For`: this branch answers each `HTTP/1.1 408 Request Timeout` at 10.0 s and the next report gets 204; origin/main answers none of them within 20 s and the next report still gets 429. - Reviews: omo-native-code-reviewer APPROVE (two LOW notes, both the documented trade-offs above); Codex (gpt-6.1-sol, xhigh) APPROVE with no findings. - `bun run check fast`: workflow syntax, generated files, typecheck and build pass; unit tests 6395 pass, 4 fail, all outside this change. Three in `server/shell-agent.test.ts` fail with EMFILE from `fs.watch`, on origin/main too: this PC is at its inotify instance limit (126 of 128) from other sessions. One in `server/omp-process.test.ts` is the readiness flake #1862 and #1869 address; it passes when run alone. --------- Co-authored-by: mp <mp@local>
Change
I made both process fixtures signal readiness before the tests read
/proc. This avoids reading inherited argv or environment data before the child executes its script. The wait has a deadline and preserves process cleanup and empty-argument assertions.Fixes #1854.
Validation
bun test server/omp-process.test.tspassed all 6 tests; 50 repeated runs also passed.bun run check fast: generated types, generated themes, typecheck, and build passed. The unit suite had 9 failures, all also present on unchanged main (427f5d3). Main had 10 failures; this change introduced none.