fix(ingestion): improve Extractor/LLM robustness and Python->Go parity (#17470)
## Summary
This PR hardens the Go ingestion **Extractor** and **LLM retry** paths
and closes several Python->Go parity gaps in the keyword/question/tag
extraction flow.
- **Generic retry utility** (`internal/common/retry.go`):
`RetryWithBackoff` with exponential backoff (default 3 retries, 2s
initial delay, capped at 1m), context-aware sleep, and a `maxRetries<=0`
fast path. Covered by `internal/common/retry_test.go`.
- **LLM retry reuse**: `agent/component/llm_retry.go` now delegates to
`common.RetryWithBackoff` instead of an inline loop (behavior preserved:
ctx cancellation short-circuits the backoff).
- **Extractor LLM calls** (`extractor.go`):
- `call()` now retries transient LLM failures via `RetryWithBackoff`
(retry exhaustion fails the chunk instead of silently skipping).
- Sets `temperature = 0.2`, matching Python `generator.py:230,245`.
- Runs keyword and question extraction **concurrently** per chunk when
both are enabled (`task_executor.py:444-448`), with mutex-guarded map
writes to avoid data races.
- Substitutes `{field_name}` placeholders (including `{chunks}` -> chunk
text) in `prompt`/`system_prompt` before the call, mirroring Python
`string_format` (`extractor.py:102-103`); unmatched placeholders are
left as-is.
- Falls back to the **tenant default chat model** when `llm_id` is empty
(`task_executor.py:573-574`).
- Strips `` **greedily** (`strings.LastIndex`) in
`cleanExtractionResult`.
- **Auto-tagging** (`extractor_tag.go`): drops the `in.llmID != ""`
guards so an empty `llm_id` no longer skips tagging (uses the tenant
default model), and strips `` greedily in `parseTaggerResponse`.
- **Docs**: fixes a misleading `PresentationChunker` docstring that
claimed per-slide `image`/`position` output (the PPTX path emits none —
unlike PDF), and removes a stale `docs/migration_python_go_diff.md`
reference in `media_dispatch.go`.
## Test plan
- `bash build.sh --test ./internal/common/...` — passes (new retry
utility + tests).
- `bash build.sh --test ./internal/ingestion/component/...` — passes
(extractor/chunker/schema).
- `gofmt` and lefthook pre-commit checks pass.
Note: the personal `docs/migration_python_go_diff.md` working notebook
in the tree is intentionally **not** part of this PR.
---------
Co-authored-by: CodeBuddy <noreply@codebuddy.ai>
2026-07-28 19:22:18 +08:00
|
|
|
// Package common — generic retry utility with exponential backoff.
|
|
|
|
|
//
|
|
|
|
|
// RetryWithBackoff calls fn up to maxRetries+1 times (one initial
|
|
|
|
|
// attempt + maxRetries retries), sleeping delay*2^attempt between
|
|
|
|
|
// failures (capped at 1 minute). The sleep honours ctx cancellation.
|
|
|
|
|
//
|
|
|
|
|
// maxRetries <= 0 yields a single attempt (no retries).
|
|
|
|
|
// initialDelay <= 0 results in no delay between retries.
|
|
|
|
|
//
|
|
|
|
|
// Usage:
|
|
|
|
|
//
|
|
|
|
|
// err := RetryWithBackoff(ctx, 3, 2*time.Second, func() error {
|
|
|
|
|
// return someFailableOperation()
|
|
|
|
|
// })
|
|
|
|
|
|
|
|
|
|
package common
|
|
|
|
|
|
|
|
|
|
import (
|
|
|
|
|
"context"
|
2026-08-07 17:47:59 +08:00
|
|
|
"errors"
|
fix(ingestion): improve Extractor/LLM robustness and Python->Go parity (#17470)
## Summary
This PR hardens the Go ingestion **Extractor** and **LLM retry** paths
and closes several Python->Go parity gaps in the keyword/question/tag
extraction flow.
- **Generic retry utility** (`internal/common/retry.go`):
`RetryWithBackoff` with exponential backoff (default 3 retries, 2s
initial delay, capped at 1m), context-aware sleep, and a `maxRetries<=0`
fast path. Covered by `internal/common/retry_test.go`.
- **LLM retry reuse**: `agent/component/llm_retry.go` now delegates to
`common.RetryWithBackoff` instead of an inline loop (behavior preserved:
ctx cancellation short-circuits the backoff).
- **Extractor LLM calls** (`extractor.go`):
- `call()` now retries transient LLM failures via `RetryWithBackoff`
(retry exhaustion fails the chunk instead of silently skipping).
- Sets `temperature = 0.2`, matching Python `generator.py:230,245`.
- Runs keyword and question extraction **concurrently** per chunk when
both are enabled (`task_executor.py:444-448`), with mutex-guarded map
writes to avoid data races.
- Substitutes `{field_name}` placeholders (including `{chunks}` -> chunk
text) in `prompt`/`system_prompt` before the call, mirroring Python
`string_format` (`extractor.py:102-103`); unmatched placeholders are
left as-is.
- Falls back to the **tenant default chat model** when `llm_id` is empty
(`task_executor.py:573-574`).
- Strips `` **greedily** (`strings.LastIndex`) in
`cleanExtractionResult`.
- **Auto-tagging** (`extractor_tag.go`): drops the `in.llmID != ""`
guards so an empty `llm_id` no longer skips tagging (uses the tenant
default model), and strips `` greedily in `parseTaggerResponse`.
- **Docs**: fixes a misleading `PresentationChunker` docstring that
claimed per-slide `image`/`position` output (the PPTX path emits none —
unlike PDF), and removes a stale `docs/migration_python_go_diff.md`
reference in `media_dispatch.go`.
## Test plan
- `bash build.sh --test ./internal/common/...` — passes (new retry
utility + tests).
- `bash build.sh --test ./internal/ingestion/component/...` — passes
(extractor/chunker/schema).
- `gofmt` and lefthook pre-commit checks pass.
Note: the personal `docs/migration_python_go_diff.md` working notebook
in the tree is intentionally **not** part of this PR.
---------
Co-authored-by: CodeBuddy <noreply@codebuddy.ai>
2026-07-28 19:22:18 +08:00
|
|
|
"fmt"
|
2026-08-07 17:47:59 +08:00
|
|
|
"strings"
|
fix(ingestion): improve Extractor/LLM robustness and Python->Go parity (#17470)
## Summary
This PR hardens the Go ingestion **Extractor** and **LLM retry** paths
and closes several Python->Go parity gaps in the keyword/question/tag
extraction flow.
- **Generic retry utility** (`internal/common/retry.go`):
`RetryWithBackoff` with exponential backoff (default 3 retries, 2s
initial delay, capped at 1m), context-aware sleep, and a `maxRetries<=0`
fast path. Covered by `internal/common/retry_test.go`.
- **LLM retry reuse**: `agent/component/llm_retry.go` now delegates to
`common.RetryWithBackoff` instead of an inline loop (behavior preserved:
ctx cancellation short-circuits the backoff).
- **Extractor LLM calls** (`extractor.go`):
- `call()` now retries transient LLM failures via `RetryWithBackoff`
(retry exhaustion fails the chunk instead of silently skipping).
- Sets `temperature = 0.2`, matching Python `generator.py:230,245`.
- Runs keyword and question extraction **concurrently** per chunk when
both are enabled (`task_executor.py:444-448`), with mutex-guarded map
writes to avoid data races.
- Substitutes `{field_name}` placeholders (including `{chunks}` -> chunk
text) in `prompt`/`system_prompt` before the call, mirroring Python
`string_format` (`extractor.py:102-103`); unmatched placeholders are
left as-is.
- Falls back to the **tenant default chat model** when `llm_id` is empty
(`task_executor.py:573-574`).
- Strips `` **greedily** (`strings.LastIndex`) in
`cleanExtractionResult`.
- **Auto-tagging** (`extractor_tag.go`): drops the `in.llmID != ""`
guards so an empty `llm_id` no longer skips tagging (uses the tenant
default model), and strips `` greedily in `parseTaggerResponse`.
- **Docs**: fixes a misleading `PresentationChunker` docstring that
claimed per-slide `image`/`position` output (the PPTX path emits none —
unlike PDF), and removes a stale `docs/migration_python_go_diff.md`
reference in `media_dispatch.go`.
## Test plan
- `bash build.sh --test ./internal/common/...` — passes (new retry
utility + tests).
- `bash build.sh --test ./internal/ingestion/component/...` — passes
(extractor/chunker/schema).
- `gofmt` and lefthook pre-commit checks pass.
Note: the personal `docs/migration_python_go_diff.md` working notebook
in the tree is intentionally **not** part of this PR.
---------
Co-authored-by: CodeBuddy <noreply@codebuddy.ai>
2026-07-28 19:22:18 +08:00
|
|
|
"time"
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
const (
|
|
|
|
|
// DefaultRetryMax is the default number of retries (3).
|
|
|
|
|
DefaultRetryMax = 3
|
|
|
|
|
// DefaultRetryDelay is the initial backoff delay (2s).
|
|
|
|
|
DefaultRetryDelay = 2 * time.Second
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
// RetryWithBackoff retries fn on error with exponential backoff.
|
|
|
|
|
// Returns nil on the first successful attempt. Returns the last
|
|
|
|
|
// error wrapped with the retry count when all attempts fail.
|
|
|
|
|
//
|
|
|
|
|
// An optional shouldRetry predicate lets callers abort on
|
|
|
|
|
// non-transient errors: when supplied and it returns false for a
|
|
|
|
|
// given error, RetryWithBackoff stops immediately and returns that
|
|
|
|
|
// error without further backoff. A nil predicate (or a nil function
|
|
|
|
|
// in the slice) retries on every error, preserving the original
|
|
|
|
|
// behavior.
|
|
|
|
|
func RetryWithBackoff(ctx context.Context, maxRetries int, initialDelay time.Duration, fn func() error, shouldRetry ...func(error) bool) error {
|
|
|
|
|
if maxRetries <= 0 {
|
|
|
|
|
return fn()
|
|
|
|
|
}
|
|
|
|
|
canRetry := func(err error) bool {
|
|
|
|
|
if len(shouldRetry) == 0 || shouldRetry[0] == nil {
|
|
|
|
|
return true
|
|
|
|
|
}
|
|
|
|
|
return shouldRetry[0](err)
|
|
|
|
|
}
|
|
|
|
|
delay := initialDelay
|
|
|
|
|
var lastErr error
|
|
|
|
|
for attempt := 0; attempt <= maxRetries; attempt++ {
|
|
|
|
|
err := fn()
|
|
|
|
|
if err == nil {
|
|
|
|
|
return nil
|
|
|
|
|
}
|
|
|
|
|
lastErr = err
|
|
|
|
|
if !canRetry(err) {
|
|
|
|
|
return err
|
|
|
|
|
}
|
|
|
|
|
if attempt == maxRetries {
|
|
|
|
|
break
|
|
|
|
|
}
|
|
|
|
|
select {
|
|
|
|
|
case <-ctx.Done():
|
|
|
|
|
return ctx.Err()
|
|
|
|
|
case <-time.After(delay):
|
|
|
|
|
}
|
|
|
|
|
delay *= 2
|
|
|
|
|
if delay > time.Minute {
|
|
|
|
|
delay = time.Minute
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
return fmt.Errorf("failed after %d retries: %w", maxRetries, lastErr)
|
|
|
|
|
}
|
2026-08-07 17:47:59 +08:00
|
|
|
|
|
|
|
|
// IsTransientError reports whether err is a transient provider failure worth
|
|
|
|
|
// retrying, as opposed to a permanent configuration/client error. It is the
|
|
|
|
|
// shared shouldRetry predicate for provider HTTP calls.
|
|
|
|
|
//
|
|
|
|
|
// Classification:
|
|
|
|
|
// - context.Canceled: permanent (caller cancelled; do not retry)
|
|
|
|
|
// - context.DeadlineExceeded: transient (a slow provider can succeed on retry)
|
|
|
|
|
// - an embedded HTTP status parsed from the error message: 5xx and 429 are
|
|
|
|
|
// transient (server / rate limit); other 4xx are permanent (auth, unknown
|
|
|
|
|
// model, invalid request)
|
|
|
|
|
// - anything else (network reset, connection refused, timeout): transient
|
|
|
|
|
//
|
|
|
|
|
// Provider drivers surface HTTP statuses as "API request failed with status
|
|
|
|
|
// <code>: <body>" (see BaseModel.doRequest), so parsing that prefix is enough.
|
|
|
|
|
func IsTransientError(err error) bool {
|
|
|
|
|
if err == nil {
|
|
|
|
|
return false
|
|
|
|
|
}
|
|
|
|
|
if errors.Is(err, context.Canceled) {
|
|
|
|
|
return false
|
|
|
|
|
}
|
|
|
|
|
if errors.Is(err, context.DeadlineExceeded) {
|
|
|
|
|
return true
|
|
|
|
|
}
|
|
|
|
|
msg := strings.ToLower(err.Error())
|
|
|
|
|
if idx := strings.Index(msg, "status "); idx >= 0 {
|
|
|
|
|
code := 0
|
|
|
|
|
for _, r := range msg[idx+len("status "):] {
|
|
|
|
|
if r < '0' || r > '9' {
|
|
|
|
|
break
|
|
|
|
|
}
|
|
|
|
|
code = code*10 + int(r-'0')
|
|
|
|
|
}
|
|
|
|
|
if code >= 500 || code == 429 {
|
|
|
|
|
return true
|
|
|
|
|
}
|
|
|
|
|
if code >= 400 && code < 500 {
|
|
|
|
|
return false
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
// No recognizable permanent marker: treat as transient (network, timeout).
|
|
|
|
|
return true
|
|
|
|
|
}
|