Skip to content

Conversation

mattdholloway
Copy link
Contributor

@mattdholloway mattdholloway commented Aug 13, 2025

Extends the get_job_logs tool and other parts of the MCP server that rely on downloading Actions job log data. The new functionality ensures memory leaks do not occur by using a 'sliding window' buffer to only have 5MB of the log in memory at a time as it reads the entire log. This then allows for a tail step after this that retains the correct amount of lines within the final window before completion.

Closes: https://github.com/github/copilot-agent-services/issues/375

@mattdholloway mattdholloway marked this pull request as ready for review August 13, 2025 12:56
@mattdholloway mattdholloway requested a review from a team as a code owner August 13, 2025 12:56
@Copilot Copilot AI review requested due to automatic review settings August 13, 2025 12:56
Copilot

This comment was marked as outdated.

@mattdholloway mattdholloway requested a review from Copilot August 15, 2025 14:16
Copilot

This comment was marked as outdated.

@mattdholloway mattdholloway requested a review from Copilot August 18, 2025 10:45
Copilot

This comment was marked as outdated.

Copy link
Member

@omgitsads omgitsads left a comment

Choose a reason for hiding this comment

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

Are the content_window_size underscores intentionally, or should they be - instead?

@mattdholloway mattdholloway requested a review from Copilot August 18, 2025 14:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the get_job_logs tool with a sliding window buffer mechanism to prevent memory leaks when processing large Actions job log files. The implementation uses a ring buffer that maintains only the last 5MB (configurable) of log data in memory at any time, significantly reducing memory usage while preserving functionality.

Key changes:

  • Added configurable content window size parameter throughout the system
  • Implemented ring buffer log processing to handle large files efficiently
  • Added performance profiling capabilities to measure memory usage improvements

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/github/tools.go Added contentWindowSize parameter to DefaultToolsetGroup and updated GetJobLogs tool instantiation
pkg/github/actions_test.go Updated test calls to include contentWindowSize parameter and added comprehensive memory usage comparison tests
pkg/github/actions.go Modified log processing functions to use ring buffer and added profiling integration
pkg/buffer/buffer.go New ring buffer implementation for efficient log processing with sliding window
internal/profiler/profiler.go New profiling system for measuring memory usage and performance metrics
internal/ghmcp/server.go Added ContentWindowSize configuration field and passed it through server initialization
cmd/github-mcp-server/main.go Added CLI flag for content-window-size configuration
cmd/github-mcp-server/generate_docs.go Updated calls to include default contentWindowSize value

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

if err != nil {
return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err)
if tailLines <= 0 {
tailLines = 1000
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

The magic number 1000 should be defined as a named constant to improve maintainability and make the default value explicit throughout the codebase.

Suggested change
tailLines = 1000
tailLines = DefaultTailLines

Copilot uses AI. Check for mistakes.

return -int64(diff)
}

return int64(after) - int64(before)
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

This condition checks if either value exceeds MaxInt64, but since these are uint64 values, they can legitimately be larger than MaxInt64. The logic should handle the conversion more clearly by checking if the difference itself would overflow int64 bounds.

Suggested change
return int64(after) - int64(before)
if after >= before {
diff := after - before
if diff > math.MaxInt64 {
return math.MaxInt64
}
return int64(diff)
}
diff := before - after
if diff > math.MaxInt64 {
return -math.MaxInt64
}
return -int64(diff)

Copilot uses AI. Check for mistakes.

@mattdholloway mattdholloway merged commit 2621dbe into main Aug 19, 2025
16 checks passed
@mattdholloway mattdholloway deleted the actions-job-log-buffer branch August 19, 2025 13:02
ipapapa pushed a commit to ipapapa/github-mcp-server that referenced this pull request Aug 28, 2025
* add sliding window for actions logs

* refactor: fix sliding

* remove trim content

* only use up to 1mb of memory for logs

* update to tail lines in second pass

* add better memory usage calculation

* increase window size to 5MB

* update test

* update vers

* undo vers change

* add incremental memory tracking

* use ring buffer

* remove unused ctx param

* remove manual GC clear

* fix cca feedback

* extract ring buffer logic to new package

* handle log content processing errors and use correct param for maxjobloglines

* fix tailing

* account for if tailLines exceeds window size

* add profiling thats reusable

* remove profiler testing

* refactor profiler: introduce safeMemoryDelta for accurate memory delta calculations

* linter fixes

* Update pkg/buffer/buffer.go

Co-authored-by: Copilot <[email protected]>

* use flag for maxJobLogLines

