Merged
Conversation
- Overhaul session management and process termination logic. - Resolve bugs in session teardown and task handling. - Improve test coverage and stability for background tasks.
There was a problem hiding this comment.
Pull request overview
Refactors the task execution subsystem to remove the old SessionPool abstraction, improve test/coverage reliability, and add pruning of idle callr::r_session workers to reduce memory footprint.
Changes:
- Reworked
Task/TaskManagerto optionally run tasks in persistentcallr::r_sessionworkers and prune idle sessions. - Updated
LanguageServerto use the newTaskManagersession model for parse/diagnostics tasks. - Added/updated testthat coverage-sensitive tests and adjusted test client shutdown behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
R/task.R |
Implements new session-backed task execution, session creation, and idle-session pruning; adds shutdown cleanup. |
R/languageserver.R |
Switches parse/diagnostics task managers to use_session=TRUE under the refactored TaskManager API. |
R/session.R |
Removes the previous Session/SessionPool implementation. |
tests/testthat/test-task.R |
Adds Task/TaskManager tests (including covr-mode and idle session pruning). |
tests/testthat/helper-utils.R |
Changes test client shutdown/cleanup behavior, with special handling under R_COVR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add `session_idle_timeout` to `TaskManager` (default 60s). - Implement `prune_sessions` to automatically close and remove idle sessions. - Add test case to verify session cleanup after inactivity.
Member
Author
|
Most of the copilot suggestions are related to closing the processes explicitly when running |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have decoupled the changes in the task manager from 0.3.17 release #717
Most significantly it fixes the coverage test issue. I have also added a functionality to prune idle sessions to reduce memory footprint. I have however also changed some of the cpu_load logic (not sure 0.8 for parsing tasks make sense, 0.8 for parsing + 0.5 for lintr > 100%), please took a look @renkun-ken.