Skip to content

Commit a8de663

Browse files
committed
admin: cover DELETE forward path edge cases (Claude review)
Claude's follow-up review on PR #644 noted that TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError only exercised the create side, leaving tryForwardDelete's matching guard untested. The DELETE forward error paths (ErrLeaderUnavailable, generic transport error) also had no direct coverage. Close the gap so a typo-class change to tryForwardDelete or writeForwardFailure on the delete side fails a test. Three additions: - Extend TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError to a two-axis sweep: 3 create cases (already_exists, validation, generic) × 3 delete cases (not_found, forbidden, generic) — 6 sub-tests pinning that the forward path runs ONLY on ErrTablesNotLeader. - TestDynamoHandler_ForwarderLeaderUnavailableReturns503_Delete: 503 + Retry-After: 1 + leader_unavailable body when the forwarder reports election in flight. - TestDynamoHandler_ForwarderTransportErrorReturns503_Delete: 503 + Retry-After: 1 + no internal detail leaking to the SPA when the forwarder gRPC transport fails. All three reuse the existing stubLeaderForwarder + notLeaderSource fixtures — no new test scaffolding needed.
1 parent d650450 commit a8de663

1 file changed

Lines changed: 84 additions & 4 deletions

File tree

internal/admin/forward_integration_test.go

Lines changed: 84 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,13 @@ func TestDynamoHandler_ForwarderTransportErrorReturns503(t *testing.T) {
169169
// be silently re-applied at the leader. Claude review on PR #644
170170
// noted the test gap; locking it down protects against a future
171171
// change accidentally removing the !errors.Is guard.
172+
//
173+
// Both create and delete paths share the same gate logic, so we
174+
// sweep both surfaces together — a typo-class change to either
175+
// tryForwardCreate or tryForwardDelete will fail one of the
176+
// sub-tests.
172177
func TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError(t *testing.T) {
173-
cases := []struct {
178+
createCases := []struct {
174179
name string
175180
err error
176181
wantCode int
@@ -179,8 +184,8 @@ func TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError(t *testing.T) {
179184
{"validation", &ValidationError{Message: "bad input"}, http.StatusBadRequest},
180185
{"generic", errors.New("opaque storage failure"), http.StatusInternalServerError},
181186
}
182-
for _, tc := range cases {
183-
t.Run(tc.name, func(t *testing.T) {
187+
for _, tc := range createCases {
188+
t.Run("create/"+tc.name, func(t *testing.T) {
184189
fwd := &stubLeaderForwarder{}
185190
src := &stubTablesSource{createErr: tc.err}
186191
h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
@@ -191,9 +196,84 @@ func TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError(t *testing.T) {
191196

192197
require.Equal(t, tc.wantCode, rec.Code)
193198
require.Empty(t, fwd.lastCreateInput.TableName,
194-
"forwarder must not be invoked for source error: %s", tc.name)
199+
"forwarder must not be invoked for create source error: %s", tc.name)
195200
})
196201
}
202+
deleteCases := []struct {
203+
name string
204+
err error
205+
wantCode int
206+
}{
207+
// not_found is the realistic delete-path counterpart to
208+
// already_exists on create.
209+
{"not_found", ErrTablesNotFound, http.StatusNotFound},
210+
{"forbidden", ErrTablesForbidden, http.StatusForbidden},
211+
{"generic", errors.New("opaque storage failure"), http.StatusInternalServerError},
212+
}
213+
for _, tc := range deleteCases {
214+
t.Run("delete/"+tc.name, func(t *testing.T) {
215+
fwd := &stubLeaderForwarder{}
216+
src := &stubTablesSource{
217+
tables: map[string]*DynamoTableSummary{"users": {Name: "users"}},
218+
deleteErr: tc.err,
219+
}
220+
h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
221+
req := httptest.NewRequest(http.MethodDelete, pathDynamoTables+"/users", nil)
222+
req = withWritePrincipal(req)
223+
rec := httptest.NewRecorder()
224+
h.ServeHTTP(rec, req)
225+
226+
require.Equal(t, tc.wantCode, rec.Code)
227+
require.Empty(t, fwd.lastDeleteName,
228+
"forwarder must not be invoked for delete source error: %s", tc.name)
229+
})
230+
}
231+
}
232+
233+
// TestDynamoHandler_ForwarderLeaderUnavailableReturns503_Delete
234+
// mirrors the create-side ErrLeaderUnavailable path — election in
235+
// flight on the leader must surface as 503 + Retry-After: 1 from
236+
// the delete endpoint too. ErrLeaderUnavailable is the
237+
// "no leader known" sentinel from the forwarder layer; tryForwardDelete
238+
// catches it and routes through writeForwardFailure.
239+
func TestDynamoHandler_ForwarderLeaderUnavailableReturns503_Delete(t *testing.T) {
240+
fwd := &stubLeaderForwarder{deleteErr: ErrLeaderUnavailable}
241+
src := &notLeaderSource{stubTablesSource: stubTablesSource{
242+
tables: map[string]*DynamoTableSummary{"users": {Name: "users"}},
243+
}}
244+
h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
245+
req := httptest.NewRequest(http.MethodDelete, pathDynamoTables+"/users", nil)
246+
req = withWritePrincipal(req)
247+
rec := httptest.NewRecorder()
248+
h.ServeHTTP(rec, req)
249+
250+
require.Equal(t, http.StatusServiceUnavailable, rec.Code)
251+
require.Equal(t, "1", rec.Header().Get("Retry-After"))
252+
require.Contains(t, rec.Body.String(), "leader_unavailable")
253+
}
254+
255+
// TestDynamoHandler_ForwarderTransportErrorReturns503_Delete is
256+
// the delete-side counterpart of the create transport-error test:
257+
// a generic gRPC failure from the forwarder must produce 503 +
258+
// Retry-After: 1 with no internal error detail leaking to the SPA.
259+
// The raw error is logged on the server (covered by inspection of
260+
// slog output in the create-side test); the assertion here is
261+
// strictly on the wire shape.
262+
func TestDynamoHandler_ForwarderTransportErrorReturns503_Delete(t *testing.T) {
263+
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+
}}
267+
h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
268+
req := httptest.NewRequest(http.MethodDelete, pathDynamoTables+"/users", nil)
269+
req = withWritePrincipal(req)
270+
rec := httptest.NewRecorder()
271+
h.ServeHTTP(rec, req)
272+
273+
require.Equal(t, http.StatusServiceUnavailable, rec.Code)
274+
require.Equal(t, "1", rec.Header().Get("Retry-After"))
275+
require.NotContains(t, rec.Body.String(), "DEL-TX-1")
276+
require.NotContains(t, rec.Body.String(), "transport sentinel")
197277
}
198278

199279
// TestDynamoHandler_ForwarderForwardsLeaderResponseSetsNosniff

0 commit comments

Comments
 (0)