Skip to content

Commit 87043f9

Browse files
committed
admin: refresh stale comment + add Retry-After assertion (Claude review)
Three follow-ups from Claude review on a8de663: 1) writeTablesError ErrTablesNotLeader comment said "follow-up PR" but the forwarder is in this PR. The arm is now reached only when no LeaderForwarder is wired (single-node / leader-only deployments); when one is configured, tryForwardCreate / tryForwardDelete intercept the error before writeTablesError is called. Refreshed the comment to reflect that. 2) TestDynamoHandler_ForwarderTransportErrorReturns503 (CREATE-side) was missing the Retry-After:1 assertion that its DELETE counterpart already had. Added it for parity, guards against a future change that drops Retry-After from writeForwardFailure on only one path. 3) TestDynamoHandler_ForwarderLeaderUnavailableReturns503_Delete and TestDynamoHandler_ForwarderTransportErrorReturns503_Delete passed a tables map into notLeaderSource, but notLeaderSource.AdminDeleteTable unconditionally returns ErrTablesNotLeader without consulting it. Dropped the no-op map init.
1 parent a8de663 commit 87043f9

2 files changed

Lines changed: 10 additions & 9 deletions

File tree

internal/admin/dynamo_handler.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -528,9 +528,10 @@ func (h *DynamoHandler) writeTablesError(w http.ResponseWriter, r *http.Request,
528528
writeJSONError(w, http.StatusForbidden, "forbidden",
529529
"this endpoint requires a full-access role")
530530
case errors.Is(err, ErrTablesNotLeader):
531-
// The follower→leader forwarding RPC (design 3.3) will
532-
// catch this case in a follow-up PR. Until then, surface
533-
// 503 + Retry-After: 1 so the SPA / curl can re-issue.
531+
// Reached only when no LeaderForwarder is configured (single-
532+
// node or leader-only deployments). When a forwarder is wired,
533+
// tryForwardCreate / tryForwardDelete intercept ErrTablesNotLeader
534+
// before writeTablesError is called.
534535
w.Header().Set("Retry-After", "1")
535536
writeJSONError(w, http.StatusServiceUnavailable, "leader_unavailable",
536537
"this admin node is not the raft leader")

internal/admin/forward_integration_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ func TestDynamoHandler_ForwarderTransportErrorReturns503(t *testing.T) {
156156
h.ServeHTTP(rec, req)
157157

158158
require.Equal(t, http.StatusServiceUnavailable, rec.Code)
159+
require.Equal(t, "1", rec.Header().Get("Retry-After"))
159160
require.NotContains(t, rec.Body.String(), "TX-1")
160161
require.NotContains(t, rec.Body.String(), "transport sentinel")
161162
}
@@ -238,9 +239,10 @@ func TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError(t *testing.T) {
238239
// catches it and routes through writeForwardFailure.
239240
func TestDynamoHandler_ForwarderLeaderUnavailableReturns503_Delete(t *testing.T) {
240241
fwd := &stubLeaderForwarder{deleteErr: ErrLeaderUnavailable}
241-
src := &notLeaderSource{stubTablesSource: stubTablesSource{
242-
tables: map[string]*DynamoTableSummary{"users": {Name: "users"}},
243-
}}
242+
// notLeaderSource.AdminDeleteTable unconditionally returns
243+
// ErrTablesNotLeader, so the embedded stubTablesSource.tables
244+
// map is never consulted — keep the construction minimal.
245+
src := &notLeaderSource{}
244246
h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
245247
req := httptest.NewRequest(http.MethodDelete, pathDynamoTables+"/users", nil)
246248
req = withWritePrincipal(req)
@@ -261,9 +263,7 @@ func TestDynamoHandler_ForwarderLeaderUnavailableReturns503_Delete(t *testing.T)
261263
// strictly on the wire shape.
262264
func TestDynamoHandler_ForwarderTransportErrorReturns503_Delete(t *testing.T) {
263265
fwd := &stubLeaderForwarder{deleteErr: errors.New("gRPC transport sentinel DEL-TX-1")}
264-
src := &notLeaderSource{stubTablesSource: stubTablesSource{
265-
tables: map[string]*DynamoTableSummary{"users": {Name: "users"}},
266-
}}
266+
src := &notLeaderSource{}
267267
h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
268268
req := httptest.NewRequest(http.MethodDelete, pathDynamoTables+"/users", nil)
269269
req = withWritePrincipal(req)

0 commit comments

Comments
 (0)