fix: make OMP process lifecycle tests full-suite safe#2290
Conversation
Same class as grok-runtime: move the Symbol.for process.exit reaper onto process-manager and reimport that module in the bound stress test so full- suite shard 4 no longer times out on omp-runtime.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe OMP ACP process-exit cleanup hook is moved into ChangesOMP lifecycle cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Full Suite 29636550951: quarantine engine heartbeat-error-recovery (30s timeout under shard load) and wait for QuickEntry submit to leave the Creating... state before asserting Fast toggle reset.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/fusion-plugin-omp-runtime/src/__tests__/process-lifecycle.test.ts`:
- Around line 17-23: Remove the explicit 15,000 ms timeout from the test “keeps
its process cleanup owner bounded across repeated module evaluation.” Keep the
test’s assertions and behavior unchanged; address any remaining slowness through
a narrow seam, fake, or fake timers rather than increasing time limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 561921af-1dab-489f-b923-c75aaa0c336c
📒 Files selected for processing (4)
.changeset/omp-process-lifecycle-owner.mdplugins/fusion-plugin-omp-runtime/src/__tests__/process-lifecycle.test.tsplugins/fusion-plugin-omp-runtime/src/acp/process-manager.tsplugins/fusion-plugin-omp-runtime/src/index.ts
| /* | ||
| FNXC:OmpRuntimeTests 2026-07-18-08:10: | ||
| Same full-suite class as grok-runtime: prove Symbol.for exit-hook bound by | ||
| re-importing process-manager (lifecycle owner), not the full plugin graph, | ||
| with a 15s cold-transform budget under shard load. | ||
| */ | ||
| it("keeps its process cleanup owner bounded across repeated module evaluation", { timeout: 15_000 }, async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not appease test flakiness with larger timeouts.
Adding a 15_000 ms timeout to this test violates the repository's coding guidelines. The guidelines explicitly state: "Never appease flakes with larger timeouts, retries, weakened assertions, or deleted assertions." If a test is flaky under heavy load or takes too long, it should be quarantined with a ledger entry and matching Vitest exclusion, or addressed using narrow seams, fakes, or fake timers.
♻️ Proposed fix
/*
FNXC:OmpRuntimeTests 2026-07-18-08:10:
Same full-suite class as grok-runtime: prove Symbol.for exit-hook bound by
re-importing process-manager (lifecycle owner), not the full plugin graph,
- with a 15s cold-transform budget under shard load.
+ with standard timeouts under shard load.
*/
- it("keeps its process cleanup owner bounded across repeated module evaluation", { timeout: 15_000 }, async () => {
+ it("keeps its process cleanup owner bounded across repeated module evaluation", async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* | |
| FNXC:OmpRuntimeTests 2026-07-18-08:10: | |
| Same full-suite class as grok-runtime: prove Symbol.for exit-hook bound by | |
| re-importing process-manager (lifecycle owner), not the full plugin graph, | |
| with a 15s cold-transform budget under shard load. | |
| */ | |
| it("keeps its process cleanup owner bounded across repeated module evaluation", { timeout: 15_000 }, async () => { | |
| /* | |
| FNXC:OmpRuntimeTests 2026-07-18-08:10: | |
| Same full-suite class as grok-runtime: prove Symbol.for exit-hook bound by | |
| re-importing process-manager (lifecycle owner), not the full plugin graph, | |
| with standard timeouts under shard load. | |
| */ | |
| it("keeps its process cleanup owner bounded across repeated module evaluation", async () => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/fusion-plugin-omp-runtime/src/__tests__/process-lifecycle.test.ts`
around lines 17 - 23, Remove the explicit 15,000 ms timeout from the test “keeps
its process cleanup owner bounded across repeated module evaluation.” Keep the
test’s assertions and behavior unchanged; address any remaining slowness through
a narrow seam, fake, or fake timers rather than increasing time limits.
Source: Coding guidelines
Greptile SummaryThis PR moves OMP exit cleanup into the process manager and updates full-suite test handling. The main changes are:
Confidence Score: 5/5The runtime change looks safe to merge, with non-blocking gaps in the lifecycle test.
plugins/fusion-plugin-omp-runtime/src/tests/process-lifecycle.test.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix: quarantine heartbeat timeout flake ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const warnings: Error[] = []; | ||
| const onWarning = (warning: Error) => warnings.push(warning); | ||
| process.on("warning", onWarning); | ||
|
|
There was a problem hiding this comment.
Importing process-manager directly always installs the hook, so this test still passes if the changed side-effect import in index.ts is removed or broken. Normal plugin loading would then leave OMP children running at process exit; the test should import ../index.js at least once before using manager reimports to check listener bounds.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| */ | ||
| it("keeps its process cleanup owner bounded across repeated module evaluation", { timeout: 15_000 }, async () => { | ||
| const baseline = listenerCounts(); | ||
| const warnings: Error[] = []; |
There was a problem hiding this comment.
The reported failure used 15 repeated evaluations under shard load, but this change cuts the loop to five while raising the timeout to 15 seconds. A slowdown or listener-growth problem that appears only under the original stress level can now pass, so the test no longer reproduces the failure condition described by this fix.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…de (#2291) ## Summary Latest Full Suite after #2290 was green on shards 1–2 and nearly green on 3–4: - **Shard 3:** QuickEntry agent picker outside click left the portal open (product) — capture-phase mousedown + open-token so late `fetchAgents` cannot re-open a dismissed picker - **Shard 4:** `@runfusion/fusion` package-lane cascade (87 failures from `extension-dist-barrel` hookTimeout + lock-retry timeouts under load) — quarantine the 14 observed files on sight (ledger + vitest exclude), no timeout appeasement Also hardens the agent-picker outside-click test. ## Test plan - [x] Local agent picker portal tests green - [ ] PR gate - [ ] Post-merge Full Suite green <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed the Quick Add agent picker so it reliably closes when clicking outside. - Prevented delayed agent-loading results from reopening the picker after it has been dismissed. - Improved the picker’s loading behavior by displaying it immediately while agents are being retrieved. - **Tests** - Added coverage for dismissing the agent picker with an outside click. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
After #2289, Full Suite shard 4 still failed on the OMP twin of the Grok process-lifecycle stress test (
import("../index.js")× 15 under shard transform load → 5s timeout).Apply the same fix class as grok-runtime:
process-managerTest plan
Summary by CodeRabbit
Bug Fixes
Tests