Skip to content

fix: Fix mocker+router segfaults + reliability fixes#7162

Merged
jthomson04 merged 11 commits intomainfrom
jthomson04/segfault-shutdown
Mar 11, 2026
Merged

fix: Fix mocker+router segfaults + reliability fixes#7162
jthomson04 merged 11 commits intomainfrom
jthomson04/segfault-shutdown

Conversation

@jthomson04
Copy link
Contributor

@jthomson04 jthomson04 commented Mar 10, 2026

Overview:

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Tests

    • Re-enabled previously skipped test to improve test coverage.
    • Increased timeout durations for router tests to improve reliability.
  • Chores

    • Added deprecation warning filter for cleaner output.
    • Improved internal process cleanup handling in test utilities.

@github-actions github-actions bot added the fix label Mar 10, 2026
@jthomson04 jthomson04 force-pushed the jthomson04/segfault-shutdown branch from 92b1041 to e85a1f4 Compare March 10, 2026 17:58
@jthomson04 jthomson04 force-pushed the jthomson04/segfault-shutdown branch from 8341c63 to e85a1f4 Compare March 10, 2026 19:46
@jthomson04 jthomson04 force-pushed the jthomson04/segfault-shutdown branch from 9d7d679 to 7db174e Compare March 10, 2026 23:30
@jthomson04 jthomson04 force-pushed the jthomson04/segfault-shutdown branch from 7db174e to e8ade26 Compare March 10, 2026 23:52
@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 11, 2026
@jthomson04 jthomson04 marked this pull request as ready for review March 11, 2026 02:18
@jthomson04 jthomson04 requested review from a team as code owners March 11, 2026 02:18
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

Walkthrough

This pull request refactors subprocess management in test utilities by consolidating cleanup logic into a centralized helper method, re-enables a previously skipped test, increases timeout thresholds for two tests, and updates configuration settings.

Changes

Cohort / File(s) Summary
Configuration Updates
pyproject.toml
Added DeprecationWarning filter for Swig-related attributes; removed trailing whitespace.
Process Management Refactoring
tests/conftest.py, tests/utils/managed_process.py
Introduced new _stop_started_processes() helper method for centralized subprocess cleanup; refactored NatsServer.stop() and ManagedProcess.__exit__() to delegate to the helper; improved _terminate_process_group() with zombie process reaping.
Test Adjustments
tests/router/test_router_e2e_with_mockers.py
Removed global test skip marker; increased timeout durations for test_indexers_sync (180s → 300s) and test_kv_router_bindings (90s → 300s).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hops through the code with glee,
New helper methods, clean and free!
Process cleanup, unified at last,
Tests re-enabled, timeouts amassed,
A refactor neat, organized flow—
This PR makes testing's garden grow! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description contains only empty template placeholders with no actual information about what changes were made, why, or where to review. Fill in all template sections with concrete details: summarize the changes, explain the rationale for process shutdown refactoring and timeout adjustments, and identify which files need close review.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title clearly relates to the main changes: fixing segfaults in mocker+router tests and improving reliability through process management refactoring and test timeout adjustments.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/utils/managed_process.py`:
- Around line 348-358: In _stop_started_processes, don't treat a wait timeout as
success: when terminate_process_tree(process.pid, ...) or
process.wait(timeout=...) raises a timeout, retry a forceful shutdown (e.g., use
process.kill() or call terminate_process_tree with a force parameter), re-wait
for exit, then only clear the Popen handle via setattr(self, attr_name, None)
after process.poll() confirms the process exited; if the process still hasn't
exited, raise a specific exception so callers (like NatsServer.stop) can detect
failure instead of proceeding with a stale handle. Also catch specific
exceptions (TimeoutExpired, OSError) rather than Exception and re-raise or
handle deterministically per the guidelines.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2e9e9859-7152-4434-b980-ca33c7877d78

📥 Commits

Reviewing files that changed from the base of the PR and between 0c94f00 and fde5c1e.

📒 Files selected for processing (4)
  • pyproject.toml
  • tests/conftest.py
  • tests/router/test_router_e2e_with_mockers.py
  • tests/utils/managed_process.py

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04 jthomson04 requested a review from a team as a code owner March 11, 2026 02:59
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 11, 2026
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04 jthomson04 changed the title fix: Maybe fix mocker segfaults fix: Fix mocker+router segfaults + reliability fixes Mar 11, 2026
@jthomson04 jthomson04 merged commit 761e848 into main Mar 11, 2026
93 of 94 checks passed
@jthomson04 jthomson04 deleted the jthomson04/segfault-shutdown branch March 11, 2026 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants