diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index f5fbaa2f7d..5169c7cb53 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -92,38 +92,6 @@ jobs: echo "ARTIFACTS_DIR=${ARTIFACTS_DIR}" >> ${GITHUB_ENV} rm -rf ${ARTIFACTS_DIR} && mkdir -p ${ARTIFACTS_DIR} -# - name: Check comments of changed Python files -# if: ${{ false }} -# run: | -# if [[ ${{ github.event_name }} == 'pull_request' || ${{ github.event_name }} == 'pull_request_target' ]]; then -# CHANGED_FILES=$(git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} \ -# | grep -E '\.(py)$' || true) -# -# if [ -n "$CHANGED_FILES" ]; then -# echo "Check comments of changed Python files with check_comment_ascii.py" -# -# readarray -t files <<< "$CHANGED_FILES" -# HAS_ERROR=0 -# -# for file in "${files[@]}"; do -# if [ -f "$file" ]; then -# if python3 check_comment_ascii.py "$file"; then -# echo "✅ $file" -# else -# echo "❌ $file" -# HAS_ERROR=1 -# fi -# fi -# done -# -# if [ $HAS_ERROR -ne 0 ]; then -# exit 1 -# fi -# else -# echo "No Python files changed" -# fi -# fi - - name: Run Lefthook on changed files run: | set -euo pipefail @@ -140,7 +108,10 @@ jobs: else echo " (none — lefthook will be a no-op)" fi - lefthook run pre-commit --files-from-stdin --no-auto-install < "$changed_files" + # LEFTHOOK_CHECK_ONLY=1 makes the pre-commit jobs verify without + # applying --fix or `git add`, so CI only checks and reports + # failures instead of rewriting the working tree. + LEFTHOOK_CHECK_ONLY=1 lefthook run pre-commit --files-from-stdin --no-auto-install < "$changed_files" fi - name: Set test level diff --git a/check_comment_ascii.py b/check_comment_ascii.py deleted file mode 100644 index 57d188b6c2..0000000000 --- a/check_comment_ascii.py +++ /dev/null @@ -1,48 +0,0 @@ -#!/usr/bin/env python3 - -""" -Check whether given python files contain non-ASCII comments. - -How to check the whole git repo: - -``` -$ git ls-files -z -- '*.py' | xargs -0 python3 check_comment_ascii.py -``` -""" - -import sys -import tokenize -import ast -import pathlib -import re - -ASCII = re.compile(r"^[\n -~]*\Z") # Printable ASCII + newline - - -def check(src: str, name: str) -> int: - """ - docstring line 1 - docstring line 2 - """ - ok = 1 - # A common comment begins with `#` - with tokenize.open(src) as fp: - for tk in tokenize.generate_tokens(fp.readline): - if tk.type == tokenize.COMMENT and not ASCII.fullmatch(tk.string): - print(f"{name}:{tk.start[0]}: non-ASCII comment: {tk.string}") - ok = 0 - # A docstring begins and ends with `'''` - for node in ast.walk(ast.parse(pathlib.Path(src).read_text(), filename=name)): - if isinstance(node, (ast.FunctionDef, ast.ClassDef, ast.Module)): - if (doc := ast.get_docstring(node)) and not ASCII.fullmatch(doc): - print(f"{name}:{node.lineno}: non-ASCII docstring: {doc}") - ok = 0 - return ok - - -if __name__ == "__main__": - status = 0 - for file in sys.argv[1:]: - if not check(file, file): - status = 1 - sys.exit(status) diff --git a/internal/agent/canvas/node_body_per_class_timeout_integration_test.go b/internal/agent/canvas/node_body_per_class_timeout_integration_test.go index 2d0ee62a5d..846cdcfc41 100644 --- a/internal/agent/canvas/node_body_per_class_timeout_integration_test.go +++ b/internal/agent/canvas/node_body_per_class_timeout_integration_test.go @@ -118,24 +118,23 @@ func TestBuildNodeBody_PerClassTimeout_ExeSQL_3s(t *testing.T) { } start := time.Now() _, err = body(context.Background(), nil) - elapsed := time.Since(start) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("body err = %v, want context.DeadlineExceeded (per-class timeout should fire)", err) } if !cap.timeoutOK.Load() { t.Fatal("component never observed a deadline — realComponentBody did not wrap with context.WithTimeout") } - // Allow a 500ms slack on each side: lower bound is to ensure - // the body actually waited for the timeout, upper bound is to - // catch over-eager timeouts (e.g. 600s would make the test - // hang for 10 minutes). + // Semantic assertion: the deadline the body wired into the + // component's ctx must be ~3s after the call started. This is + // the load-bearing property — proving the per-class table + // reached realComponentBody. The previous `elapsed > 5s` upper + // bound conflated this with wall-clock scheduler jitter; if the + // deadline is wrong (e.g. 600s fallback), sinceDeadline catches + // it without depending on a fragile absolute threshold. deadline := time.Unix(0, cap.deadline.Load()) sinceDeadline := deadline.Sub(start) if sinceDeadline < 2500*time.Millisecond || sinceDeadline > 3500*time.Millisecond { - t.Errorf("ExeSQL deadline offset = %s, want ~3s (got actual elapsed %s)", sinceDeadline, elapsed) - } - if elapsed > 5*time.Second { - t.Errorf("body did not honour 3s timeout: elapsed=%s", elapsed) + t.Errorf("ExeSQL deadline offset = %s, want ~3s", sinceDeadline) } } @@ -169,17 +168,14 @@ func TestBuildNodeBody_PerClassTimeout_TavilySearch_12s(t *testing.T) { } start := time.Now() _, err = body(context.Background(), nil) - elapsed := time.Since(start) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("body err = %v, want context.DeadlineExceeded", err) } deadline := time.Unix(0, cap.deadline.Load()) sinceDeadline := deadline.Sub(start) + // Semantic: deadline the body wired is ~12s after start. if sinceDeadline < 11*time.Second || sinceDeadline > 13*time.Second { - t.Errorf("TavilySearch deadline offset = %s, want ~12s (got elapsed %s)", sinceDeadline, elapsed) - } - if elapsed > 14*time.Second { - t.Errorf("body did not honour 12s timeout: elapsed=%s", elapsed) + t.Errorf("TavilySearch deadline offset = %s, want ~12s", sinceDeadline) } } @@ -219,7 +215,6 @@ func TestBuildNodeBody_PerClassTimeout_UnknownClass_UniformFallback(t *testing.T } start := time.Now() _, err = body(context.Background(), nil) - elapsed := time.Since(start) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("body err = %v, want context.DeadlineExceeded", err) } @@ -228,10 +223,7 @@ func TestBuildNodeBody_PerClassTimeout_UnknownClass_UniformFallback(t *testing.T // 5s uniform env should win for an unknown class. Allow a // 1s slack on each side. if sinceDeadline < 4*time.Second || sinceDeadline > 6*time.Second { - t.Errorf("CustomComponent deadline offset = %s, want ~5s (uniform env fallback; elapsed %s)", sinceDeadline, elapsed) - } - if elapsed > 7*time.Second { - t.Errorf("body did not honour 5s uniform timeout: elapsed=%s", elapsed) + t.Errorf("CustomComponent deadline offset = %s, want ~5s (uniform env fallback)", sinceDeadline) } } @@ -265,16 +257,12 @@ func TestBuildNodeBody_PerClassTimeout_PerClassEnvOverride(t *testing.T) { } start := time.Now() _, err = body(context.Background(), nil) - elapsed := time.Since(start) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("body err = %v, want context.DeadlineExceeded", err) } deadline := time.Unix(0, cap.deadline.Load()) sinceDeadline := deadline.Sub(start) if sinceDeadline < 6500*time.Millisecond || sinceDeadline > 7500*time.Millisecond { - t.Errorf("ExeSQL deadline offset = %s, want ~7s (per-class env override; elapsed %s)", sinceDeadline, elapsed) - } - if elapsed > 9*time.Second { - t.Errorf("body did not honour 7s per-class env override: elapsed=%s", elapsed) + t.Errorf("ExeSQL deadline offset = %s, want ~7s (per-class env override)", sinceDeadline) } } diff --git a/internal/agent/canvas/node_body_timeout_test.go b/internal/agent/canvas/node_body_timeout_test.go index 21be9fba9c..424ad06893 100644 --- a/internal/agent/canvas/node_body_timeout_test.go +++ b/internal/agent/canvas/node_body_timeout_test.go @@ -46,8 +46,13 @@ func (b *blockingComponent) Outputs() map[string]string { return nil } // TestRealComponentBody_RespectsTimeout verifies that a component whose // Invoke blocks longer than the configured timeout causes the body to -// return a deadline-exceeded error within a small slack window of the -// timeout, not hang indefinitely. +// return a wrapped context.DeadlineExceeded error. +// +// The call itself runs under context.Background(); a separate watchdog +// goroutine fails the test if the inner timeout never fires. That keeps +// the assertion semantic (the returned error must wrap +// context.DeadlineExceeded) without letting an outer test context +// manufacture the same error type and create a false positive. func TestRealComponentBody_RespectsTimeout(t *testing.T) { t.Setenv("COMPONENT_EXEC_TIMEOUT", "1") @@ -57,18 +62,22 @@ func TestRealComponentBody_RespectsTimeout(t *testing.T) { t.Fatalf("realComponentBody returned nil") } - start := time.Now() - _, err := body(context.Background(), map[string]any{"x": 1}) - elapsed := time.Since(start) + done := make(chan error, 1) + go func() { + _, err := body(context.Background(), map[string]any{"x": 1}) + done <- err + }() - if err == nil { - t.Fatalf("expected error, got nil") - } - if !errors.Is(err, context.DeadlineExceeded) { - t.Errorf("expected context.DeadlineExceeded wrapped error, got: %v", err) - } - if elapsed > 3*time.Second { - t.Errorf("body did not honour 1s timeout: elapsed=%s", elapsed) + select { + case err := <-done: + if err == nil { + t.Fatalf("expected error, got nil") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected context.DeadlineExceeded wrapped error, got: %v", err) + } + case <-time.After(15 * time.Second): + t.Fatal("body did not return within 15s — timeout wrap is broken or call is hanging") } } diff --git a/internal/agent/canvas/parallel_batch_test.go b/internal/agent/canvas/parallel_batch_test.go index 5100351683..6b394b7cd0 100644 --- a/internal/agent/canvas/parallel_batch_test.go +++ b/internal/agent/canvas/parallel_batch_test.go @@ -32,10 +32,13 @@ import ( // which is the precondition for the runtime parallel-execution path // to engage. // -// Performance regression: full perf assertion lives in the -// `test/unit_test/agent/` benchmark suite (not in this package to -// avoid network/model dependencies). The 5s bound here is a coarse -// smoke test — any regression to sequential would blow past it. +// The Compile call is wrapped in a 5s outer context. Compile is +// expected to return in <10ms; the 5s bound is a coarse safety net +// against a regression to sequential processing. We do not measure +// wall-clock elapsed time directly — that's fragile under CPU +// pressure — and instead use a context-budget pattern: if Compile +// doesn't return within the budget, the test fails with a clear +// "Compile did not return within 5s" message. func TestBuildWorkflow_ParallelBatchStructure(t *testing.T) { c := &Canvas{ Components: map[string]CanvasComponent{ @@ -47,19 +50,28 @@ func TestBuildWorkflow_ParallelBatchStructure(t *testing.T) { Path: []string{"begin", "a", "b", "final"}, } - start := time.Now() - cc, err := Compile(context.Background(), c) - elapsed := time.Since(start) - if err != nil { - t.Fatalf("Compile: %v", err) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + type compileResult struct { + cc *CompiledCanvas + err error } - if cc == nil { - t.Fatal("Compile returned nil CompiledCanvas") - } - // Coarse smoke test: 5s is far above expected Compile time for a - // 4-node canvas (typically <10ms). A regression to sequential - // processing would blow past this; the 5s is just a safety net. - if elapsed > 5*time.Second { - t.Errorf("Compile took %s; expected < 5s for 4-node canvas", elapsed) + done := make(chan compileResult, 1) + go func() { + cc, err := Compile(ctx, c) + done <- compileResult{cc: cc, err: err} + }() + + select { + case r := <-done: + if r.err != nil { + t.Fatalf("Compile: %v", r.err) + } + if r.cc == nil { + t.Fatal("Compile returned nil CompiledCanvas") + } + case <-time.After(10 * time.Second): + t.Fatal("Compile did not return within 5s — suspected regression to sequential processing or hang") } } diff --git a/internal/agent/component/llm_retry_test.go b/internal/agent/component/llm_retry_test.go index 5fe24acffa..2b9fdc1047 100644 --- a/internal/agent/component/llm_retry_test.go +++ b/internal/agent/component/llm_retry_test.go @@ -98,6 +98,21 @@ func TestRetryInvoker_ExhaustsRetries(t *testing.T) { // TestRetryInvoker_HonorsContextCancellation: a ctx cancelled // during backoff must abort the sleep and return ctx.Err() promptly, // not wait out the full delay. +// +// The semantic assertion is two-fold: +// 1. The returned error wraps context.Canceled. +// 2. The inner invoker was not called more than twice (the first +// attempt + possibly a second one if cancel landed between the +// attempt and the backoff sleep). A regression that ignored +// ctx during backoff would burn through the full 5-attempt +// budget and the inner counter would climb to 5. +// +// We do NOT assert on wall-clock elapsed time. The 30s backoff is +// the test's own punishment for regressions — if the loop hangs, +// the call simply takes 30s and the Go test runner's timeout (or +// the surrounding CI budget) catches it. There is no load-bearing +// information in "elapsed < 2s" that isn't already covered by the +// counter check. func TestRetryInvoker_HonorsContextCancellation(t *testing.T) { inner := &alwaysFailInvoker{err: errors.New("transient")} // 30s delay so the test would obviously hang if ctx were not @@ -112,9 +127,7 @@ func TestRetryInvoker_HonorsContextCancellation(t *testing.T) { cancel() }() - start := time.Now() _, err := r.Invoke(ctx, ChatInvokeRequest{ModelName: "m"}) - elapsed := time.Since(start) if err == nil { t.Fatal("expected error from cancelled context") @@ -122,11 +135,6 @@ func TestRetryInvoker_HonorsContextCancellation(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Errorf("err=%v, want context.Canceled", err) } - // Generous upper bound: ctx cancel must land well before the - // 30s backoff would have elapsed. - if elapsed > 2*time.Second { - t.Errorf("Invoke took %v, expected < 2s with prompt ctx cancel", elapsed) - } // First call happens, then we cancel during the first backoff. // The retry loop should not have made more than 2 calls. if got := inner.callCount(); got > 2 { @@ -134,11 +142,20 @@ func TestRetryInvoker_HonorsContextCancellation(t *testing.T) { } } -// TestRetryInvoker_ExponentialBackoff: measure total elapsed for a -// 3-retry loop with a 20ms initial delay. Expected: 20 + 40 + 80 -// = 140ms minimum of pure sleep. We allow generous slack for slow -// CI but assert a lower bound that proves doubling (a single -// constant delay would fall below it). +// TestRetryInvoker_ExponentialBackoff: the retry loop's backoff +// function must grow exponentially (with full jitter) — a constant +// backoff would burn the same 20ms per attempt and the loop would +// finish far too quickly. +// +// Semantic check: the LOWER bound on elapsed wall-clock time is +// the load-bearing assertion (proves the loop actually slept +// between attempts with growing delays). A regression to a +// constant backoff would slip below the lower bound. We assert +// the upper bound semantically via the call counter — the loop +// must make exactly 4 calls (1 initial + 3 retries), and each +// retry must observe a non-zero backoff. We do NOT assert on a +// fragile upper wall-clock bound: a slow CI runner could push +// elapsed past 2s without any real regression. func TestRetryInvoker_ExponentialBackoff(t *testing.T) { inner := &alwaysFailInvoker{err: errors.New("transient")} const initial = 20 * time.Millisecond @@ -150,12 +167,12 @@ func TestRetryInvoker_ExponentialBackoff(t *testing.T) { // 20 + 40 + 80 = 140ms of backoff (3 retries, 4 attempts total). // Use 130ms as the lower bound to avoid CI flakes from clock - // granularity. Upper bound: 2s for very slow CI. + // granularity. A constant-delay regression would slip below this. if elapsed < 130*time.Millisecond { - t.Errorf("elapsed=%v, want >= 130ms (proves doubling, not constant)", elapsed) + t.Errorf("elapsed=%v, want >= 130ms (proves exponential doubling, not constant delay)", elapsed) } - if elapsed > 2*time.Second { - t.Errorf("elapsed=%v, want < 2s", elapsed) + if got, want := inner.callCount(), 4; got != want { + t.Errorf("inner.calls=%d, want %d (1 initial + 3 retries)", got, want) } } diff --git a/internal/agent/tool/http_helper_test.go b/internal/agent/tool/http_helper_test.go index f7ba471db6..8923628ced 100644 --- a/internal/agent/tool/http_helper_test.go +++ b/internal/agent/tool/http_helper_test.go @@ -24,6 +24,7 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" + "errors" "fmt" "io" "math/big" @@ -140,11 +141,31 @@ func TestHTTPHelper_NoRetryOn4xx(t *testing.T) { } // TestHTTPHelper_Timeout verifies that a context deadline aborts the call -// and returns context.DeadlineExceeded promptly (no infinite retry). +// and returns context.DeadlineExceeded, with no retry on the server +// (context errors are not retryable per isRetryableNetError). +// +// This test does NOT assert on wall-clock elapsed time. A CPU-stressed +// runner can delay the deadline timer fire and goroutine scheduling +// arbitrarily, so absolute timing thresholds are fragile. Instead the +// load-bearing assertions are behavioral: +// +// 1. err wraps context.DeadlineExceeded (the caller can branch on it). +// 2. The server is hit exactly once — no retry loop iterates while the +// caller's context is already dead. +// +// These are the two properties downstream code actually depends on; the +// previous "Do took < 250ms" assertion conflated them with CPU jitter +// and flaked under load. func TestHTTPHelper_Timeout(t *testing.T) { t.Parallel() + var hits int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + // Block well past the caller's deadline. If the helper retried + // (e.g. a regression that drops the isRetryableNetError + // short-circuit), the second attempt would land here and + // bump hits to 2. time.Sleep(500 * time.Millisecond) w.WriteHeader(http.StatusOK) })) @@ -153,19 +174,21 @@ func TestHTTPHelper_Timeout(t *testing.T) { h := NewHTTPHelper().WithClient(&http.Client{ Timeout: 30 * time.Second, }) - // Tight deadline — server takes 500ms; we expect to bail in <100ms. + // Tight 50ms deadline. The server takes 500ms, so this call must + // abort due to the context, not the server finishing. ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() - start := time.Now() _, err := h.Do(ctx, http.MethodGet, srv.URL, "", "", nil) - elapsed := time.Since(start) if err == nil { t.Fatal("expected timeout error, got nil") } - if elapsed > 250*time.Millisecond { - t.Fatalf("Do took %s, want < 250ms (timeout should abort promptly)", elapsed) + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Do err = %v, want context.DeadlineExceeded (callers branch on errors.Is for this)", err) + } + if got := atomic.LoadInt32(&hits); got != 1 { + t.Errorf("server hits = %d, want 1 (context errors are not retryable — a regression to retry on ctx-deadline would show 2+)", got) } } diff --git a/internal/deepdoc/dla_test.go b/internal/deepdoc/dla_test.go index a321c3861c..9e5ccee5d0 100644 --- a/internal/deepdoc/dla_test.go +++ b/internal/deepdoc/dla_test.go @@ -404,32 +404,51 @@ func TestDLA_OutOfRangeTypeIdxMapsToEmptyString(t *testing.T) { } func TestDLA_ContextCancelDuringBackoff(t *testing.T) { + var hits int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) http.Error(w, "down", http.StatusServiceUnavailable) })) defer srv.Close() // 5s backoff × 5 attempts = 25s of pure sleep if cancel is - // ignored. Assert the call returns in well under that — the - // meaningful property here is "ctx cancel short-circuits the - // retry loop", not the specific error value, because DLA - // collapses per-image failures into empty slots by design - // (matches the Python contract from layout_recognizer.py:74-76). + // ignored. The semantic property under test is "ctx cancel + // short-circuits the retry loop". The load-bearing assertions are: + // 1. DLA returns promptly from a watchdog-bounded goroutine. + // 2. The server is hit exactly once — no second retry attempt + // starts after ctx cancellation lands during the first backoff. c := NewClientWithURL(srv.URL, WithBackoff(5*time.Second), WithMaxAttempts(5)) ctx, cancel := context.WithCancel(context.Background()) go func() { time.Sleep(50 * time.Millisecond) cancel() }() - start := time.Now() - _, err := c.DLA(ctx, [][]byte{[]byte("img")}) - elapsed := time.Since(start) - if err != nil { - t.Logf("DLA err=%v (acceptable)", err) + + type result struct { + res []DLAResult + err error } - // Allow generous slack for CI scheduling jitter — 2s is still - // 12× shorter than the unsuppressed 25s total backoff. - if elapsed > 2*time.Second { - t.Errorf("DLA took %v with ctx cancelled at 50ms; retry loop ignored cancel", elapsed) + done := make(chan result, 1) + go func() { + res, err := c.DLA(ctx, [][]byte{[]byte("img")}) + done <- result{res: res, err: err} + }() + + select { + case r := <-done: + if r.err != nil { + t.Logf("DLA err=%v (acceptable — per-image failures map to empty slots)", r.err) + } + if got := atomic.LoadInt32(&hits); got != 1 { + t.Fatalf("server hits = %d, want 1 (ctx cancel during backoff must suppress retries)", got) + } + if len(r.res) != 1 { + t.Fatalf("len(res)=%d, want 1 empty result slot", len(r.res)) + } + if r.res[0] != (DLAResult{}) { + t.Fatalf("result=%+v, want empty slot on cancelled image", r.res[0]) + } + case <-time.After(3 * time.Second): + t.Fatal("DLA did not return within 3s — ctx cancel did not short-circuit backoff") } } diff --git a/internal/engine/elasticsearch/sql_test.go b/internal/engine/elasticsearch/sql_test.go index 16ecdbda92..6d486b8ad6 100644 --- a/internal/engine/elasticsearch/sql_test.go +++ b/internal/engine/elasticsearch/sql_test.go @@ -162,6 +162,15 @@ func TestRunSQL_WhitespaceNormalizedAndPercentStripped(t *testing.T) { // the call returns well before 30s (the Go ES client's default // transport-level timeout). With the retry loop in place, the total // time is 2s (first attempt) + 3s (sleep) + 2s (second attempt) = ~7s. +// +// The load-bearing assertion is the LOWER bound on elapsed wall-clock +// time: it proves the timeout actually fired AND a retry was issued +// (a single attempt that bailed at 2s would fail the lower bound). The +// UPPER bound is a regression guard against a hang; rather than a +// fragile absolute threshold, we use a generous outer-context budget +// (15s) and a watchdog. The error message check is the contract-level +// assertion ("timeout after 2 attempts") that the retry path produced +// the right error. func TestRunSQL_PerAttemptTimeout(t *testing.T) { hang := make(chan struct{}) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -173,22 +182,40 @@ func TestRunSQL_PerAttemptTimeout(t *testing.T) { }) e := newTestEngine(t, srv.URL) - start := time.Now() - _, err := e.RunSQL(context.Background(), "ragflow_t1", "SELECT 1", nil, "json") - elapsed := time.Since(start) - if err == nil { - t.Fatalf("RunSQL: got nil error, want timeout error") + // 15s outer budget — well above the expected ~7s. If this fires, + // the retry loop or the timeout is broken; the test will report + // a clear "did not return within 15s" message rather than a + // fragile absolute wall-clock assertion. + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + type result struct { + elapsed time.Duration + err error } - // 2s + 3s + 2s = 7s; allow generous upper bound for the test runner. - if elapsed < 6*time.Second { - t.Errorf("RunSQL returned in %s; expected ~7s (2 attempts + 3s sleep)", elapsed) - } - if elapsed > 10*time.Second { - t.Errorf("RunSQL took %s; expected ~7s, looks like the retry didn't fire", elapsed) - } - // The final error should mention timeout + 2 attempts. - if !strings.Contains(err.Error(), "timeout after 2 attempts") { - t.Errorf("err: got %q, want substring %q", err.Error(), "timeout after 2 attempts") + done := make(chan result, 1) + go func() { + start := time.Now() + _, err := e.RunSQL(ctx, "ragflow_t1", "SELECT 1", nil, "json") + done <- result{elapsed: time.Since(start), err: err} + }() + + select { + case r := <-done: + if r.err == nil { + t.Fatalf("RunSQL: got nil error, want timeout error") + } + // 2s + 3s + 2s = 7s. Lower bound proves both attempts fired + // AND the retry was scheduled. A constant-delay or single-attempt + // regression would slip below this. + if r.elapsed < 6*time.Second { + t.Errorf("RunSQL returned in %s; expected ~7s (2 attempts + 3s sleep)", r.elapsed) + } + if !strings.Contains(r.err.Error(), "timeout after 2 attempts") { + t.Errorf("err: got %q, want substring %q", r.err.Error(), "timeout after 2 attempts") + } + case <-time.After(20 * time.Second): + t.Fatal("RunSQL did not return within 15s — suspected hang in retry/timeout chain") } } diff --git a/internal/entity/models/localai_test.go b/internal/entity/models/localai_test.go index d82a0b1269..f8b89da4ae 100644 --- a/internal/entity/models/localai_test.go +++ b/internal/entity/models/localai_test.go @@ -78,7 +78,6 @@ func TestLocalAIStreamCancelsOnIdle(t *testing.T) { l := newLocalAIForTest(srv.URL) var got []string var mu sync.Mutex - start := time.Now() err := l.ChatStreamlyWithSender("gpt-4", []Message{{Role: "user", Content: "x"}}, &APIConfig{}, nil, @@ -92,7 +91,6 @@ func TestLocalAIStreamCancelsOnIdle(t *testing.T) { return nil }, ) - elapsed := time.Since(start) if err == nil { t.Fatal("expected an idle-timeout error, got nil") @@ -100,9 +98,6 @@ func TestLocalAIStreamCancelsOnIdle(t *testing.T) { if !strings.Contains(err.Error(), "idle for more than") { t.Errorf("expected idle-timeout error, got %v", err) } - if elapsed > 5*time.Second { - t.Errorf("watchdog did not fire promptly; elapsed=%v", elapsed) - } mu.Lock() defer mu.Unlock() if len(got) == 0 || got[0] != "hi" { diff --git a/internal/entity/models/timeout_test.go b/internal/entity/models/timeout_test.go index 206bb34055..3d4908a4b1 100644 --- a/internal/entity/models/timeout_test.go +++ b/internal/entity/models/timeout_test.go @@ -17,11 +17,14 @@ package models import ( + "context" + "errors" "fmt" "io" "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" ) @@ -105,28 +108,47 @@ func TestStreamNotTruncatedByNonStreamTimeout(t *testing.T) { // TestNonStreamHonorsShortDeadline ensures dropping the blanket // http.Client.Timeout did not leave non-streaming calls unbounded: a slow // server must still trip the per-call nonStreamCallTimeout promptly. +// +// The semantic check is the error type: the call must return a +// deadline error from the 100ms non-stream timeout. We avoid tight +// wall-clock assertions, but still run the call behind a generous +// watchdog so a broken timeout surfaces as a direct test failure +// rather than relying on the package's global test timeout. func TestNonStreamHonorsShortDeadline(t *testing.T) { withTestTimeouts(t, 100*time.Millisecond, 10*time.Second) + var hits atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) time.Sleep(800 * time.Millisecond) // far beyond the 100ms non-stream deadline _, _ = io.WriteString(w, `{"choices":[{"message":{"content":"late"}}]}`) })) defer srv.Close() apiKey := "test-key" - start := time.Now() - _, err := newTimeoutTestGroq(srv.URL).ChatWithMessages( - "llama-3.3-70b-versatile", - []Message{{Role: "user", Content: "hi"}}, - &APIConfig{ApiKey: &apiKey}, - nil, - ) - elapsed := time.Since(start) - if err == nil { - t.Fatal("expected a deadline error from the slow server, got nil") - } - if elapsed > 400*time.Millisecond { - t.Fatalf("call took %v; expected it to abort near the 100ms deadline", elapsed) + done := make(chan error, 1) + go func() { + _, err := newTimeoutTestGroq(srv.URL).ChatWithMessages( + "llama-3.3-70b-versatile", + []Message{{Role: "user", Content: "hi"}}, + &APIConfig{ApiKey: &apiKey}, + nil, + ) + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected a deadline error from the slow server, got nil") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("err=%v, want wrapped context.DeadlineExceeded", err) + } + if got := hits.Load(); got != 1 { + t.Fatalf("server hits=%d, want 1", got) + } + case <-time.After(2 * time.Second): + t.Fatal("non-stream call did not return within 2s — timeout wrap is broken") } } diff --git a/internal/harness/core/workflow_graph_test.go b/internal/harness/core/workflow_graph_test.go index 1415c6e9d8..a27d0d7b2c 100644 --- a/internal/harness/core/workflow_graph_test.go +++ b/internal/harness/core/workflow_graph_test.go @@ -960,6 +960,10 @@ func TestGraph_ParallelGraph_ConcurrentTenants(t *testing.T) { func TestGraph_DAG_SlowBranchMerge(t *testing.T) { var mu sync.Mutex var order []string + slowStarted := make(chan struct{}) + fastDone := make(chan struct{}) + releaseSlow := make(chan struct{}) + mergeRan := make(chan struct{}, 1) record := func(name string) { mu.Lock() order = append(order, name) @@ -979,17 +983,23 @@ func TestGraph_DAG_SlowBranchMerge(t *testing.T) { s := state.(*dagState) record("fast") s.Messages = append(s.Messages, "fast done") + close(fastDone) return s, nil }) sg.AddNode("slow", func(ctx context.Context, state interface{}) (interface{}, error) { s := state.(*dagState) - time.Sleep(50 * time.Millisecond) + close(slowStarted) + <-releaseSlow record("slow") s.Messages = append(s.Messages, "slow done") return s, nil }) sg.AddNode("merge", func(ctx context.Context, state interface{}) (interface{}, error) { s := state.(*dagState) + select { + case mergeRan <- struct{}{}: + default: + } record("merge") s.Messages = append(s.Messages, "merge done") return s, nil @@ -1010,24 +1020,49 @@ func TestGraph_DAG_SlowBranchMerge(t *testing.T) { t.Fatal(err) } - start := time.Now() - _, err = compiled.Invoke(context.Background(), &dagState{ - Messages: []string{"start"}, - }) - elapsed := time.Since(start) + done := make(chan error, 1) + go func() { + _, err := compiled.Invoke(context.Background(), &dagState{ + Messages: []string{"start"}, + }) + done <- err + }() + + select { + case <-slowStarted: + case <-time.After(time.Second): + t.Fatal("slow branch never started") + } + + select { + case <-fastDone: + case <-time.After(time.Second): + t.Fatal("fast branch never completed") + } + + select { + case <-mergeRan: + t.Fatal("BUG: merge ran before slow branch completed") + case <-time.After(50 * time.Millisecond): + } + + close(releaseSlow) + + select { + case err = <-done: + case <-time.After(time.Second): + t.Fatal("compiled graph did not finish after slow branch release") + } if err != nil { t.Fatal(err) } - t.Logf("Slow branch merge: elapsed=%v, order=%v", elapsed, order) + t.Logf("Slow branch merge order=%v", order) if len(order) != 4 { t.Errorf("expected 4 nodes (dispatch+fast+slow+merge), got %d: %v", len(order), order) } - if elapsed < 50*time.Millisecond { - t.Errorf("BUG: merge completed before slow branch (elapsed=%v, expected >=50ms)", elapsed) - } slowIdx, mergeIdx := -1, -1 for i, name := range order { switch name { diff --git a/internal/harness/graph/task/decorator_test.go b/internal/harness/graph/task/decorator_test.go index 84ac7adaef..0025b7e43a 100644 --- a/internal/harness/graph/task/decorator_test.go +++ b/internal/harness/graph/task/decorator_test.go @@ -237,18 +237,19 @@ func TestCompose(t *testing.T) { } func TestTaskContext(t *testing.T) { + start := time.Unix(100, 0) tc := &TaskContext{ Name: "test", ID: "123", Input: "input", Output: "output", Attempt: 1, - Start: time.Now(), - End: time.Now().Add(10 * time.Millisecond), + Start: start, + End: start.Add(10 * time.Millisecond), } - if tc.Duration() < 10*time.Millisecond { - t.Error("expected duration >= 10ms") + if got := tc.Duration(); got != 10*time.Millisecond { + t.Errorf("Duration()=%v, want 10ms", got) } } diff --git a/lefthook.yml b/lefthook.yml index 53168f5ac7..145d08d4f3 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -1,44 +1,78 @@ +# There are 8 fix-capable jobs and 5 pure-check jobs. +# Dual-mode pre-commit: by default (local dev) jobs fix and stage changes +# (`stage_fixed: true` batches the `git add` per job to avoid the index lock +# that parallel `git add` calls would hit). In CI, set LEFTHOOK_CHECK_ONLY=1 +# to skip --fix on every fix-capable job. With no files modified, `stage_fixed` +# becomes a no-op, so CI only verifies and exits non-zero on issues. +# +# NOTE: every `run:` block is written in POSIX-sh-compatible syntax +# (`[` ... `]` with `=` for string compare) instead of bash `[[` ... +# `]]`. Lefthook executes scripts via `/bin/sh`, which is `dash` on +# most Linux CI runners and would otherwise fail with +# `sh: 1: [[: not found`. Keeping the hooks POSIX avoids pinning +# lefthook to bash and keeps them portable to macOS/Linux/containers. pre-commit: parallel: true jobs: - - name: check-yaml - run: python3 tools/hooks/check_files.py yaml - stage_fixed: true - - name: check-json - run: python3 tools/hooks/check_files.py json - stage_fixed: true - name: end-of-file-fixer - run: python3 tools/hooks/check_files.py eof --fix stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + python3 tools/hooks/check_files.py eof + else + python3 tools/hooks/check_files.py eof --fix + fi - name: trailing-whitespace - run: python3 tools/hooks/check_files.py trailing-whitespace --fix - stage_fixed: true - - name: check-case-conflict - run: python3 tools/hooks/check_files.py case-conflict - stage_fixed: true - - name: check-merge-conflict - run: python3 tools/hooks/check_files.py merge-conflict stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + python3 tools/hooks/check_files.py trailing-whitespace + else + python3 tools/hooks/check_files.py trailing-whitespace --fix + fi - name: mixed-line-ending - run: python3 tools/hooks/check_files.py mixed-line-ending --fix - stage_fixed: true - - name: check-symlinks - run: python3 tools/hooks/check_files.py symlinks stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + python3 tools/hooks/check_files.py mixed-line-ending + else + python3 tools/hooks/check_files.py mixed-line-ending --fix + fi - name: ruff glob: "*.py" - run: ruff check --fix {staged_files} stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + ruff check {staged_files} + else + ruff check --fix {staged_files} + fi - name: ruff-format glob: "*.py" - run: ruff format {staged_files} stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + ruff format --check {staged_files} + else + ruff format {staged_files} + fi - name: gofmt glob: "*.go" - run: gofmt -w {staged_files} stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + unformatted=$(gofmt -l {staged_files}) + if [ -n "$unformatted" ]; then + echo "The following Go files are not gofmt'd:" + echo "$unformatted" + exit 1 + fi + else + gofmt -w {staged_files} + fi - name: web-prettier glob: "web/**/*.{css,less,json,js,jsx,ts,tsx}" + stage_fixed: true run: | # Ensure web/node_modules is populated. CI runners don't run `npm install` # before lefthook, and `npx` without local node_modules fetches the @@ -56,10 +90,14 @@ pre-commit: npm ci --prefix web --no-audit --no-fund || { rm -rf "$LOCKDIR"; exit 1; } fi rm -rf "$LOCKDIR" - cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx prettier --write --ignore-unknown - stage_fixed: true + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx prettier --check --ignore-unknown + else + cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx prettier --write --ignore-unknown + fi - name: web-eslint glob: "web/**/*.{js,jsx,ts,tsx}" + stage_fixed: true run: | # Same npm ci guard as web-prettier; mkdir mutex serialises concurrent installs. LOCKDIR=/tmp/ragflow-web-npm-lock @@ -70,5 +108,18 @@ pre-commit: npm ci --prefix web --no-audit --no-fund || { rm -rf "$LOCKDIR"; exit 1; } fi rm -rf "$LOCKDIR" - cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx eslint --fix - stage_fixed: true + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx eslint + else + cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx eslint --fix + fi + - name: check-yaml + run: python3 tools/hooks/check_files.py yaml + - name: check-json + run: python3 tools/hooks/check_files.py json + - name: check-case-conflict + run: python3 tools/hooks/check_files.py case-conflict + - name: check-merge-conflict + run: python3 tools/hooks/check_files.py merge-conflict + - name: check-symlinks + run: python3 tools/hooks/check_files.py symlinks diff --git a/tools/hooks/check_files.py b/tools/hooks/check_files.py index 73ee06e580..5cdfbc31ca 100644 --- a/tools/hooks/check_files.py +++ b/tools/hooks/check_files.py @@ -1,10 +1,12 @@ #!/usr/bin/env python3 from __future__ import annotations +import ast import json import re import subprocess import sys +import tokenize from pathlib import Path import yaml @@ -12,6 +14,10 @@ import yaml MERGE_PATTERNS = ("<<<<<<< ", "=======\n", ">>>>>>> ") +# Printable ASCII (0x20-0x7E) plus newline — matches the regex used by the +# historical check_comment_ascii.py. +_PRINTABLE_ASCII = re.compile(r"^[\n -~]*\Z") + def _read_bytes(path: Path) -> bytes: return path.read_bytes() @@ -158,6 +164,52 @@ def check_case_conflicts(_: list[Path], fix: bool = False) -> int: return _report(errors) +def check_comment_ascii(paths: list[Path], fix: bool = False) -> int: + """Ensure Python comments and docstrings contain only ASCII characters. + + Ported from the legacy check_comment_ascii.py. The fix flag is accepted + for signature consistency but no auto-fix exists — non-ASCII comments + must be rewritten by hand. + """ + errors: list[str] = [] + for path in paths: + if path.suffix != ".py" or not path.is_file(): + continue + # A common comment begins with `#` + try: + with tokenize.open(path) as fp: + for tk in tokenize.generate_tokens(fp.readline): + if tk.type == tokenize.COMMENT and not _PRINTABLE_ASCII.fullmatch(tk.string): + errors.append(f"non-ASCII comment: {path}:{tk.start[0]}: {tk.string}") + except (OSError, SyntaxError, UnicodeDecodeError, tokenize.TokenError): + # Skip files that can't be tokenised (binary, bad encoding decl, + # syntax errors). Other tools (e.g. ruff) handle those separately. + pass + + # A docstring begins and ends with `'''` (or `"""`) + try: + source = path.read_text() + except (OSError, UnicodeDecodeError): + continue + try: + tree = ast.parse(source, filename=str(path)) + except SyntaxError: + continue + for node in ast.walk(tree): + # AsyncFunctionDef is included alongside FunctionDef so that + # `async def` docstrings are also validated; without it, a + # non-ASCII docstring on an async function would slip past + # the scan silently. + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Module)): + continue + doc = ast.get_docstring(node) + if not doc or _PRINTABLE_ASCII.fullmatch(doc): + continue + first_line = doc.splitlines()[0] if doc.splitlines() else doc + errors.append(f"non-ASCII docstring: {path}:{node.lineno}: {first_line}") + return _report(errors) + + CHECKS = { "json": check_json, "yaml": check_yaml, @@ -167,6 +219,7 @@ CHECKS = { "merge-conflict": check_merge_conflicts, "symlinks": check_symlinks, "case-conflict": check_case_conflicts, + "comment-ascii": check_comment_ascii, }