diff --git a/cmd/kata/conn_dropped_unix.go b/cmd/kata/conn_dropped_unix.go new file mode 100644 index 00000000..fa6009c9 --- /dev/null +++ b/cmd/kata/conn_dropped_unix.go @@ -0,0 +1,12 @@ +//go:build !windows + +package main + +import ( + "errors" + "syscall" +) + +func connDroppedErrno(err error) bool { + return errors.Is(err, syscall.ECONNRESET) || errors.Is(err, syscall.EPIPE) +} diff --git a/cmd/kata/conn_dropped_unix_test.go b/cmd/kata/conn_dropped_unix_test.go new file mode 100644 index 00000000..cd29dda4 --- /dev/null +++ b/cmd/kata/conn_dropped_unix_test.go @@ -0,0 +1,33 @@ +//go:build !windows + +package main + +import ( + "net" + "net/http" + "net/url" + "os" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConnectionDroppedErrnosUnix(t *testing.T) { + for _, errno := range []syscall.Errno{syscall.EPIPE, syscall.ECONNRESET} { + err := &url.Error{Op: http.MethodPost, URL: "https://daemon.example/issues", Err: &net.OpError{ + Op: "write", Net: "tcp", Err: errno, + }} + got := createRequestError(err, false) + var cliErr *cliError + require.ErrorAs(t, got, &cliErr) + assert.Equal(t, "create_outcome_unknown", cliErr.Code) + assert.NotContains(t, cliErr.Message, "daemon.example") + } + + refused := &url.Error{Op: http.MethodPost, URL: "https://daemon.example/issues", Err: &net.OpError{ + Op: "dial", Err: os.NewSyscallError("connect", syscall.ECONNREFUSED), + }} + assert.Same(t, refused, createRequestError(refused, false)) +} diff --git a/cmd/kata/conn_dropped_windows.go b/cmd/kata/conn_dropped_windows.go new file mode 100644 index 00000000..42bd2872 --- /dev/null +++ b/cmd/kata/conn_dropped_windows.go @@ -0,0 +1,14 @@ +//go:build windows + +package main + +import ( + "errors" + "syscall" +) + +func connDroppedErrno(err error) bool { + return errors.Is(err, syscall.WSAECONNRESET) || + errors.Is(err, syscall.WSAECONNABORTED) || + errors.Is(err, syscall.ERROR_BROKEN_PIPE) +} diff --git a/cmd/kata/conn_dropped_windows_test.go b/cmd/kata/conn_dropped_windows_test.go new file mode 100644 index 00000000..9f711d52 --- /dev/null +++ b/cmd/kata/conn_dropped_windows_test.go @@ -0,0 +1,36 @@ +//go:build windows + +package main + +import ( + "net" + "net/http" + "net/url" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConnectionDroppedErrnosWindows(t *testing.T) { + for _, errno := range []syscall.Errno{ + syscall.WSAECONNRESET, + syscall.WSAECONNABORTED, + syscall.ERROR_BROKEN_PIPE, + } { + err := &url.Error{Op: http.MethodPost, URL: "https://daemon.example/issues", Err: &net.OpError{ + Op: "write", Net: "tcp", Err: errno, + }} + got := createRequestError(err, false) + var cliErr *cliError + require.ErrorAs(t, got, &cliErr) + assert.Equal(t, "create_outcome_unknown", cliErr.Code) + assert.NotContains(t, cliErr.Message, "daemon.example") + } + + refused := &url.Error{Op: http.MethodPost, URL: "https://daemon.example/issues", Err: &net.OpError{ + Op: "dial", Err: syscall.Errno(10061), // WSAECONNREFUSED + }} + assert.Same(t, refused, createRequestError(refused, false)) +} diff --git a/cmd/kata/create.go b/cmd/kata/create.go index 48951470..113e7ad5 100644 --- a/cmd/kata/create.go +++ b/cmd/kata/create.go @@ -201,6 +201,10 @@ func createRequestError(err error, forceNew bool) error { reason = "request timed out" case errors.Is(err, context.Canceled): reason = "request canceled before the response arrived" + case responseBodyWasCut(err): + reason = "the response was cut off before it completed" + case connectionDroppedBeforeResponse(err): + reason = "the connection dropped before the response arrived" default: return err } @@ -222,10 +226,19 @@ func requestTimedOut(err error) bool { return true } var netErr net.Error - if !errors.As(err, &netErr) || netErr == nil { - return false - } - return netErr.Timeout() + // netErr != nil is guaranteed when errors.As returns true; the explicit + // check exists to satisfy NilAway (same idiom as internal/federation). + return errors.As(err, &netErr) && netErr != nil && netErr.Timeout() +} + +func responseBodyWasCut(err error) bool { + var readErr *responseBodyReadError + return errors.As(err, &readErr) +} + +func connectionDroppedBeforeResponse(err error) bool { + return errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) || + connDroppedErrno(err) } // initialLinksAsChanges builds a synthetic mutationChanges from the diff --git a/cmd/kata/create_test.go b/cmd/kata/create_test.go index 6c641d46..33c12712 100644 --- a/cmd/kata/create_test.go +++ b/cmd/kata/create_test.go @@ -64,6 +64,16 @@ func TestCreateRequestError(t *testing.T) { assert.Same(t, otherErr, createRequestError(otherErr, false)) } +func TestCreateRequestErrorConnectionDropped(t *testing.T) { + dropped := &url.Error{Op: http.MethodPost, URL: "https://daemon.example/issues", Err: io.EOF} + got := createRequestError(dropped, false) + var cliErr *cliError + require.ErrorAs(t, got, &cliErr) + assert.Equal(t, "create_outcome_unknown", cliErr.Code) + assert.Contains(t, cliErr.Message, "connection dropped") + +} + func TestCreateRequestErrorCanceled(t *testing.T) { canceledErr := &url.Error{ Op: http.MethodPost, @@ -172,6 +182,45 @@ func TestCreateTimeoutClassificationAtCommandBoundary(t *testing.T) { }) } +func TestCreateResponseBodyCutClassificationAtCommandBoundary(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/projects/resolve": + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"project":{"id":7,"name":"example-project"}}`) + case "/api/v1/projects/7/issues": + _, _ = io.Copy(io.Discard, r.Body) + hj, ok := w.(http.Hijacker) + if !ok { + http.Error(w, "hijacking unsupported", http.StatusInternalServerError) + return + } + conn, buf, err := hj.Hijack() + if err != nil { + return + } + defer func() { _ = conn.Close() }() + _, _ = buf.WriteString("HTTP/1.1 200 OK\r\n" + + "Content-Type: application/json\r\n" + + "Content-Length: 512\r\n\r\n" + + `{"issue":{"short_id":"abc4","title":"example issue","status":"open"`) + _ = buf.Flush() + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + + ctx := contextWithBaseURL(context.Background(), server.URL) + _, _, err := executeRootCapture(t, ctx, "--workspace", t.TempDir(), + "create", "example issue") + cliErr := requireCLIError(t, err, ExitInternal) + assert.Equal(t, "create_outcome_unknown", cliErr.Code) + assert.Contains(t, cliErr.Message, "cut off") + assert.Contains(t, cliErr.Message, "check whether the issue was created") + assert.NotContains(t, cliErr.Message, "timed out") +} + func TestCreate_PrintsIssueShortIDInQuietMode(t *testing.T) { env, dir := setupCLIEnv(t) out := runCLI(t, env, dir, "--quiet", "create", "first issue", "--body", "details") diff --git a/cmd/kata/delete.go b/cmd/kata/delete.go index 276e790f..d57279b7 100644 --- a/cmd/kata/delete.go +++ b/cmd/kata/delete.go @@ -244,6 +244,16 @@ func isShellSafeArg(s string) bool { return true } +// responseBodyReadError marks a response that was cut off after its headers +// arrived, when a mutating request may already have committed. +type responseBodyReadError struct{ err error } + +func (e *responseBodyReadError) Error() string { + return "read response body: " + e.err.Error() +} + +func (e *responseBodyReadError) Unwrap() error { return e.err } + // httpDoJSONWithHeader mirrors httpDoJSON but lets callers attach extra // request headers (notably X-Kata-Confirm). Defined here so delete and the // upcoming purge command don't have to extend the helpers.go signature. @@ -274,7 +284,7 @@ func httpDoJSONWithHeader(ctx context.Context, client *http.Client, defer func() { _ = resp.Body.Close() }() out, err := io.ReadAll(resp.Body) if err != nil { - return 0, nil, err + return 0, nil, &responseBodyReadError{err: err} } return resp.StatusCode, out, nil } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 1d637b57..6be9a5ef 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -139,10 +139,11 @@ kata create \ `--meta` binds string-valued metadata at creation and is repeatable. Before creating an issue, the daemon checks existing non-deleted look-alikes, -including closed ones, using the full title and the first 500 Unicode code -points of the body. `--force-new` bypasses that check; idempotency still wins -when an idempotency key matches. If create times out, or the request is -canceled before the response arrives, its outcome is unknown: check whether +including closed ones, using the first 500 Unicode code points of the title +and body. `--force-new` bypasses that check; idempotency still wins +when an idempotency key matches. If create times out, the request is canceled +or the connection drops before the response arrives, or the response is cut +off before it completes, its outcome is unknown: check whether the issue exists before retrying, and use `--force-new` only after confirming that no issue was created. diff --git a/internal/daemon/handlers_issues.go b/internal/daemon/handlers_issues.go index 3bc5896d..872aadf7 100644 --- a/internal/daemon/handlers_issues.go +++ b/internal/daemon/handlers_issues.go @@ -1320,8 +1320,8 @@ func tryIdempotencyMatch(ctx context.Context, cfg ServerConfig, in *api.CreateIs return fp, out, nil } -// runLookalikeCheck runs the §3.7 soft-block: SearchFTSAny over the title and -// the body prefix used by the scorer +// runLookalikeCheck runs the §3.7 soft-block: SearchFTSAny over the +// 500-code-point title and body prefixes used by the scorer // (OR-of-tokens for high recall), scores each candidate via similarity.Score, // and returns a 409 duplicate_candidates error if any candidate is at or // above the 0.7 threshold. nil means proceed. The OR variant is required diff --git a/internal/daemon/handlers_issues_internal_test.go b/internal/daemon/handlers_issues_internal_test.go index b57bc67f..69668322 100644 --- a/internal/daemon/handlers_issues_internal_test.go +++ b/internal/daemon/handlers_issues_internal_test.go @@ -35,6 +35,18 @@ func TestRunLookalikeCheckBoundsSearchQuery(t *testing.T) { assert.Contains(t, store.query, "留") assert.NotContains(t, store.query, "discard-this-suffix") assert.True(t, utf8.ValidString(store.query)) + + store = &lookalikeQueryRecordingStore{} + in.Body.Title = strings.Repeat("界", 499) + "留 discarded-title-suffix" + in.Body.Body = "plain body" + + err = runLookalikeCheck(context.Background(), ServerConfig{DB: store}, in) + + require.NoError(t, err) + assert.Contains(t, store.query, "plain body") + assert.Contains(t, store.query, "留") + assert.NotContains(t, store.query, "discarded-title-suffix") + assert.True(t, utf8.ValidString(store.query)) } type lookalikeCandidateStore struct { diff --git a/internal/hooks/dispatcher_test.go b/internal/hooks/dispatcher_test.go index 5649567d..0fc4a1f9 100644 --- a/internal/hooks/dispatcher_test.go +++ b/internal/hooks/dispatcher_test.go @@ -53,7 +53,10 @@ func mustNewDispatcher(t *testing.T, hooks []ResolvedHook, cfg Config) (*Dispatc // second wait matters on Windows, where an open appender prevents unlinking. func cleanupDispatcher(t *testing.T, d *Dispatcher) { t.Helper() - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + // Shutdown aborts in-flight work at deadline-2x GraceWindow, so a 2s + // budget is too tight for five sequential process spawns on Windows CI. + const budget = 20 * dispatcherTestGraceWindow + ctx, cancel := context.WithTimeout(context.Background(), budget) err := d.Shutdown(ctx) cancel() if err == nil { diff --git a/internal/similarity/similarity.go b/internal/similarity/similarity.go index 219ab20c..6f38270c 100644 --- a/internal/similarity/similarity.go +++ b/internal/similarity/similarity.go @@ -126,31 +126,43 @@ func Jaccard(a, b []string) float64 { // Score returns the weighted similarity between two issues: // -// 0.6 * Jaccard(title tokens) + 0.4 * Jaccard(body[:500] tokens) +// 0.6 * Jaccard(title[:500] tokens) + 0.4 * Jaccard(body[:500] tokens) // -// Body slicing is rune-based: the first 500 Unicode codepoints of each body -// are tokenized. Spec §3.7. +// Field slicing is rune-based: the first 500 Unicode codepoints of each title +// and body are tokenized. Spec §3.7. func Score(titleA, bodyA, titleB, bodyB string) float64 { - titleScore := Jaccard(Tokenize(titleA), Tokenize(titleB)) + titleScore := Jaccard(Tokenize(TitlePrefix(titleA)), Tokenize(TitlePrefix(titleB))) bodyScore := Jaccard(Tokenize(BodyPrefix(bodyA)), Tokenize(BodyPrefix(bodyB))) return 0.6*titleScore + 0.4*bodyScore } -// BodyPrefix returns the first 500 runes of body. If body has fewer than 500 -// runes, it is returned unchanged. -func BodyPrefix(body string) string { +const lookalikePrefixRunes = 500 + +func fieldPrefix(value string) string { count := 0 - for i := range body { - if count == 500 { - return body[:i] + for i := range value { + if count == lookalikePrefixRunes { + return value[:i] } count++ } - return body + return value +} + +// BodyPrefix returns the first 500 runes of body. If body has fewer than 500 +// runes, it is returned unchanged. +func BodyPrefix(body string) string { + return fieldPrefix(body) +} + +// TitlePrefix returns the first 500 runes of title. If title has fewer than +// 500 runes, it is returned unchanged. +func TitlePrefix(title string) string { + return fieldPrefix(title) } // LookalikeQuery returns the issue text used to retrieve look-alike -// candidates. Its body portion matches the prefix used by Score. +// candidates. Its title and body portions match the prefixes used by Score. func LookalikeQuery(title, body string) string { - return strings.TrimSpace(title + " " + BodyPrefix(body)) + return strings.TrimSpace(TitlePrefix(title) + " " + BodyPrefix(body)) } diff --git a/internal/similarity/similarity_test.go b/internal/similarity/similarity_test.go index 18819e83..3eb402a8 100644 --- a/internal/similarity/similarity_test.go +++ b/internal/similarity/similarity_test.go @@ -41,6 +41,16 @@ func TestLookalikeQuery(t *testing.T) { assert.Equal(t, "full issue title "+prefix, got) } +func TestLookalikeQuery_TitleIsBounded(t *testing.T) { + title := strings.Repeat("界", 499) + "留 discard-this-suffix" + + got := similarity.LookalikeQuery(title, "") + + assert.Contains(t, got, "留") + assert.NotContains(t, got, "discard-this-suffix") + assert.True(t, utf8.ValidString(got)) +} + const epsilon = 1e-9 type tokenizeTestCase struct { @@ -144,6 +154,14 @@ func TestScore_Body500CharLimit(t *testing.T) { "divergence past 500 chars must not affect the score") } +func TestScore_Title500RuneLimit(t *testing.T) { + prefix := strings.Repeat("x", 500) + assertScore(t, 1.0, + prefix+" alpha-divergent", "same body", + prefix+" beta-divergent", "same body", + "title divergence past 500 runes must not affect the score") +} + // TestTokenize_AllStopWordsAreFiltered guards against stopword/stem ordering // regressions. Spec §3.7: stopword removal must come BEFORE stemming so // "has" doesn't stem to "ha" and slip through.