feat(geo): add GEOSEARCHSTORE command

[rounaknandanwar]

Adds GEOSEARCHSTORE (dest + src keys, same search options as GEOSEARCH, optional STOREDIST).
Most of the work was already in GeoSearchStoreGeneric() via GEORADIUS ... STORE — this wires up the dedicated command and parser. Closes #3883.

Also fixed a couple of Redis mismatches in the shared store/search path:

• STORE when the source key is missing now returns 0 and clears the dest key (was an empty array)
• COUNT without ASC/DESC now defaults to ASC, same as Redis
  Tests in geo_family_test.cc. Ran geo_family_test locally + manual redis-cli checks.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

---

🟠 Medium

1. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View medium (15)

4. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

13. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

14. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.
### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.
### Fix Focus Areas
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

15. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.
### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.
### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]
### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
- In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

16. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]</...

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Add GEOSEARCHSTORE command with Redis-compatible store/count semantics

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

---

AI Description

• Add GEOSEARCHSTORE command with dedicated argument grammar and STOREDIST support.
• Make store-mode searches clear destination and return 0 when the source key is missing.
• Match Redis COUNT behavior by defaulting to ASC when COUNT is set without sorting.

Diagram

graph TD
  C["Client"] --> H["Cmd GEOSEARCHSTORE"] --> G["GeoSearchStoreGeneric"] --> S[("Source ZSET")]
  G --> D{"Store mode?"} --> Z["ZSetFamily ops"] --> T[("Dest ZSET")]
  D -->|"KEY_NOTFOUND"| E["Store empty"] --> T
  subgraph Legend
    direction LR
    _p["Process"] ~~~ _d{"Decision"} ~~~ _db[("ZSET key")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Implement GEOSEARCHSTORE as an alias to GEOSEARCH + STORE options

• ➕ Less new parsing surface area (reuse GEOSEARCH grammar directly)
• ➕ Potentially fewer code paths to maintain
• ➖ Harder to match Redis-specific syntax restrictions (e.g., disallowing WITH* options) cleanly
• ➖ More coupling to legacy STORE/GEORADIUS option parsing semantics

2. Extend the existing GEOSEARCH grammar with a 'store-only mode' toggle

• ➕ Single grammar definition reduces duplication between GEOSEARCH and GEOSEARCHSTORE
• ➕ Makes it easier to keep option sets aligned over time
• ➖ Grammar becomes more complex/conditional, increasing maintenance risk
• ➖ Higher chance of accidental acceptance of invalid option combinations

Recommendation: Current approach (dedicated GEOSEARCHSTORE grammar + reusing GeoSearchStoreGeneric for execution) is the best balance: it keeps execution logic centralized while allowing GEOSEARCHSTORE to enforce Redis-compatible syntax and semantics without contorting the existing GEOSEARCH parser.

Files changed (2) +144 / -24

Enhancement (1) +79 / -22

geo_family.ccAdd GEOSEARCHSTORE command, store helpers, and Redis-compatible COUNT/store behavior +79/-22

Add GEOSEARCHSTORE command, store helpers, and Redis-compatible COUNT/store behavior

• Introduces GEOSEARCHSTORE parsing (including STOREDIST) and registers the new command. Extracts common helpers for store-mode detection, destination overwrites, and COUNT finalization (default ASC when COUNT is set without sorting). Fixes store-mode behavior to return 0 and clear the destination key when the source key is missing.

src/server/geo_family.cc

Tests (1) +65 / -2

geo_family_test.ccAdd GEOSEARCHSTORE coverage and adjust geo store/sort expectations +65/-2

Add GEOSEARCHSTORE coverage and adjust geo store/sort expectations

• Adds a new GeoSearchStore test suite covering basic store, STOREDIST, missing source key semantics (dest cleared), and invalid syntax/COUNT handling. Updates existing tests to reflect Redis-compatible behavior changes (STORE on missing key returns 0; COUNT default sorting affects result order).

src/server/geo_family_test.cc

[augmentcode]

🤖 Augment PR Summary

Summary: Adds Redis-compatible GEOSEARCHSTORE to store GEOSEARCH results into a destination zset.

Changes:

• Introduced a dedicated argument grammar and command handler for GEOSEARCHSTORE (supports the same search options as GEOSEARCH plus optional STOREDIST).
• Refactored store logic into helpers to write/clear the destination key and reuse the existing shared search/store implementation.
• Aligned STORE behavior with Redis when the source key is missing: returns 0 and clears the destination key.
• Adjusted COUNT behavior to default to ascending distance order when COUNT is provided without ASC/DESC (matching Redis).
• Registered the new command as journaled and OOM-denying.
• Added comprehensive unit tests for GEOSEARCHSTORE and updated existing GEORADIUS/GEORADIUSBYMEMBER expectations.

Technical Notes: The implementation reuses GeoSearchStoreGeneric and stores either geohash scores or computed distances depending on STOREDIST.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

src/server/geo_family.cc:500 — ZSetFamily::OpAdd’s result is ignored and store_cb always returns OpStatus::OK, so an OUT_OF_MEMORY/other write failure could still reply with smvec.size() and leave dest_key not updated. That would be a silent correctness issue for the GEO* STORE paths.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 12 rules

✅ Cross-repo context

  Not relevant to this PR: romange/helio

---

🔴 Action Required

1. Broken cross-shard journaling ✓ Resolved 🐞 Bug ☼ Reliability

Description

GEOSEARCHSTORE is registered without CO::NO_AUTOJOURNAL, so when src/dest hash to different shards,
auto-journaling emits shard-local arguments derived from the key index only (keys), omitting
mandatory non-key search options. This produces journal/AOF entries that can’t be replayed correctly
(likely syntax error or wrong behavior) on replicas/recovery.

Code

src/server/geo_family.cc[833]

+            << CI{"GEOSEARCHSTORE", CO::JOURNALED | CO::DENYOOM, -8, 1, 2}.HFUNC(GeoSearchStore)

Relevance

●●● Strong

Journaling/replication correctness issues are commonly fixed; command option/journaling fixes
accepted before (PR #6731, #6492).

PR-#6731
PR-#6492

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Auto-journaling switches to per-shard ShardArgs when a transaction spans multiple shards. For
GEOSEARCHSTORE, the transaction key index covers only dest/src key positions, so ShardArgs
contains only those key arguments; the journal serializer writes only iterated ShardArgs elements,
dropping required non-key options needed to replay the command.

src/server/geo_family.cc[826-835]
src/server/transaction.cc[1815-1854]
src/server/transaction.cc[333-344]
src/server/transaction.cc[1613-1646]
src/server/journal/serializer.cc[37-55]
src/server/tx_base.h[129-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GEOSEARCHSTORE` is registered as `CO::JOURNALED` without `CO::NO_AUTOJOURNAL`. For multi-shard executions (src/dest on different shards), the auto-journal path serializes *only shard-local key slices*, which excludes the required non-key search arguments (FROM*/BY* and modifiers). This makes replication/AOF replay invalid or semantically incorrect.

### Issue Context
- `DetermineKeys()` computes key slices only for key positions (not the rest of the arguments).
- When `unique_shard_cnt_ > 1`, auto-journaling uses `ShardArgs` (sliced args), and the serializer writes only those sliced args.

### Fix Focus Areas
- src/server/geo_family.cc[830-835]
- src/server/geo_family.cc[493-506]

### Suggested fix approach
1. Register `GEOSEARCHSTORE` with `CO::NO_AUTOJOURNAL` (similar to other multi-key commands that can’t be replayed from shard-local key slices).
2. Explicitly journal the actual destination mutations instead:
  - In `GeoStoreToDest`, call `ZSetFamily::OpAdd` with `ZParams{.override=true, .journal_update=true}` so it records `DEL` + `ZADD` (and `DEL` on empty override).
3. Ensure OpAdd status is propagated/checked (see separate finding) so journaling isn’t emitted on failed writes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

🟡 Remediation Recommended

2. OpAdd errors ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

GeoStoreToDest drops the OpResult from ZSetFamily::OpAdd and always returns OpStatus::OK from the
transaction callback, then replies with smvec.size(). If OpAdd fails (e.g., OUT_OF_MEMORY), the
transaction can still conclude/journal as success and the client receives a positive stored count
despite the destination update failing.

Code

src/server/geo_family.cc[R499-502]

+      zparams.override = true;
+      ZSetFamily::OpAdd(t->GetOpArgs(shard), zparams, dest_key, ScoredMemberSpan{smvec});
+    }
+    return OpStatus::OK;

Relevance

●●● Strong

Team often accepts propagating non-OK OpStatus/OpResult instead of silent success (e.g., PR #6950).

PR-#6950

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper ignores OpAdd’s OpResult and always returns OK, preventing Transaction’s multi-shard
callback machinery from detecting OOM/failure. A similar store path (ZRANGESTORE) captures the add
result and explicitly errors on OOM, demonstrating the intended pattern.

src/server/geo_family.cc[493-506]
src/server/transaction.cc[682-697]
src/server/zset_family.cc[1754-1779]
PR-#6950

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GeoStoreToDest()` calls `ZSetFamily::OpAdd(...)` but ignores its returned status and unconditionally returns `OpStatus::OK` from the shard callback, then sends `smvec.size()` to the client. This can hide destination write failures (notably OOM) and report success with no/partial write.

### Issue Context
For multi-shard callbacks, `Transaction::RunCallback` only records OOM when the callback returns `OpStatus::OUT_OF_MEMORY`; otherwise it expects OK. Ignoring `OpAdd`’s status prevents failures from propagating.

### Fix Focus Areas
- src/server/geo_family.cc[493-506]

### Suggested fix approach
1. Capture the `OpResult<ZSetFamily::AddResult>` from `OpAdd` inside the destination-shard callback.
2. Return the captured status from the callback on the destination shard (return OK on other shards).
3. After `tx->Execute(...)`, check the captured result and send an error on failure (at minimum handle `OUT_OF_MEMORY`, and preferably propagate other non-OK statuses similarly).
4. Only `SendLong(smvec.size())` when the add/delete succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ℹ️ Informational

3. CmdGeoSearchStore lacks doc comment 📘 Rule violation ⚙ Maintainability

Description

The newly added CmdGeoSearchStore handler is a user-visible/public command entrypoint but has no
purpose/documentation comment directly above its declaration. This violates the requirement to
document public API methods with a purpose comment.

Code

src/server/geo_family.cc[R722-725]

+void CmdGeoSearchStore(CmdArgParser parser, CommandContext* cmd_cntx) {
+  auto* builder = cmd_cntx->rb();
+
+  string_view dest = parser.Next();

Relevance

●● Moderate

No clear historical evidence that Cmd* command handlers require purpose doc comments; mostly
optional comment nits.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1535206 requires a purpose comment above public API methods/functions. The diff
adds the new command handler CmdGeoSearchStore without any documentation comment preceding it.

Rule 1535206: Document public API methods with purpose comments
src/server/geo_family.cc[722-748]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CmdGeoSearchStore` is a new public/user-visible command handler but lacks a documentation comment describing its purpose.

## Issue Context
The compliance checklist requires public API methods/functions in changed files to have a purpose comment immediately above their declaration.

## Fix Focus Areas
- src/server/geo_family.cc[722-748]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Powered by Qodo

[vyavdoshenko]

@rounaknandanwar Thanks for your contribution.

Unfortunately, this is not enough. You have to add/update fuzzer seeds. Please take a look at an example here: https://github.com/dragonflydb/dragonfly/tree/main/fuzz/seeds/resp

[rounaknandanwar]

@vyavdoshenko I have fixed it with the last commit.

[vyavdoshenko]

Must fix:

---

1. Replica score corruption (broken replication for GEOSEARCHSTORE)
   The manual journal path serializes ZADD scores via absl::StrCat (6 significant digits). Since GEOSEARCHSTORE is NO_AUTOJOURNAL, these lossy entries are its only replication mechanism.

master> GEOSEARCHSTORE dst src FROMLONLAT 15 37 BYRADIUS 500 km STOREDIST
master>  ZSCORE dst Catania   -> 56.4412578701582
replica> ZSCORE dst Catania   -> 56.4413
# default mode: geohash 3479099956230698 -> 3479100000000000 (corrupted coordinates)

Fix: round-trip double formatting in the OpAdd journal_update path (also fixes the same latent bug in ZRANGESTORE/ZDIFFSTORE). Add a replication test asserting exact ZSCORE equality.

---

2. GEORADIUS / GEORADIUSBYMEMBER STORE now double-journal
   They share GeoStoreToDest (which now sets journal_update=true) but are still auto-journaled.

master> GEORADIUS src 15 37 200 km STORE dst
replica INFO commandstats -> cmdstat_del:1, cmdstat_zadd:1, cmdstat_georadius:1
# replica applies the store twice: manual DEL+ZADD, then the replayed command

Fix: add CO::NO_AUTOJOURNAL to both radius commands (also fixes #7996) - but only together with fix 1, otherwise the cross-shard crash becomes silent score corruption. Alternatively, keep journal_update=false on the radius path in this PR.

---

3. New fuzz seeds are malformed RESP

• geo_ops2.resp: $4 for 5-byte keys gdst2/gdst3 (must be $5); *9 for the 8-element ... FROMMEMBER Palermo ... command (must be *8).
• georadius_ops.resp: *10 for the 9-element ... FROMMEMBER catania ... ASC command (must be *9) - this swallows the next *12 header and desyncs every seed after it, so the intended GEOSEARCHSTORE coverage never executes.

Should fix:

• Validate BYRADIUS/BYBOX at parse time. GEOSEARCH src FROMLONLAT 15 37 BYRADIUS -5 km never replies and pins a proactor thread at 100% CPU (pre-existing, but this PR's new fuzz tokens make the fuzzer hit it). Negative BYBOX in store mode deletes dest and returns 0 instead of erroring. Reference errors: "radius cannot be negative" / "height or width cannot be negative".
• Sign the commits - all 4 are unsigned (DCO only); merge is blocked on it.
• Drop the comment above CmdGeoSearchStore - contains a non-ASCII em-dash and a competitor product reference.
• Test coverage: DESC, COUNT n ANY, BYBOX+STOREDIST, FROMMEMBER+STOREDIST, dest==src, dest TTL cleared, empty result with existing src (case-3 empty path); assert the exact count/members in the europe_box block instead of EXPECT_GE(ZCARD, 1) and remove the dead resp assignment.

[vyavdoshenko]

@rounaknandanwar
Please follow the instructions to sign the commits: https://github.com/dragonflydb/dragonfly/blob/main/CONTRIBUTING.md

[rounaknandanwar]

Must fix:

1. Replica score corruption (broken replication for GEOSEARCHSTORE)
   The manual journal path serializes ZADD scores via absl::StrCat (6 significant digits). Since GEOSEARCHSTORE is NO_AUTOJOURNAL, these lossy entries are its only replication mechanism.

master> GEOSEARCHSTORE dst src FROMLONLAT 15 37 BYRADIUS 500 km STOREDIST
master>  ZSCORE dst Catania   -> 56.4412578701582
replica> ZSCORE dst Catania   -> 56.4413
# default mode: geohash 3479099956230698 -> 3479100000000000 (corrupted coordinates)

Fix: round-trip double formatting in the OpAdd journal_update path (also fixes the same latent bug in ZRANGESTORE/ZDIFFSTORE). Add a replication test asserting exact ZSCORE equality.

2. GEORADIUS / GEORADIUSBYMEMBER STORE now double-journal
   They share GeoStoreToDest (which now sets journal_update=true) but are still auto-journaled.

master> GEORADIUS src 15 37 200 km STORE dst
replica INFO commandstats -> cmdstat_del:1, cmdstat_zadd:1, cmdstat_georadius:1
# replica applies the store twice: manual DEL+ZADD, then the replayed command

Fix: add CO::NO_AUTOJOURNAL to both radius commands (also fixes #7996) - but only together with fix 1, otherwise the cross-shard crash becomes silent score corruption. Alternatively, keep journal_update=false on the radius path in this PR.

3. New fuzz seeds are malformed RESP

• geo_ops2.resp: $4 for 5-byte keys gdst2/gdst3 (must be $5); *9 for the 8-element ... FROMMEMBER Palermo ... command (must be *8).
• georadius_ops.resp: *10 for the 9-element ... FROMMEMBER catania ... ASC command (must be *9) - this swallows the next *12 header and desyncs every seed after it, so the intended GEOSEARCHSTORE coverage never executes.

Should fix:

• Validate BYRADIUS/BYBOX at parse time. GEOSEARCH src FROMLONLAT 15 37 BYRADIUS -5 km never replies and pins a proactor thread at 100% CPU (pre-existing, but this PR's new fuzz tokens make the fuzzer hit it). Negative BYBOX in store mode deletes dest and returns 0 instead of erroring. Reference errors: "radius cannot be negative" / "height or width cannot be negative".
• Sign the commits - all 4 are unsigned (DCO only); merge is blocked on it.
• Drop the comment above CmdGeoSearchStore - contains a non-ASCII em-dash and a competitor product reference.
• Test coverage: DESC, COUNT n ANY, BYBOX+STOREDIST, FROMMEMBER+STOREDIST, dest==src, dest TTL cleared, empty result with existing src (case-3 empty path); assert the exact count/members in the europe_box block instead of EXPECT_GE(ZCARD, 1) and remove the dead resp assignment.

@vyavdoshenko I have fixed all 3 in the last commit.

[vyavdoshenko]

@rounaknandanwar
Please resolve merge conflicts.

[vyavdoshenko]

@rounaknandanwar
Is it ready for review?

[rounaknandanwar]

@rounaknandanwar Is it ready for review?

@vyavdoshenko Yes its ready to review

[vyavdoshenko]

Require READ permission for the source key
Declaring key positions 1..2 is correct for routing, but because this command is marked JOURNALED, the ACL validator treats both keys as write keys. The second key is read-only.
A user with +GEOSEARCHSTORE +ZRANGE %W~secret %RW~out can copy secret into out and read it without having %R~secret, which is a confidentiality bypass.
Please add per-key ACL metadata or explicitly require READ permission for src and WRITE permission for dest.

[rounaknandanwar]

Added the ACL fix for GEOSEARCHSTORE: dest requires WRITE, src requires READ (validator.cc + tests).

For other commands, RequiredKeyPermissions() still uses the old default (all keys read on READONLY, all keys write on JOURNALED), so behaviour is unchanged except GEOSEARCHSTORE. The same read+write gap still exists on COPY/RENAME/*STORE etc.; I have only special-cased this command here.

I personally think a better long-term approach is per-key ACL on CommandId at registration, since this one needs special handling for all the read+write commands - which can become a mess in future. Let me know what you think.
Happy to do a follow-up for the broader mixed-key ACL work if you want it tracked separately (and keep the scope of this PR limited to geosearchstore implementation).

[vyavdoshenko]

Reject negative radius and box dimensions before executing the search
These parsers accept negative dimensions. With an existing source, BYRADIUS -1 m reaches geohashEstimateStepsByRadius() and loops forever because the negative value remains below MERCATOR_MAX while being doubled.
A negative BYBOX produces no matches, so GEOSEARCHSTORE deletes an existing destination and returns 0 instead of reporting an error.
Please validate the dimensions in the shared parsers and add regression tests.

[rounaknandanwar]

I have added the checks and tests to validate those.