* add param passing for context window size

* refactor: rename contextWindowSize to contentWindowSize for consistency

* fix: use tailLines if bigger but only if <= 5000

* fix: limit tailLines to a maximum of 500 for log content download

* Update cmd/github-mcp-server/main.go

Co-authored-by: Adam Holt <[email protected]>

* Update cmd/github-mcp-server/main.go

Co-authored-by: Adam Holt <[email protected]>

* move profiler to internal/

* update actions test with new profiler location

* fix: adjust buffer size limits

* make line buffer 1028kb

* fix mod path

* change test to use same buffer size as normal use

* improve test for non-sliding window implementation to not count empty lines

* make test memory measurement more accurate

* remove impossible conditional

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: Adam Holt <[email protected]>
LuluBeatson added a commit that referenced this pull request Sep 4, 2025
* docs: Add Google Gemini CLI installation guide and integration

- Add comprehensive installation guide for Google Gemini CLI
- Include Docker and binary configuration options
- Add authentication setup for Gemini API and Vertex AI
- Update main README.md to include Gemini CLI in installation guides
- Update installation guides index with Gemini CLI entry and support matrix
- Follow established documentation patterns and security best practices

* Fix Gemini CLI command syntax and add remote server method

- Replace all 'gemini-cli' commands with correct 'gemini' syntax
- Fix verification commands to use '/mcp list' and '/tools' prompts
- Add httpUrl remote server method as primary configuration option
- Update config file paths from settings.json to config.json
- Correct npx installation command syntax
- Add link to official Gemini CLI documentation

Addresses feedback from soisyourface in PR review.

* Emphasize official Gemini CLI documentation link

Reduce detailed installation steps and direct users to official docs for
up-to-date instructions, addressing reviewer feedback about maintainability.

* Fix Gemini CLI configuration file name: config.json -> settings.json

The correct configuration file for Gemini CLI is settings.json, not config.json.
This applies to both global (~/.gemini/settings.json) and project-specific
(.gemini/settings.json) configurations as confirmed by official documentation.

* Remove Gemini CLI installation and authentication sections

Removed lines 11-41 containing Gemini CLI installation commands and
authentication setup instructions.

* Add Podman as Docker alternative in prerequisites

Added Podman as container engine option alongside Docker.

* Remove references to deprecated npm package

* Add comprehensive ~/.gemini/.env file example

* Fix authorization header to use literal token placeholder

