logpuller: clear first element from cache in subscription client

[wk989898]

What problem does this PR solve?

Issue Number: close #4143

What is changed and how it works?

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note.

If you don't think this PR needs a release note then fill it with `None`.

Summary by CodeRabbit

• Refactor
  • Internal refactoring of error handling infrastructure to improve performance and reliability.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign flowbehappy for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes convert regionErrorInfo from value types to pointer types across the error handling path in the logservice logpuller module. The constructor, method signatures, cache structures, and type channels are updated to consistently pass and store pointers to regionErrorInfo rather than values.

Changes

Cohort / File(s)	Summary
Constructor return type conversion logservice/logpuller/region_state.go	Constructor newRegionErrorInfo changed to return *regionErrorInfo instead of regionErrorInfo value type.
Error handling signatures and cache refactoring logservice/logpuller/subscription_client.go	Method signatures onRegionFail and doHandleError updated to accept *regionErrorInfo parameters. errCache structure changed to store []*regionErrorInfo in cache slice and use chan *regionErrorInfo for the error channel. Constructor and add method refactored to work with pointers.
Test updates logservice/logpuller/subscription_client_test.go	Updated errCache usage and mock regionErrorInfo creation to work with pointer types []*regionErrorInfo and *regionErrorInfo.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

lgtm, size/M

Suggested reviewers

• asddongmen
• lidezhu

Poem

🐰 A pointer's hop through the memory lane,
No values copy, no leaks remain,
The cache now holds what's light and free,
Each error tracked with clarity! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description is largely incomplete with minimal implementation details. While it identifies the linked issue (#4143), most required sections are unfilled placeholders.	Add detailed explanation of what changes were made to fix the memory leak, which compatibility concerns apply, and whether tests were added; provide a release note.
Out of Scope Changes check	❓ Inconclusive	The widespread refactoring from regionErrorInfo values to pointers across multiple files appears broader than the minimal description suggests, introducing structural changes beyond simple cache cleanup.	Clarify whether the value-to-pointer refactoring is necessary for the memory leak fix or if it represents additional scope creep; document the rationale for these changes.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly references the main change: clearing the first element from cache in subscription client, which matches the primary objective of addressing the memory leak.
Linked Issues check	✅ Passed	Changes refactor regionErrorInfo handling from values to pointers and implement proper cache cleanup via clear(), directly addressing the memory leak in errCache.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch wk989898-patch-3

---

No actionable comments were generated in the recent review. 🎉

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Summary of Changes

Hello @wk989898, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a potential memory leak within the logpuller component by enhancing the memory management of its internal error cache. By explicitly clearing references to elements that are removed from the cache, the change ensures that the garbage collector can promptly reclaim associated memory, leading to improved stability and resource utilization for the logpuller's subscription client.

Highlights

• Memory Management Improvement: The dispatch method within the errCache in the logpuller's subscription client now explicitly clears the first element of the cache slice before re-slicing. This ensures that the reference to the removed error information is nulled out, allowing the Go garbage collector to reclaim memory more efficiently and preventing potential memory leaks.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• logservice/logpuller/subscription_client.go
  • Added a call to clear(e.cache[:1]) in the dispatch method of the errCache to explicitly nil out the first element of the slice before it is re-sliced, preventing memory leaks by allowing the garbage collector to free up memory.

Activity

• No human activity (comments, reviews, etc.) has been recorded for this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly addresses a potential memory leak in the errCache by using clear() on the slice before reslicing, which is a solid improvement for memory management. No security vulnerabilities were identified in the modified code. Additionally, consider shrinking the cache slice when it becomes too sparse after a burst of errors to further enhance memory efficiency.

[gemini-code-assist]

While this change correctly prevents a memory leak from the removed element, the underlying array of e.cache will never shrink. If a large burst of errors causes the slice to grow significantly, it will hold onto that large memory allocation even after the errors are processed. This can be inefficient for a long-running service.

To further improve memory management, you could consider shrinking the slice when it becomes too sparse. For example, you could add logic like this after reslicing:

// initialErrCacheCapacity is the capacity set in newErrCache, which is 1024.
const initialErrCacheCapacity = 1024 
// Shrink if capacity is more than twice the length and well above initial capacity.
if len(e.cache) < cap(e.cache)/2 && cap(e.cache) > initialErrCacheCapacity*2 {
    newCache := make([]regionErrorInfo, len(e.cache))
    copy(newCache, e.cache)
    e.cache = newCache
}

This would create a new, smaller backing array and allow the old, larger one to be garbage collected.