Skip to content

fix: close idle HTTP connections to prevent process hanging on exit#4

Merged
yhuan123 merged 1 commit into
mainfrom
fix/close-idle-connections
Apr 1, 2026
Merged

fix: close idle HTTP connections to prevent process hanging on exit#4
yhuan123 merged 1 commit into
mainfrom
fix/close-idle-connections

Conversation

@yhuan123

@yhuan123 yhuan123 commented Apr 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • CLI 命令执行完毕后进程卡住不退出,原因是 GitLab SDK 底层 go-retryablehttp 维持的 idle HTTP connections 未被关闭
  • 新增 GitLabClient.CloseIdleConnections() 方法,在所有命令函数中通过 defer 调用确保退出时立即释放连接

Changes

  • pkg/client/client.go: 新增 CloseIdleConnections() 方法
  • internal/cli/cmd.go: 所有 5 个 run* 函数添加 defer gitlabClient.CloseIdleConnections()

Test plan

  • 运行 gitlab-cli user delete --username <user> 验证命令完成后立即退出
  • 运行 gitlab-cli user list --prefix <prefix> 验证同样秒退
  • make test 通过

🤖 Generated with Claude Code

The GitLab SDK's underlying go-retryablehttp maintains idle HTTP connections.
Without explicitly closing them, the process hangs after command completion
waiting for connection timeouts.

Added CloseIdleConnections() to GitLabClient and defer-called it in all
command functions to ensure immediate cleanup on exit.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@yhuan123 yhuan123 requested a review from dancer430 April 1, 2026 02:33

@alaudabot alaudabot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

This PR correctly addresses the CLI hang issue by closing idle HTTP connections on exit. The implementation is clean, follows Go best practices with proper defer usage, and applies the fix consistently across all 5 command entry points.

Suggestions (non-blocking):

  • Consider using English comments for broader maintainability, though this is consistent with existing codebase style.

No critical issues or warnings found. The fix is ready to merge.

@alaudabot

Copy link
Copy Markdown

🤖 AI Code Review

Property Value
Model opencode/minimax-m2.5-free
Style strict
Issues Found 0
Config Source centralized
Profile ❌ Not Found
Personalized Prompt ❌ No
Prompt Path .github/review/profiles/alaudadevops/gitlab-cli/pr-review.md
Alauda Skills ✅ base-sample-email-draft, builders-jira-cli, builders-sample-code-review, devops-autodns, devops-bulk-string-replace, devops-docker-keyword-modify, devops-task-overview-template, devops-tekton-dynamic-form-optimizer, devops-tekton-operator-task-e2e, devops-tekton-task-generator, devops-tekton-task-version-upgrade, devops-tektoncd-vuln-fix, devops-upgrade-go
Reviewed at 2026-04-01 02:35:21 UTC

Summary

This PR addresses a CLI hang issue where the process doesn't exit after command completion due to GitLab SDK's underlying go-retryablehttp maintaining idle HTTP connections. The fix adds a CloseIdleConnections() method to the GitLabClient and uses defer in all 5 run functions to ensure connections are closed on exit. The implementation is clean and follows Go best practices.

Review Statistics

Category Count
Critical Issues 0
Warnings 0
Suggestions 1
Files Reviewed 2

Critical Issues

Issues that MUST be addressed before merging (security, bugs, breaking changes)

None

Warnings

Issues that SHOULD be addressed but are not blocking

None

Suggestions

Recommendations for improvement (nice to have)

  • [internal/cli/cmd.go:138,211,260,357,386] (style/convention): Consider standardizing comment language. The new comment in pkg/client/client.go:27 uses Chinese ("关闭空闲连接,防止程序退出时卡住"), which is consistent with existing code, but the PR title and body are in English. For an open-source project, English comments would improve broader maintainability. However, since this is consistent with the existing codebase style, this is optional.

Positive Feedback

  • Good pattern: Using defer immediately after successful client initialization is the correct pattern to ensure cleanup happens regardless of how the function exits.
  • Clean API design: The CloseIdleConnections() method is a thin wrapper around http.Client.CloseIdleConnections(), which is the standard Go way to handle this issue.
  • Consistent application: The fix is applied to all 5 entry points (runUserCreate, runUserCleanup, runUserDelete, runUserList, runUserDeleteByPrefix), ensuring complete coverage.
  • No breaking changes: The implementation is backward compatible - closing idle connections is safe and has no side effects.


ℹ️ About this review

This review was automatically generated using the run-actions workflow.

  • Shared prompt: .github/prompts/code-review.md
  • Config source: centralized
  • Profile path: Not Found
  • Profile ref: c0c47624410504de6c1af1213b2c80cfcdaf38f3
  • No repository-specific prompt configured
  • Alauda skills: base-sample-email-draft, builders-jira-cli, builders-sample-code-review, devops-autodns, devops-bulk-string-replace, devops-docker-keyword-modify, devops-task-overview-template, devops-tekton-dynamic-form-optimizer, devops-tekton-operator-task-e2e, devops-tekton-task-generator, devops-tekton-task-version-upgrade, devops-tektoncd-vuln-fix, devops-upgrade-go

@yhuan123 yhuan123 merged commit 8fce15a into main Apr 1, 2026
7 checks passed
@yhuan123 yhuan123 deleted the fix/close-idle-connections branch April 1, 2026 02:37
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.

3 participants