Environment variable substitution in headers is not yet supported
by Gemini CLI (see google-gemini/gemini-cli#5282).

* Add issue types (#869)

* feat: add type to issues

* test: add `type` test for create and update issues

* Generate docs and toolsnaps

* Update pkg/github/issues.go

Co-authored-by: Copilot <[email protected]>

* Use github ptr

---------

Co-authored-by: Pranav RK <[email protected]>
Co-authored-by: Pranav RK <[email protected]>
Co-authored-by: Alon Kenneth <[email protected]>
Co-authored-by: Copilot <[email protected]>

* Enable Dependabot (#654)

* Create/Update dependabot.yaml

* Apply suggestion from @Copilot

Co-authored-by: Copilot <[email protected]>

---------

Co-authored-by: Copilot <[email protected]>

* Bump SDK version to 0.36.0 (#863)

* Use server.ServerResourceTemplate and server.ServerPrompt wrappers (#886)

* Update "Close inactive issues" workflow to close issues after 180 days of inactivity (#909)

* update PR_DAYS_BEFORE_STALE

* update to mark as stale after 60 days

* Update Claude MCP install guide after testing (#706)

* Revise Claude installation guide

- Verified Claude Code installation steps
- Identified and documented issues with Claude Desktop setup
- Updated installation documentation based on testing

* Revise instructions for opening Claude Code

Updated recommendations for opening Claude Code.

* Update docs/installation-guides/install-claude.md

Co-authored-by: Copilot <[email protected]>

* Update docs/installation-guides/install-claude.md

Co-authored-by: Copilot <[email protected]>

* Update installation guide for Claude setup

Added installation option for using Claude Code using a release binary.

* Change section title for Go Binary installation

Updated section title for clarity regarding installation without Docker.

* Close double quote in bash command

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: LuluBeatson <[email protected]>
Co-authored-by: Matt Holloway <[email protected]>
Co-authored-by: Tommaso Moro <[email protected]>

* Add actions job log buffer and profiler (#866)

* add sliding window for actions logs

* refactor: fix sliding

* remove trim content

* only use up to 1mb of memory for logs

* update to tail lines in second pass

* add better memory usage calculation

* increase window size to 5MB

* update test

* update vers

* undo vers change

* add incremental memory tracking

* use ring buffer

* remove unused ctx param

* remove manual GC clear

* fix cca feedback

* extract ring buffer logic to new package

* handle log content processing errors and use correct param for maxjobloglines

* fix tailing

* account for if tailLines exceeds window size

* add profiling thats reusable

* remove profiler testing

* refactor profiler: introduce safeMemoryDelta for accurate memory delta calculations

* linter fixes

* Update pkg/buffer/buffer.go

Co-authored-by: Copilot <[email protected]>

* use flag for maxJobLogLines

* add param passing for context window size

* refactor: rename contextWindowSize to contentWindowSize for consistency

* fix: use tailLines if bigger but only if <= 5000

* fix: limit tailLines to a maximum of 500 for log content download

* Update cmd/github-mcp-server/main.go

Co-authored-by: Adam Holt <[email protected]>

* Update cmd/github-mcp-server/main.go

Co-authored-by: Adam Holt <[email protected]>

* move profiler to internal/

* update actions test with new profiler location

* fix: adjust buffer size limits

* make line buffer 1028kb

* fix mod path

* change test to use same buffer size as normal use

* improve test for non-sliding window implementation to not count empty lines

* make test memory measurement more accurate

* remove impossible conditional

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: Adam Holt <[email protected]>

* Add get_release_by_tag tool (#938)

* add get_release_by_tag tool

* add tool

* add tests

* autogen

* remove comment

* docs(readme): Update readme to point to correct installation guides index (#892)

* docs(readme): Update readme to point to correct installation guides index

* feat(contributors): add list_repository_contributors tool

* Revert "feat(contributors): add list_repository_contributors tool"

This reverts commit ece480e.

---------

Co-authored-by: Tommaso Moro <[email protected]>

* Add Global Security Advisories Toolset (#919)

* Repository security advisories (#925)

* Add support for listing repo level security advisories

* Add support for listing repo security advisories at the org level

* Update Cursor installation link (#940)

* use new link

* update local install link

* Change role from "system" to "user" in prompt messages for `AssignCodingAgentPrompt` and `IssueToFixWorkflowPrompt`. Role "system" is not allowed by Claude Code in MCP provided prompt (allowed only role "user" and "assistant") (#941)

Co-authored-by: 0xGosu <[email protected]>

* Local MCP is supported

* Refactor Gemini CLI install guide

* Remove Bearer from Authorization header

* Add reference to main README for latest config

* Bearer needed for headers, add references

* Add minimal response to CRUD tools, `repositories` and `search` toolsets (#988)

* add comprehensive minimal response where appropriate

* remove unneeded comments

* remove incorrect diff param

* update docs

* rm comment

* Update pkg/github/repositories.go

Co-authored-by: Lulu <[email protected]>

* update toolsnaps and docs

* change minimal_output to use new OptionalBoolParamWithDefault

* Update pkg/github/repositories.go

Co-authored-by: Lulu <[email protected]>

* refactor minimal conversion funcs to minimal_types.go

* consolidate response structs and remove unneeded message field

* consolidate response further

* remove CloneURL field

* Update pkg/github/repositories.go

Co-authored-by: Copilot <[email protected]>

* Update pkg/github/server.go

Co-authored-by: Copilot <[email protected]>

* fix undefined

* change incorrect comment

* remove old err var declaration

* Update pkg/github/repositories.go

Co-authored-by: Copilot <[email protected]>

* fix syntax issue

* update toolsnaps

---------

Co-authored-by: Lulu <[email protected]>
Co-authored-by: Copilot <[email protected]>

* initial org repo create support (#1023)

---------

Co-authored-by: JoannaaKL <[email protected]>
Co-authored-by: Pranav RK <[email protected]>
Co-authored-by: Pranav RK <[email protected]>
Co-authored-by: Alon Kenneth <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Zack Koppert <[email protected]>
Co-authored-by: Ksenia Bobrova <[email protected]>
Co-authored-by: Tommaso Moro <[email protected]>
Co-authored-by: Dimitrios Philliou <[email protected]>
Co-authored-by: LuluBeatson <[email protected]>
Co-authored-by: Matt Holloway <[email protected]>
Co-authored-by: Adam Holt <[email protected]>
Co-authored-by: Rebecca Biju <[email protected]>
Co-authored-by: Jurre <[email protected]>
Co-authored-by: 0xGosu <[email protected]>
Co-authored-by: Lulu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants