Skip to content

Improve zombie client connection detection - #4465

Open
harrylin98 wants to merge 1 commit into
valkey-io:forklessfrom
harrylin98:forkless_update
Open

Improve zombie client connection detection#4465
harrylin98 wants to merge 1 commit into
valkey-io:forklessfrom
harrylin98:forkless_update

Conversation

@harrylin98

Copy link
Copy Markdown
Contributor

Preparatory cleanup extracted from the throttling PR: https://github.com/valkey-io/valkey/pull/4356/changes#diff-b95cee2ea24f3f4245c20ba624dd69c6b741fe7d63e64a29e451e4f09b536abeR426

  • Rename connSocketIsClosingconnTcpSocketIsClosing
  • Move the "has active handler" check from the connection layer up to clientsCronTcpIsClosing
  • Use offsetof-based length validation for getsockopt instead of requiring the full struct size
  • Run zombie detection earlier in clientsCron (skip buffer resize on dead clients)

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aeb4b31d-c898-484a-9b6e-cf92eec21980

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger 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

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

@harrylin98
harrylin98 requested a review from JimB123 August 18, 2026 21:58
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.40%. Comparing base (50ec936) to head (57ad97b).

Additional details and impacted files
@@             Coverage Diff              @@
##           forkless    #4465      +/-   ##
============================================
- Coverage     79.82%   79.40%   -0.43%     
============================================
  Files           175      175              
  Lines         94843    93223    -1620     
============================================
- Hits          75712    74021    -1691     
- Misses        19131    19202      +71     
Files with missing lines Coverage Δ
src/connection.h 88.67% <ø> (ø)
src/server.c 89.75% <100.00%> (+<0.01%) ⬆️
src/socket.c 94.68% <100.00%> (+0.53%) ⬆️
src/tls.c 17.64% <ø> (ø)

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@harrylin98
harrylin98 requested a review from nitaicaro August 19, 2026 17:18

@JimB123 JimB123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this isn't critical specifically for the forkless branch, I'd recommend keeping this with the throttling branch and resolve during the merge.

If you concur, please close the PR

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