feat(php): implement BGSAVE, BGSAVE SCHEDULE, and BGSAVE CANCEL commands

[prateek-kumar-improving]

Summary

Implement BGSAVE, BGSAVE SCHEDULE, and BGSAVE CANCEL commands for both standalone and cluster clients, matching PHPRedis API and the implementation in other GLIDE clients (Go, Python,
Node, C#).

Issue link

This Pull Request is linked to issue (URL): #236
Related issues and pull requests:

• [2.5] Implement New Missing Commands [2.5] Implement New Missing Commands valkey-glide#6010
• feat: Add BGSAVE commands feat: Add BGSAVE commands valkey-glide-csharp#436

Features / Behaviour Changes

• Add ValkeyGlide::bgSave(?string $mode = null): bool|string for standalone client
• Add ValkeyGlideCluster::bgSave(mixed $route, ?string $mode = null): bool|string for cluster client
• PHPRedis compatible: Returns true on success, false on failure by default
• With OPT_REPLY_LITERAL enabled, returns the actual status string (e.g., "Background saving started")
• Supports three modes:
  • null (default): Initiates a background save immediately
  • "SCHEDULE": Schedules a background save
  • "CANCEL": Aborts in-progress/scheduled saves (Valkey 8.1+)

Implementation

• execute_bgsave_command() added in valkey_glide_commands.c
• BGSAVE_METHOD_IMPL macro added in valkey_glide_commands_common.h
• Macro invoked for ValkeyGlide in valkey_z_php_methods.c
• Macro invoked for ValkeyGlideCluster in valkey_glide_cluster.c
• BgSave case added to prepare_message_args switch in valkey_glide_core_common.c
• Added Map response type handling in process_core_string_result for cluster multi-node responses
• Stub definitions added to valkey_glide.stub.php and valkey_glide_cluster.stub.php

Testing

• 3 standalone tests: testBgSave, testBgSaveSchedule, testBgSaveCancel
• 3 cluster tests with route parameter
• BGSAVE CANCEL tests version-gated to Valkey 8.1+
• waitForSaveNotInProgress() helper prevents test conflicts from concurrent saves
• Verified OPT_REPLY_LITERAL returns actual string response

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message has a detailed description of what changed and why.
• Tests are added or updated.
• CHANGELOG.md and documentation files are updated.
• Destination branch is correct - main or release
• Create merge commit if merging release branch into main, squash otherwise.

[currantw]

Reviewed to 215e710.

[currantw]

Does this match the behaviour of PHPRedis – does it also only take the first value (even though it is entirely possible that different nodes may return different values)?

[prateek-kumar-improving]

Yes, this matches PHPRedis behavior. PHPRedis's bgSave() returns a single bool or string — it doesn't return an array of per-node results.

[currantw]

Did you confirm (either by running it, from the documentation, or from the code) that the value returned by PHPRedis is simply the value from one of the nodes? Can you explain how you verified this?

[prateek-kumar-improving]

Verified from PHPRedis documentation (cluster.md). BGSAVE is listed as a "Directed node command" — in RedisCluster, it's sent to a single node (specified by the caller) and returns that node's scalar response.

PHPRedis does not aggregate multi-node results. Our allPrimaries route is a GLIDE-specific extension; taking the first value maintains the scalar return type that PHPRedis users expect.

When a user passes a key or host:port, they get a single-node response (just like PHPRedis). The "take first value from Map" logic only exists for multi-node routes (allPrimaries, allNodes).
PHPRedis doesn't have an equivalent to allPrimaries — it's a GLIDE addition.

[currantw]

In that case, should GLIDE return return a dictionary of strings (map from address to response string) for multi-node routes? If multi-node routes are not supported by PHPRedis at all, then this would be an extension of the PHPRedis API, rather than a change to it (i.e. the result would still be compatible for PHPRedis-support commands), and it would be consistent with the other GLIDE clients (which return a dictionary/ClusterValue for multi-node routes). If we do not return dictionary for multi-node commands, than we hiding responses from the user and mis-representing the actual responses from the server.

[prateek-kumar-improving]

Extended the implementation by adding an array return type: 646893a, 5d20fcb

[currantw]

Reviewed to 6124361.

[currantw]

Reviewed to 9f9d976.

[Aryex]

Why is this only looking at the first array_value? Should we also process the other responses as well?

[prateek-kumar-improving]

PHPRedis bgSave() returns a single scalar — it doesn't aggregate multi-node results. In PHPRedis cluster, BGSAVE is a "directed node command" sent to one specific node. Our allPrimaries route is a GLIDE-specific extension; we take the first value to maintain the scalar return type.

[Aryex]

Would this correctly handles bgSave("allPrimaries")?

[currantw]

I think that we may need to return a dictionary/ClusterValue for multi-node routes, as suggested in this comment: #237 (comment). This is what other GLIDE clients do, and I believe that it is still compatible with PHPRedis behaviour.

[prateek-kumar-improving]

Extended the implementation by adding an array return type: 646893a, 5d20fcb

[currantw]

Thanks! I have reviewed these changes and added a few more suggestions, but generally looks good – I think this is a much improved interface that more closely matches other GLIDE clients.

[currantw]

Reviewed to 4302cf1.

[currantw]

Reviewed to a657785.

[currantw]

The differences in return type and behaviour between standalone and cluster are not documented here.

[prateek-kumar-improving]

Updated in this commit: 7a122c9

[currantw]

Why does BGSAVE CANCEL not return an array like the other methods? This command could still succeed on some nodes and fail on others, right?

For other clients, failure causes an exception to be thrown, so we are not losing information if we return "OK"; but in this case, where – as I understand it – failure would NOT cause an exception, we are losing information if we don't return a bool. Moreover, it is unclear what this bool actually means. Does true mean that ALL nodes returned true? Or just one? Or most of them?

[prateek-kumar-improving]

Added a comment for this: d2d1991.

It's a limitation of the glide-core's response aggregation, not a PHP-level design choice. For BGSAVE (which has no explicit ResponsePolicy in redis-rs), multi-node routing uses the None policy path. Before building the Value::Map of per-node results, the core iterates all resolved values and returns the first ServerError as a single Err if any node errors. When CANCEL is called with no save in progress, ALL nodes return errors, so the core collapses them into a single command_error — we never receive a Map response on the PHP side.

This means:

• Success case (save was in progress): at least one node returns OK → no error short-circuit → we get the Map → PHP returns array<string, bool>
• All-failure case (no save in progress): all nodes error → core returns single Err → PHP returns scalar false

This matches PHPRedis semantics (returns false on failure) and is consistent with how the Python GLIDE client handles this — Python would throw a RequestError in the same scenario rather than returning a dict.

Changing this would require modifying the core's ResponsePolicy for BGSAVE (e.g., to Special) so per-node errors are preserved in the Map, which would be a separate core-level change. I've added an explanatory comment in the test.

[currantw]

Should this be process_core_bgsave_string_result instead?

Suggested change

	* - With OPT_REPLY_LITERAL: return raw string (process_core_string_result)
	* - With OPT_REPLY_LITERAL: return raw string (process_core_bgsave_string_result)

[prateek-kumar-improving]

Updated in this commit: 83da041

[currantw]

Nit. Can simplify this. Reduces return paths/branching and nesting:

Suggested change

	if (execute_core_command(valkey_glide, &core_args, NULL, processor, return_value)) {
	if (valkey_glide->is_in_batch_mode) {
	/* In batch mode, return $this for method chaining */
	ZVAL_COPY(return_value, object);
	return 1;
	}
	return 1;
	} else {
	return 0;
	}
	if (!execute_core_command(valkey_glide, &core_args, NULL, processor, return_value)) {
	return 0;
	}
	if (valkey_glide->is_in_batch_mode) {
	/* In batch mode, return $this for method chaining */
	ZVAL_COPY(return_value, object);
	}
	return 1;

[prateek-kumar-improving]

Updated in this commit: 83da041

[currantw]

I think we can significantly simplify this and reduce duplication by implementing something like process_core_cluster_result, which would take a converter function and apply it to a single- or multi-node cluster result. This would be similar to logic that exists in other clients, and would avoid duplication for new commands.

Here is an AI generated example of what this could look like:

  int process_core_cluster_result(
      CommandResponse* response,
      void* output,
      zval* return_value,
      z_result_processor_t single_node_processor
  ) {
      if (!response) {
          ZVAL_FALSE(return_value);
          return 0;
      }

      if (response->response_type == Map && response->array_value &&
          response->array_value_len > 0) {
          array_init(return_value);
          for (long i = 0; i < response->array_value_len; i++) {
              CommandResponse* element = &response->array_value[i];
              CommandResponse* value   = element->map_value;
              if (element->map_key && element->map_key->string_value && value) {
                  zval node_result;
                  single_node_processor(value, output, &node_result);
                  add_assoc_zval_ex(return_value,
                                    element->map_key->string_value,
                                    element->map_key->string_value_len,
                                    &node_result);
              }
          }
          return 1;
      }

      // Single-node: delegate directly
      return single_node_processor(response, output, return_value);
  }

  int process_core_cluster_bool_result(CommandResponse* r, void* o, zval* rv) {
      return process_core_cluster_result(r, o, rv, process_core_bool_result);
  }

  int process_core_cluster_string_result(CommandResponse* r, void* o, zval* rv) {
      return process_core_cluster_result(r, o, rv, process_core_string_result);

This is much simpler, and has the advantage of being more generic and reusable.

[prateek-kumar-improving]

Refactored here: d370bcd