Skip to content

Commit be0e65e

Browse files
committed
admin: cover forwarded ErrTablesNotLeader race-window (Claude review)
forwardErrorResponse already maps ErrTablesNotLeader to 503 leader_unavailable for the case where the leader passes its isVerifiedDynamoLeader guard but then drops leadership mid- Dispatch — the comment at forward_server.go:248 documents the race. No test exercised that path though, which Claude's review on PR #635 called out as a gap. Add two tests that pin both the create and delete sides: TestForwardServer_CreateTable_LeaderSteppedDownReturns503 and TestForwardServer_DeleteTable_LeaderSteppedDownReturns503. Both inject ErrTablesNotLeader through the stub source and assert 503 + leader_unavailable in the response body. These are pure stub-driven unit tests so they run alongside the existing 17 forward-server cases without spinning up a real Raft cluster.
1 parent 2283e59 commit be0e65e

1 file changed

Lines changed: 40 additions & 0 deletions

File tree

internal/admin/forward_server_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,46 @@ func TestForwardServer_DeleteTable_GenericErrorUsesOperationSpecificCode(t *test
314314
require.NotContains(t, string(resp.GetPayload()), "DEL-1")
315315
}
316316

317+
// TestForwardServer_CreateTable_LeaderSteppedDownReturns503 covers
318+
// the race-window comment in forwardErrorResponse: even though the
319+
// leader passed isVerifiedDynamoLeader at the top of
320+
// AdminCreateTable, leadership can still drop mid-dispatch and the
321+
// adapter then returns ErrTablesNotLeader. The forward server must
322+
// surface that as a 503 leader_unavailable response so the
323+
// follower's bridge can re-emit the exact 503 contract clients
324+
// already know — Claude review on PR #635 flagged the gap (no test
325+
// exercised this code path despite the comment).
326+
func TestForwardServer_CreateTable_LeaderSteppedDownReturns503(t *testing.T) {
327+
src := &stubTablesSource{createErr: ErrTablesNotLeader}
328+
srv := newForwardServerForTest(src, fullPrincipalRoleStore())
329+
resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
330+
Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
331+
Operation: pb.AdminOperation_ADMIN_OP_CREATE_TABLE,
332+
Payload: mustJSON(t, CreateTableRequest{TableName: "users", PartitionKey: CreateTableAttribute{Name: "id", Type: "S"}}),
333+
})
334+
require.NoError(t, err)
335+
require.Equal(t, int32(http.StatusServiceUnavailable), resp.GetStatusCode())
336+
require.Contains(t, string(resp.GetPayload()), "leader_unavailable")
337+
}
338+
339+
// TestForwardServer_DeleteTable_LeaderSteppedDownReturns503 mirrors
340+
// the create-side coverage on the delete path.
341+
func TestForwardServer_DeleteTable_LeaderSteppedDownReturns503(t *testing.T) {
342+
src := &stubTablesSource{
343+
tables: map[string]*DynamoTableSummary{"users": {Name: "users"}},
344+
deleteErr: ErrTablesNotLeader,
345+
}
346+
srv := newForwardServerForTest(src, fullPrincipalRoleStore())
347+
resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
348+
Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
349+
Operation: pb.AdminOperation_ADMIN_OP_DELETE_TABLE,
350+
Payload: []byte(`{"name":"users"}`),
351+
})
352+
require.NoError(t, err)
353+
require.Equal(t, int32(http.StatusServiceUnavailable), resp.GetStatusCode())
354+
require.Contains(t, string(resp.GetPayload()), "leader_unavailable")
355+
}
356+
317357
// TestForwardServer_LogsUnexpectedSourceError confirms the leader
318358
// emits an error log line for non-sentinel source failures so
319359
// operators can investigate forwarded write 500s without

0 commit comments

Comments
 (0)