Skip to content

Commit cff1db2

Browse files
committed
fix(dist-memory): broaden set-forward promotion to any transport error
Previously, `handleForwardPrimary` only promoted to a replica owner when the forward error matched the in-process transport's `ErrBackendNotFound` sentinel. HTTP/gRPC transports against a stopped node surface `net.OpError`, `io.EOF`, or `context.DeadlineExceeded` instead — causing writes to fail silently for keys whose primary had just been killed, rather than falling through to a replica. Promotion now triggers on any non-nil forward error when the local node is listed in `owners[1:]` and still owns the key locally (defensive against a stale ring snapshot). Spurious promotion on a transient blip is benign: `applySet` version-compares on the receiver and `chooseNewer` / merkle anti-entropy reconcile divergent `(version, origin)` pairs via the existing LWW rule. Add `TestDistSet_PromotesOnGenericForwardError` which uses chaos hooks at `DropRate=1.0` to deterministically force a generic forward error and asserts the Set succeeds via promotion; `TestDistFailureRecovery` continues to pass unchanged (the change widens the gate, doesn't narrow it). Also add a `check_command_exists` Makefile macro and CHANGELOG entry.
1 parent b7d3449 commit cff1db2

4 files changed

Lines changed: 129 additions & 17 deletions

File tree

CHANGELOG.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,23 @@ All notable changes to HyperCache are recorded here. The format follows
286286

287287
### Fixed
288288

289+
- **Set-forward promotion no longer requires the in-process `ErrBackendNotFound` sentinel.**
290+
[`handleForwardPrimary`](pkg/backend/dist_memory.go) used to gate "primary unreachable → promote to
291+
replica" on `errors.Is(errFwd, sentinel.ErrBackendNotFound)`, the error the in-process transport returns
292+
for an unregistered peer. HTTP/gRPC transports against a stopped container surface
293+
`net.OpError` / `io.EOF` / `context.DeadlineExceeded` instead — none of which matched the condition.
294+
Result: when a cluster node was killed (e.g. `docker stop` in
295+
[`scripts/tests/20-test-cluster-resilience.sh`](scripts/tests/20-test-cluster-resilience.sh)), writes for
296+
keys whose primary was the dead node failed immediately at the forwarding hop, no hint was queued, and
297+
the data never landed anywhere — the same 7 of 50 "during-*" writes failed reproducibly in CI's cluster
298+
workflow. Promotion now triggers on **any** non-nil forward error when the local node is in `owners[1:]`,
299+
matching the in-process and production transport behavior under the same resilience contract. Spurious
300+
promotion on a transient blip is benign — `applySet` version-compares on the receiver, and merkle
301+
anti-entropy / `chooseNewer` reconcile any divergent `(version, origin)` pair via the existing
302+
last-write-wins rule. New test [`TestDistSet_PromotesOnGenericForwardError`](tests/hypercache_distmemory_forward_primary_promotion_test.go)
303+
uses the chaos hooks at `DropRate=1.0` to deterministically force a generic forward error and asserts the
304+
Set succeeds via promotion; the existing `TestDistFailureRecovery` continues to pass byte-identical (the
305+
change widens the promotion gate, doesn't narrow it).
289306
- **`TestDistRebalanceReplicaDiffThrottle` no longer flakes under `make test-race`.** The test's 900ms hard
290307
sleep wasn't enough wall-clock budget for the rebalancer's 80ms-tick loop to actually fire 11 ticks under
291308
`-race` + `-shuffle=on`'s scheduler pressure. Replaced the sleep with a 5-second polling loop that exits as

Makefile

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ run-example:
9999
update-deps:
100100
go get -u -t ./... && go mod tidy -v && go mod verify
101101

102+
# check_command_exists expands to a recipe line that succeeds if the
103+
# given command resolves on PATH, otherwise prints "<cmd> command not
104+
# found" and exits non-zero. Used by prepare-base-tools' chain of
105+
# `$(call check_command_exists,X) || install-fallback` lines: the
106+
# first failure message above is the developer-facing hint; the
107+
# install fallback fires when the tool itself is genuinely missing.
108+
define check_command_exists
109+
@which $(1) > /dev/null 2>&1 || (echo "$(1) command not found" && exit 1)
110+
endef
111+
102112
prepare-toolchain: prepare-base-tools
103113

104114
prepare-base-tools:

pkg/backend/dist_memory.go

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3622,7 +3622,21 @@ func (dm *DistMemory) repairRemoteReplica(
36223622
dm.metrics.readRepair.Add(1)
36233623
}
36243624

3625-
// handleForwardPrimary tries to forward a Set to the primary; returns (proceedAsPrimary,false) if promotion required.
3625+
// handleForwardPrimary tries to forward a Set to the primary; returns
3626+
// (proceedAsPrimary, err). On any non-nil forward error — not just
3627+
// ErrBackendNotFound — the function attempts to promote to a replica
3628+
// owner if the local node is in the replica list. This is the correct
3629+
// resilience contract: the in-process transport surfaces unreachable
3630+
// peers as ErrBackendNotFound, but HTTP/gRPC transports against a
3631+
// stopped container surface net.OpError / io.EOF /
3632+
// context.DeadlineExceeded. Gating promotion on the in-process-only
3633+
// sentinel meant production forwarding failures dropped writes to keys
3634+
// whose primary had just been killed, until the next merkle tick.
3635+
//
3636+
// A spurious promotion (primary was healthy but the call hit a
3637+
// transient blip) is benign: applySet on the receiver version-compares,
3638+
// and `chooseNewer` / merkle anti-entropy reconcile any divergent
3639+
// `(version, origin)` pair through the existing last-write-wins rule.
36263640
func (dm *DistMemory) handleForwardPrimary(ctx context.Context, owners []cluster.NodeID, item *cache.Item) (bool, error) {
36273641
transport := dm.loadTransport()
36283642
if transport == nil {
@@ -3632,26 +3646,23 @@ func (dm *DistMemory) handleForwardPrimary(ctx context.Context, owners []cluster
36323646
dm.metrics.forwardSet.Add(1)
36333647

36343648
errFwd := transport.ForwardSet(ctx, string(owners[0]), item, true)
3635-
switch {
3636-
case errFwd == nil:
3637-
return false, nil // forwarded successfully
3638-
case errors.Is(errFwd, sentinel.ErrBackendNotFound) && len(owners) > 1:
3639-
// primary missing: promote if this node is a listed replica
3640-
for _, oid := range owners[1:] {
3641-
if oid == dm.localNode.ID { // we can promote
3642-
if !dm.ownsKeyInternal(item.Key) { // still not recognized locally (ring maybe outdated)
3643-
return false, errFwd
3644-
}
3649+
if errFwd == nil {
3650+
return false, nil
3651+
}
36453652

3646-
return true, nil // proceed as primary path
3653+
// Primary unreachable for any reason — try to promote to a replica
3654+
// owner. The local node must be in owners[1:] AND still recognize
3655+
// itself as an owner (defensive against a stale ring snapshot
3656+
// mid-membership-change).
3657+
if len(owners) > 1 {
3658+
for _, oid := range owners[1:] {
3659+
if oid == dm.localNode.ID && dm.ownsKeyInternal(item.Key) {
3660+
return true, nil
36473661
}
36483662
}
3649-
3650-
return false, errFwd // not promotable
3651-
3652-
default:
3653-
return false, errFwd
36543663
}
3664+
3665+
return false, errFwd
36553666
}
36563667

36573668
// initStandaloneMembership initializes membership & ring for standalone mode with optional seeds.
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
package tests
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/hyp3rd/hypercache/internal/cluster"
8+
"github.com/hyp3rd/hypercache/pkg/backend"
9+
cache "github.com/hyp3rd/hypercache/pkg/cache/v2"
10+
)
11+
12+
// TestDistSet_PromotesOnGenericForwardError pins the resilience
13+
// contract that `handleForwardPrimary` must promote to a replica
14+
// owner on ANY non-nil forward error, not just the in-process
15+
// transport's `ErrBackendNotFound` sentinel. Production HTTP/gRPC
16+
// transports against a stopped container surface `net.OpError`,
17+
// `io.EOF`, or `context.DeadlineExceeded` — none of which match
18+
// the pre-fix promotion condition.
19+
//
20+
// Pre-fix behavior on this test: Set returns ErrChaosDrop (the
21+
// forward fails, the default switch arm returns the error
22+
// verbatim). Post-fix: Set returns nil and the key lands on the
23+
// promoting node's local shard.
24+
//
25+
// Uses chaos hooks at DropRate=1.0 to deterministically force every
26+
// outbound forward call to return ErrChaosDrop. Combined with
27+
// ConsistencyOne writes so the local primary path satisfies quorum
28+
// without needing any successful replica fan-out (chaos breaks the
29+
// replica calls too — that's covered by TestDistChaos_*
30+
// in tests/integration/, not what this test pins).
31+
func TestDistSet_PromotesOnGenericForwardError(t *testing.T) {
32+
t.Parallel()
33+
34+
chaos := backend.NewChaos()
35+
36+
// 3 nodes, RF=3, ConsistencyOne. Chaos is wired onto every
37+
// node's transport; we only ever Set from one node, so only
38+
// that node's forwards exercise the promotion path.
39+
dc := SetupInProcessClusterRF(
40+
t, 3, 3,
41+
backend.WithDistChaos(chaos),
42+
backend.WithDistWriteConsistency(backend.ConsistencyOne),
43+
)
44+
45+
a := dc.Nodes[0]
46+
b := dc.Nodes[1]
47+
c := dc.Nodes[2]
48+
49+
// Pick a key whose primary is `b` and where `a` is a replica
50+
// owner. From `a`, the Set will forward to `b`; chaos drops
51+
// that call; promotion should fire because `a` is in
52+
// owners[1:].
53+
desired := []cluster.NodeID{b.LocalNodeID(), a.LocalNodeID(), c.LocalNodeID()}
54+
55+
key, ok := FindOwnerKey(a, "promote-on-net-err-", desired, 5000)
56+
if !ok {
57+
t.Fatalf("could not find key with owner ordering [B, A, C]")
58+
}
59+
60+
chaos.SetDropRate(1.0)
61+
62+
err := a.Set(context.Background(), &cache.Item{Key: key, Value: "v1"})
63+
if err != nil {
64+
t.Fatalf("Set: got %v, want nil (promotion should have succeeded)", err)
65+
}
66+
67+
if !a.LocalContains(key) {
68+
t.Errorf("LocalContains(%q) on promoting node: got false, want true", key)
69+
}
70+
71+
if chaos.Drops() == 0 {
72+
t.Errorf("chaos.Drops: got 0, want > 0 (chaos didn't see the forward attempt)")
73+
}
74+
}

0 commit comments

Comments
 (0)