Test deadline behavior without context identity
This commit is contained in:
@@ -3,18 +3,18 @@
|
|||||||
## Scope And Status
|
## Scope And Status
|
||||||
|
|
||||||
Review baseline: `e177d7c`, covering implementation commits after `5c3b0cf`.
|
Review baseline: `e177d7c`, covering implementation commits after `5c3b0cf`.
|
||||||
This document records an outstanding test-contract issue; it does not authorize
|
This document records a resolved test-contract issue; it does not authorize
|
||||||
unrelated implementation changes. No runtime defect or missing planned feature
|
unrelated implementation changes. No runtime defect or missing planned feature
|
||||||
was identified in this review. Release preparation remains separate work.
|
was identified in this review. Release preparation remains separate work.
|
||||||
|
|
||||||
## R1: Remove The Unrequested Context-Identity Invariant
|
## R1: Remove The Unrequested Context-Identity Invariant
|
||||||
|
|
||||||
Priority: P3 (low). Status: Open.
|
Priority: P3 (low). Status: Complete.
|
||||||
|
|
||||||
Location: `internal/adapter/http/handler_test.go:334–349`,
|
Original location: `internal/adapter/http/handler_test.go:334–349`,
|
||||||
`TestHandlerRunTimeoutValidationAndContextMapping`, introduced in `7de2b02`.
|
`TestHandlerRunTimeoutValidationAndContextMapping`, introduced in `7de2b02`.
|
||||||
|
|
||||||
The test's `child` field and `(ctx != parent) != tc.child` assertion require
|
The test's `child` field and `(ctx != parent) != tc.child` assertion required
|
||||||
the exact parent context object for omitted/null/zero timeouts and a distinct
|
the exact parent context object for omitted/null/zero timeouts and a distinct
|
||||||
object for a positive timeout. The requested contract concerns effective
|
object for a positive timeout. The requested contract concerns effective
|
||||||
deadlines and cancellation propagation, not Go interface identity. In
|
deadlines and cancellation propagation, not Go interface identity. In
|
||||||
@@ -22,7 +22,7 @@ particular, a harmless derived context that preserves the parent's deadline,
|
|||||||
values, and cancellation would fail the omission cases despite satisfying the
|
values, and cancellation would fail the omission cases despite satisfying the
|
||||||
application contract.
|
application contract.
|
||||||
|
|
||||||
This elevates an incidental implementation choice into a test-enforced
|
This elevated an incidental implementation choice into a test-enforced
|
||||||
invariant, contrary to the [agent policy](../policy/agents.md) and
|
invariant, contrary to the [agent policy](../policy/agents.md) and
|
||||||
[testing policy](../policy/testing.md). The current handler's context behavior
|
[testing policy](../policy/testing.md). The current handler's context behavior
|
||||||
is correct; this is a maintainability issue, not a runtime cancellation bug.
|
is correct; this is a maintainability issue, not a runtime cancellation bug.
|
||||||
@@ -48,7 +48,19 @@ refactor solely to change this test.
|
|||||||
is a different object.
|
is a different object.
|
||||||
- Existing timeout validation and HTTP outcome coverage remains intact.
|
- Existing timeout validation and HTTP outcome coverage remains intact.
|
||||||
|
|
||||||
## Review Verification
|
### Resolution And Validation
|
||||||
|
|
||||||
|
Removed the identity-only table field and context comparisons. The test now
|
||||||
|
checks effective deadlines, including omitted/null/zero requests with no
|
||||||
|
parent deadline, and explicitly checks that parent cancellation reaches the
|
||||||
|
runner. Existing positive-deadline integration tests and live-context
|
||||||
|
provider-timeout mapping remain intact. No production code changed.
|
||||||
|
|
||||||
|
Focused HTTP timeout tests, `go test ./...`, `go test -race ./...`,
|
||||||
|
`go vet ./...`, a native build outside the repository, and `git diff --check`
|
||||||
|
all passed after remediation.
|
||||||
|
|
||||||
|
## Original Review Verification
|
||||||
|
|
||||||
- `go test -race -count=1 ./...` passed, including retry, shared-capacity,
|
- `go test -race -count=1 ./...` passed, including retry, shared-capacity,
|
||||||
deadline, report, definition-conformance, and architecture-boundary tests.
|
deadline, report, definition-conformance, and architecture-boundary tests.
|
||||||
|
|||||||
@@ -332,26 +332,30 @@ func TestHandlerRunTimeoutValidationAndContextMapping(t *testing.T) {
|
|||||||
defer cancel()
|
defer cancel()
|
||||||
parentDeadline, _ := parent.Deadline()
|
parentDeadline, _ := parent.Deadline()
|
||||||
for _, tc := range []struct {
|
for _, tc := range []struct {
|
||||||
name, body string
|
name, body string
|
||||||
child bool
|
parent context.Context
|
||||||
|
wantDeadline time.Time
|
||||||
}{
|
}{
|
||||||
{name: "omitted", body: `{"prompt_id":"p"}`},
|
{name: "omitted inherits deadline", body: `{"prompt_id":"p"}`, parent: parent, wantDeadline: parentDeadline},
|
||||||
{name: "null", body: `{"prompt_id":"p","run_timeout_seconds":null}`},
|
{name: "null inherits deadline", body: `{"prompt_id":"p","run_timeout_seconds":null}`, parent: parent, wantDeadline: parentDeadline},
|
||||||
{name: "zero", body: `{"prompt_id":"p","run_timeout_seconds":0}`},
|
{name: "zero inherits deadline", body: `{"prompt_id":"p","run_timeout_seconds":0}`, parent: parent, wantDeadline: parentDeadline},
|
||||||
{name: "positive", body: `{"prompt_id":"p","run_timeout_seconds":10}`, child: true},
|
{name: "positive preserves earlier deadline", body: `{"prompt_id":"p","run_timeout_seconds":10}`, parent: parent, wantDeadline: parentDeadline},
|
||||||
|
{name: "omitted adds no deadline", body: `{"prompt_id":"p"}`, parent: context.Background()},
|
||||||
|
{name: "null adds no deadline", body: `{"prompt_id":"p","run_timeout_seconds":null}`, parent: context.Background()},
|
||||||
|
{name: "zero adds no deadline", body: `{"prompt_id":"p","run_timeout_seconds":0}`, parent: context.Background()},
|
||||||
} {
|
} {
|
||||||
t.Run(tc.name, func(t *testing.T) {
|
t.Run(tc.name, func(t *testing.T) {
|
||||||
called := false
|
called := false
|
||||||
h := NewHandler(runnerFunc(func(ctx context.Context, req promptkit.RunRequest) (*promptkit.RunResult, error) {
|
h := NewHandler(runnerFunc(func(ctx context.Context, req promptkit.RunRequest) (*promptkit.RunResult, error) {
|
||||||
called = true
|
called = true
|
||||||
deadline, ok := ctx.Deadline()
|
deadline, ok := ctx.Deadline()
|
||||||
if !ok || !deadline.Equal(parentDeadline) || (ctx != parent) != tc.child {
|
if ok != !tc.wantDeadline.IsZero() || (ok && !deadline.Equal(tc.wantDeadline)) {
|
||||||
t.Fatalf("unexpected execution context: deadline=%v child=%t", deadline, ctx != parent)
|
t.Fatalf("execution deadline: got %v (present=%t), want %v", deadline, ok, tc.wantDeadline)
|
||||||
}
|
}
|
||||||
return &promptkit.RunResult{}, nil
|
return &promptkit.RunResult{}, nil
|
||||||
}))
|
}))
|
||||||
w := httptest.NewRecorder()
|
w := httptest.NewRecorder()
|
||||||
h.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/v1/runs", strings.NewReader(tc.body)).WithContext(parent))
|
h.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/v1/runs", strings.NewReader(tc.body)).WithContext(tc.parent))
|
||||||
if !called || w.Code != http.StatusOK {
|
if !called || w.Code != http.StatusOK {
|
||||||
t.Fatalf("context mapping: called=%t status=%d body=%s", called, w.Code, w.Body.String())
|
t.Fatalf("context mapping: called=%t status=%d body=%s", called, w.Code, w.Body.String())
|
||||||
}
|
}
|
||||||
@@ -361,6 +365,9 @@ func TestHandlerRunTimeoutValidationAndContextMapping(t *testing.T) {
|
|||||||
canceled, cancelParent := context.WithCancel(context.Background())
|
canceled, cancelParent := context.WithCancel(context.Background())
|
||||||
cancelParent()
|
cancelParent()
|
||||||
h := NewHandler(runnerFunc(func(ctx context.Context, req promptkit.RunRequest) (*promptkit.RunResult, error) {
|
h := NewHandler(runnerFunc(func(ctx context.Context, req promptkit.RunRequest) (*promptkit.RunResult, error) {
|
||||||
|
if ctx.Err() != context.Canceled {
|
||||||
|
t.Fatalf("parent cancellation did not reach execution: %v", ctx.Err())
|
||||||
|
}
|
||||||
return nil, ctx.Err()
|
return nil, ctx.Err()
|
||||||
}))
|
}))
|
||||||
w := httptest.NewRecorder()
|
w := httptest.NewRecorder()
|
||||||
|
|||||||
Reference in New Issue
Block a user