Skip to content

Set pending_command flag consistently across all command execution paths#3600

Merged
JimB123 merged 2 commits into
valkey-io:forklessfrom
harrylin98:forkless_blocking_fix
May 27, 2026
Merged

Set pending_command flag consistently across all command execution paths#3600
JimB123 merged 2 commits into
valkey-io:forklessfrom
harrylin98:forkless_blocking_fix

Conversation

@harrylin98

@harrylin98 harrylin98 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

The pending_command flag is documented as indicating a client has a fully parsed command ready for execution. However, it was only set in the IO thread path, not in the main thread processInputBuffer path which parses and executes directly without ever setting the flag. This inconsistency meant blocking APIs like blockClientInUseOnKeys had to require callers to manually set pending_command = 1 before calling.

This change:

  • Sets pending_command = 1 in processInputBuffer when the command is fully parsed.
  • Clears it in commandProcessed after execution completes.
  • Removes redundant clears in processPendingCommandAndInputBuffer and unblockClientOnKey.

Now any command that blocks during processCommand naturally has pending_command = 1 already set, without caller intervention.

Exception:

  • WAIT and WAITAOF explicitly clear pending_command before blocking, since processClientsWaitingReplicas uses it to distinguish reply commands (pending_command = 0) from re-execution commands like CLUSTER SETSLOT (pending_command = 1).
  • Module clients clear pending_command in moduleHandleBlockedClients before unblocking, since their reply is already handled by the module's callback (reply callback for VM_BlockClient, key-notification callback for VM_BlockClientOnKeys). The exception is module auth-in-progress clients, which keep pending_command = 1 because they need re-execution after authentication completes.

@harrylin98 harrylin98 requested a review from JimB123 April 30, 2026 21:13
@harrylin98 harrylin98 added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Apr 30, 2026
@harrylin98 harrylin98 force-pushed the forkless_blocking_fix branch 2 times, most recently from 6191dae to e5de2c7 Compare May 1, 2026 03:58
@codecov

codecov Bot commented May 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.73%. Comparing base (c0f533c) to head (782ff6c).

Files with missing lines Patch % Lines
src/module.c 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           forkless    #3600      +/-   ##
============================================
+ Coverage     76.66%   76.73%   +0.06%     
============================================
  Files           159      159              
  Lines         79570    79575       +5     
============================================
+ Hits          61003    61058      +55     
+ Misses        18567    18517      -50     
Files with missing lines Coverage Δ
src/blocked.c 92.03% <100.00%> (-0.02%) ⬇️
src/db.c 94.36% <100.00%> (+<0.01%) ⬆️
src/networking.c 92.54% <100.00%> (+0.47%) ⬆️
src/replication.c 86.45% <100.00%> (+0.38%) ⬆️
src/module.c 26.30% <50.00%> (+<0.01%) ⬆️

... 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 force-pushed the forkless_blocking_fix branch 12 times, most recently from a83ba21 to 324fb13 Compare May 1, 2026 16:13
@harrylin98 harrylin98 requested a review from murphyjacob4 May 2, 2026 02:29
@harrylin98 harrylin98 force-pushed the forkless_blocking_fix branch from 324fb13 to 2c8d170 Compare May 4, 2026 15:39
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
@harrylin98 harrylin98 force-pushed the forkless_blocking_fix branch from 2c8d170 to a12a5b0 Compare May 4, 2026 15:39
@JimB123

JimB123 commented May 11, 2026

Copy link
Copy Markdown
Member

I think this is a good change to simplify the overall command processing logic. Since the pending_command flag means that a command is parsed and ready to execute, we should be setting it when the command is parsed, and clearing it when the command is executed. Currently, it seems to be maintained only for special cases.

Comment thread src/networking.c
Comment thread src/blocked.c Outdated
Comment thread src/blocked.c Outdated
Comment thread src/module.c Outdated
Comment thread src/replication.c
@coderabbitai

coderabbitai Bot commented May 19, 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: bfd5e2ea-29d5-4639-9253-56cb5e251610

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

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

@harrylin98 harrylin98 force-pushed the forkless_blocking_fix branch 2 times, most recently from a7d2819 to b241952 Compare May 19, 2026 20:44
…g command on module blocking clients

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
@harrylin98 harrylin98 force-pushed the forkless_blocking_fix branch from b241952 to 782ff6c Compare May 19, 2026 20:55
@JimB123 JimB123 merged commit 6b8bd41 into valkey-io:forkless May 27, 2026
27 checks passed
harrylin98 added a commit to harrylin98/valkey_forked that referenced this pull request Jun 23, 2026
…ths (valkey-io#3600)

The `pending_command` flag indicates that a client has a fully parsed command ready for execution. This update ensures that the flag is set/cleared consistently across different execution paths.

---------

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants