From c5ce27009095a94b11d5b05849fd2a2f47bee0e2 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 4 May 2026 22:38:20 -0500 Subject: [PATCH] Added a configurable timeout knob --- README.md | 1 + internal/adapter/cli/run.go | 21 ++++--- internal/adapter/cli/run_test.go | 60 +++++++++++++++++++ internal/adapter/http/dto.go | 11 ++-- internal/adapter/http/handler.go | 11 ++-- internal/adapter/http/handler_test.go | 5 +- internal/domain/domain.go | 11 ++-- internal/llm/openai_compatible_client.go | 22 ++++++- internal/llm/openai_compatible_client_test.go | 49 +++++++++++++++ internal/profile/filesystem_repository.go | 3 + internal/profile/repository_test.go | 10 ++++ .../profile/testdata/negative_timeout.yaml | 10 ++++ internal/profile/testdata/valid.yaml | 1 + internal/usecase/runner.go | 3 + internal/usecase/runner_test.go | 31 +++++++--- 15 files changed, 215 insertions(+), 34 deletions(-) create mode 100644 internal/profile/testdata/negative_timeout.yaml diff --git a/README.md b/README.md index d7d178c..5f7df09 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,7 @@ Response shape: 1. Add a YAML file under `profiles/` with: - `id`, `version`, `expected_inputs`, `templates`, `model_defaults`, `output_format`, `validation` + - optional model timeout via `model_defaults.timeout_seconds` (per-run LLM timeout override) 2. Use template helpers such as `{{input "transcript"}}`. 3. For structured JSON output, set: - `output_format: json` diff --git a/internal/adapter/cli/run.go b/internal/adapter/cli/run.go index 120082c..7bbbfeb 100644 --- a/internal/adapter/cli/run.go +++ b/internal/adapter/cli/run.go @@ -40,6 +40,7 @@ type runConfig struct { temperature float64 maxTokens int schemaDir string + timeout time.Duration } type serveConfig struct { @@ -93,10 +94,13 @@ func runCommand(args []string, stdout, stderr io.Writer) int { fmt.Fprintf(stderr, "input parse error: %v\n", err) return ExitRuntimeError } - varMappings, err := parseMappings(cfg.varRaw, false) - if err != nil { - fmt.Fprintf(stderr, "var parse error: %v\n", err) - return ExitRuntimeError + varMappings := map[string]string{} + if len(cfg.varRaw) > 0 { + varMappings, err = parseMappings(cfg.varRaw, false) + if err != nil { + fmt.Fprintf(stderr, "var parse error: %v\n", err) + return ExitRuntimeError + } } inputs := make(map[string]domain.ArtifactRef, len(inputMappings)) @@ -108,7 +112,7 @@ func runCommand(args []string, stdout, stderr io.Writer) int { BaseURL: cfg.llmBaseURL, APIKey: cfg.llmAPIKey, Model: cfg.model, - Timeout: 60 * time.Second, + Timeout: cfg.timeout, }) if err != nil { fmt.Fprintf(stderr, "llm client error: %v\n", err) @@ -205,6 +209,7 @@ func parseRunArgs(args []string) (*runConfig, error) { fs.Float64Var(&cfg.temperature, "temperature", 0, "optional temperature override") fs.IntVar(&cfg.maxTokens, "max-tokens", 0, "optional max tokens override") fs.StringVar(&cfg.schemaDir, "schema-dir", ".", "base directory for validation schemas") + fs.DurationVar(&cfg.timeout, "timeout", 10*time.Minute, "LLM request timeout") if err := fs.Parse(args); err != nil { return nil, err @@ -249,7 +254,7 @@ func parseServeArgs(args []string) (*serveConfig, error) { fs.StringVar(&cfg.llmBaseURL, "llm-base-url", "", "OpenAI-compatible base URL including /v1") fs.StringVar(&cfg.llmAPIKey, "llm-api-key", "", "optional API key") fs.StringVar(&cfg.model, "model", "", "optional default model") - fs.DurationVar(&cfg.timeout, "timeout", 60*time.Second, "LLM request timeout") + fs.DurationVar(&cfg.timeout, "timeout", 10*time.Minute, "LLM request timeout") if err := fs.Parse(args); err != nil { return nil, err @@ -346,6 +351,6 @@ func printSummary(stderr io.Writer, res *domain.RunResult) { func printUsage(w io.Writer) { fmt.Fprintln(w, "usage: scriptorium ...") - fmt.Fprintln(w, " run: scriptorium run --profile-dir DIR --profile-id ID --input name=path [--input ...] --llm-base-url URL --model NAME [--var k=v] [--out path]") - fmt.Fprintln(w, " serve: scriptorium serve --addr :8080 --profile-dir DIR --llm-base-url URL [--schema-dir DIR] [--llm-api-key KEY] [--model NAME]") + fmt.Fprintln(w, " run: scriptorium run --profile-dir DIR --profile-id ID --input name=path [--input ...] --llm-base-url URL --model NAME [--var k=v] [--out path] [--timeout 10m]") + fmt.Fprintln(w, " serve: scriptorium serve --addr :8080 --profile-dir DIR --llm-base-url URL [--schema-dir DIR] [--llm-api-key KEY] [--model NAME] [--timeout 10m]") } diff --git a/internal/adapter/cli/run_test.go b/internal/adapter/cli/run_test.go index 1592999..bc5bab6 100644 --- a/internal/adapter/cli/run_test.go +++ b/internal/adapter/cli/run_test.go @@ -1,8 +1,11 @@ package cli import ( + "bytes" "errors" + "strings" "testing" + "time" "gitea.maximumdirect.net/eric/scriptorium/internal/domain" ) @@ -92,6 +95,40 @@ func TestParseServeArgsRequiredFlags(t *testing.T) { if cfg.addr != ":8080" { t.Fatalf("expected default addr :8080, got %q", cfg.addr) } + if cfg.timeout != 10*time.Minute { + t.Fatalf("expected default timeout 10m, got %s", cfg.timeout) + } +} + +func TestParseRunArgsTimeout(t *testing.T) { + cfg, err := parseRunArgs([]string{ + "--profile-dir", "./profiles", + "--profile-id", "p", + "--input", "a=b", + "--llm-base-url", "http://x/v1", + "--model", "m", + }) + if err != nil { + t.Fatalf("expected valid run args, got %v", err) + } + if cfg.timeout != 10*time.Minute { + t.Fatalf("expected default timeout 10m, got %s", cfg.timeout) + } + + cfg, err = parseRunArgs([]string{ + "--profile-dir", "./profiles", + "--profile-id", "p", + "--input", "a=b", + "--llm-base-url", "http://x/v1", + "--model", "m", + "--timeout", "2m30s", + }) + if err != nil { + t.Fatalf("expected valid run args with timeout override, got %v", err) + } + if cfg.timeout != 2*time.Minute+30*time.Second { + t.Fatalf("expected timeout override 2m30s, got %s", cfg.timeout) + } } func TestDetermineExitCode(t *testing.T) { @@ -108,3 +145,26 @@ func TestDetermineExitCode(t *testing.T) { t.Fatalf("expected success exit code for skipped validation, got %d", got) } } + +func TestRunCommandVarsOptional(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := runCommand([]string{ + "--profile-dir", "./profiles", + "--profile-id", "p", + "--input", "transcript=./t.md", + "--llm-base-url", "://bad-url", + "--model", "m", + }, &stdout, &stderr) + + if code != ExitRuntimeError { + t.Fatalf("expected runtime error exit code, got %d", code) + } + if strings.Contains(stderr.String(), "var parse error") { + t.Fatalf("expected --var to be optional, got stderr=%q", stderr.String()) + } + if !strings.Contains(stderr.String(), "llm client error") { + t.Fatalf("expected llm client error after parsing succeeds, got stderr=%q", stderr.String()) + } +} diff --git a/internal/adapter/http/dto.go b/internal/adapter/http/dto.go index 74ca1df..f12d2ff 100644 --- a/internal/adapter/http/dto.go +++ b/internal/adapter/http/dto.go @@ -21,11 +21,12 @@ type inputRefDTO struct { } type modelOverrideRequestDTO struct { - Endpoint string `json:"endpoint,omitempty"` - Model string `json:"model,omitempty"` - Temperature float64 `json:"temperature,omitempty"` - MaxTokens int `json:"max_tokens,omitempty"` - TopP float64 `json:"top_p,omitempty"` + Endpoint string `json:"endpoint,omitempty"` + Model string `json:"model,omitempty"` + Temperature float64 `json:"temperature,omitempty"` + MaxTokens int `json:"max_tokens,omitempty"` + TopP float64 `json:"top_p,omitempty"` + TimeoutSeconds int `json:"timeout_seconds,omitempty"` } type runResponseDTO struct { diff --git a/internal/adapter/http/handler.go b/internal/adapter/http/handler.go index 252305e..c947e1d 100644 --- a/internal/adapter/http/handler.go +++ b/internal/adapter/http/handler.go @@ -62,11 +62,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { var model *domain.ModelTarget if req.Model != nil { model = &domain.ModelTarget{ - Endpoint: req.Model.Endpoint, - Model: req.Model.Model, - Temperature: req.Model.Temperature, - MaxTokens: req.Model.MaxTokens, - TopP: req.Model.TopP, + Endpoint: req.Model.Endpoint, + Model: req.Model.Model, + Temperature: req.Model.Temperature, + MaxTokens: req.Model.MaxTokens, + TopP: req.Model.TopP, + TimeoutSeconds: req.Model.TimeoutSeconds, } } diff --git a/internal/adapter/http/handler_test.go b/internal/adapter/http/handler_test.go index e45bfb9..94e5515 100644 --- a/internal/adapter/http/handler_test.go +++ b/internal/adapter/http/handler_test.go @@ -61,7 +61,7 @@ func TestHandlerPostRunsSuccess(t *testing.T) { "transcript": {"type": "file", "uri": "./t.md"} }, "vars": {"k": "v"}, - "model": {"model": "gpt-x"} + "model": {"model": "gpt-x", "timeout_seconds": 120} }`) req := httptest.NewRequest(http.MethodPost, "/v1/runs", bytes.NewReader(body)) w := httptest.NewRecorder() @@ -91,6 +91,9 @@ func TestHandlerPostRunsSuccess(t *testing.T) { if r.last.Model == nil || r.last.Model.Model != "gpt-x" { t.Fatalf("expected model override, got %#v", r.last.Model) } + if r.last.Model.TimeoutSeconds != 120 { + t.Fatalf("expected timeout_seconds override 120, got %#v", r.last.Model) + } } func TestHandlerInvalidJSON(t *testing.T) { diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 43e47c3..97038c1 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -106,11 +106,12 @@ type PromptMessageTemplate struct { // ModelTarget represents the LLM endpoint and configuration. type ModelTarget struct { - Endpoint string `yaml:"endpoint"` - Model string `yaml:"model"` - Temperature float64 `yaml:"temperature"` - MaxTokens int `yaml:"max_tokens"` - TopP float64 `yaml:"top_p"` + Endpoint string `yaml:"endpoint"` + Model string `yaml:"model"` + Temperature float64 `yaml:"temperature"` + MaxTokens int `yaml:"max_tokens"` + TopP float64 `yaml:"top_p"` + TimeoutSeconds int `yaml:"timeout_seconds"` } // OutputContract defines the requirements for the output artifact. diff --git a/internal/llm/openai_compatible_client.go b/internal/llm/openai_compatible_client.go index 900912a..09fd329 100644 --- a/internal/llm/openai_compatible_client.go +++ b/internal/llm/openai_compatible_client.go @@ -49,7 +49,7 @@ func NewOpenAICompatibleClient(cfg OpenAICompatibleConfig) (*OpenAICompatibleCli timeout := cfg.Timeout if timeout <= 0 { - timeout = 30 * time.Second + timeout = 10 * time.Minute } var client *http.Client @@ -72,6 +72,10 @@ func NewOpenAICompatibleClient(cfg OpenAICompatibleConfig) (*OpenAICompatibleCli } func (c *OpenAICompatibleClient) Generate(ctx context.Context, req domain.GenerateRequest) (*domain.GenerateResponse, error) { + if req.Target.TimeoutSeconds < 0 { + return nil, fmt.Errorf("%w: timeout_seconds must be greater than or equal to 0", ErrInvalidRequest) + } + model := strings.TrimSpace(req.Target.Model) if model == "" { model = strings.TrimSpace(c.defaultModel) @@ -122,7 +126,21 @@ func (c *OpenAICompatibleClient) Generate(ctx context.Context, req domain.Genera httpReq.Header.Set("Authorization", "Bearer "+c.apiKey) } - httpResp, err := c.httpClient.Do(httpReq) + effectiveTimeout := c.timeout + if req.Target.TimeoutSeconds > 0 { + effectiveTimeout = time.Duration(req.Target.TimeoutSeconds) * time.Second + } + + httpClient := c.httpClient + if httpClient == nil { + httpClient = &http.Client{Timeout: effectiveTimeout} + } else if httpClient.Timeout != effectiveTimeout { + cloned := *httpClient + cloned.Timeout = effectiveTimeout + httpClient = &cloned + } + + httpResp, err := httpClient.Do(httpReq) if err != nil { return nil, fmt.Errorf("%w: %v", ErrRequestFailed, err) } diff --git a/internal/llm/openai_compatible_client_test.go b/internal/llm/openai_compatible_client_test.go index 0aa65cb..b028975 100644 --- a/internal/llm/openai_compatible_client_test.go +++ b/internal/llm/openai_compatible_client_test.go @@ -287,3 +287,52 @@ func TestOpenAICompatibleClientTimeout(t *testing.T) { t.Fatalf("expected ErrRequestFailed, got %v", err) } } + +func TestOpenAICompatibleClientRequestTimeoutOverride(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(100 * time.Millisecond) + _, _ = w.Write([]byte(`{"choices":[{"message":{"content":"ok"}}]}`)) + })) + defer ts.Close() + + client, err := NewOpenAICompatibleClient(OpenAICompatibleConfig{ + BaseURL: ts.URL + "/v1", + Model: "m", + Timeout: 50 * time.Millisecond, + }) + if err != nil { + t.Fatal(err) + } + + resp, err := client.Generate(context.Background(), domain.GenerateRequest{ + Prompt: domain.RenderedPrompt{Messages: []domain.RenderedMessage{{Role: "user", Content: "hi"}}}, + Target: domain.ModelTarget{TimeoutSeconds: 1}, + }) + if err != nil { + t.Fatalf("expected request-level timeout override to succeed, got %v", err) + } + if resp.Content != "ok" { + t.Fatalf("expected response content ok, got %q", resp.Content) + } +} + +func TestOpenAICompatibleClientNegativeTimeoutRejected(t *testing.T) { + client, err := NewOpenAICompatibleClient(OpenAICompatibleConfig{ + BaseURL: "http://example.com/v1", + Model: "m", + }) + if err != nil { + t.Fatal(err) + } + + _, err = client.Generate(context.Background(), domain.GenerateRequest{ + Prompt: domain.RenderedPrompt{Messages: []domain.RenderedMessage{{Role: "user", Content: "hi"}}}, + Target: domain.ModelTarget{TimeoutSeconds: -1}, + }) + if err == nil { + t.Fatal("expected invalid request error") + } + if !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } +} diff --git a/internal/profile/filesystem_repository.go b/internal/profile/filesystem_repository.go index 5bdaef1..91dfefb 100644 --- a/internal/profile/filesystem_repository.go +++ b/internal/profile/filesystem_repository.go @@ -107,6 +107,9 @@ func validateProfile(p *domain.PromptProfile) error { if p.Validation.ValidationMode == domain.ValidationJSONSchema && strings.TrimSpace(p.Validation.SchemaPath) == "" { return errors.New("validation.schema_path is required when validation_mode is json_schema") } + if p.ModelDefaults.TimeoutSeconds < 0 { + return errors.New("model_defaults.timeout_seconds must be greater than or equal to 0") + } if p.Validation.Format != "" && p.Validation.Format != p.OutputFormat { return fmt.Errorf("validation format %q does not match output format %q", p.Validation.Format, p.OutputFormat) } diff --git a/internal/profile/repository_test.go b/internal/profile/repository_test.go index 1e887aa..3dee818 100644 --- a/internal/profile/repository_test.go +++ b/internal/profile/repository_test.go @@ -64,6 +64,9 @@ func TestFilesystemRepository_GetProfile(t *testing.T) { if p.Validation.ValidationMode != domain.ValidationBasic { t.Fatalf("expected validation mode basic, got %q", p.Validation.ValidationMode) } + if p.ModelDefaults.TimeoutSeconds != 120 { + t.Fatalf("expected timeout_seconds 120, got %d", p.ModelDefaults.TimeoutSeconds) + } }) t.Run("invalid YAML", func(t *testing.T) { @@ -94,6 +97,13 @@ func TestFilesystemRepository_GetProfile(t *testing.T) { } }) + t.Run("negative timeout seconds", func(t *testing.T) { + _, err := repo.GetProfile(ctx, "negative-timeout", "") + if !errors.Is(err, ErrInvalidProfile) { + t.Errorf("expected ErrInvalidProfile for negative timeout_seconds, got %v", err) + } + }) + t.Run("profile not found", func(t *testing.T) { _, err := repo.GetProfile(ctx, "unknown", "") if !errors.Is(err, ErrProfileNotFound) { diff --git a/internal/profile/testdata/negative_timeout.yaml b/internal/profile/testdata/negative_timeout.yaml new file mode 100644 index 0000000..50fbc07 --- /dev/null +++ b/internal/profile/testdata/negative_timeout.yaml @@ -0,0 +1,10 @@ +id: negative-timeout +version: "1.0.0" +templates: + - role: user + content: "Say hi" +model_defaults: + timeout_seconds: -1 +output_format: text +validation: + validation_mode: none diff --git a/internal/profile/testdata/valid.yaml b/internal/profile/testdata/valid.yaml index 1881ba4..566e67f 100644 --- a/internal/profile/testdata/valid.yaml +++ b/internal/profile/testdata/valid.yaml @@ -12,6 +12,7 @@ templates: model_defaults: model: gpt-4o temperature: 0.7 + timeout_seconds: 120 output_format: markdown validation: validation_mode: basic diff --git a/internal/usecase/runner.go b/internal/usecase/runner.go index 6d82b70..1eb1fda 100644 --- a/internal/usecase/runner.go +++ b/internal/usecase/runner.go @@ -215,6 +215,9 @@ func mergeModelTarget(base domain.ModelTarget, override *domain.ModelTarget) dom if override.TopP != 0 { out.TopP = override.TopP } + if override.TimeoutSeconds != 0 { + out.TimeoutSeconds = override.TimeoutSeconds + } return out } diff --git a/internal/usecase/runner_test.go b/internal/usecase/runner_test.go index 929188b..c9a9ddd 100644 --- a/internal/usecase/runner_test.go +++ b/internal/usecase/runner_test.go @@ -117,11 +117,12 @@ func TestRunnerRunSuccessful(t *testing.T) { Version: "1.0.0", OutputFormat: domain.FormatMarkdown, ModelDefaults: domain.ModelTarget{ - Endpoint: "ep1", - Model: "model-default", - Temperature: 0.4, - MaxTokens: 200, - TopP: 0.9, + Endpoint: "ep1", + Model: "model-default", + Temperature: 0.4, + MaxTokens: 200, + TopP: 0.9, + TimeoutSeconds: 90, }, Validation: domain.OutputContract{ ValidationMode: domain.ValidationBasic, @@ -158,9 +159,10 @@ func TestRunnerRunSuccessful(t *testing.T) { "glossary": {Type: domain.ArtifactRefFile, URI: "a://g"}, }, Model: &domain.ModelTarget{ - Model: "model-override", - Temperature: 0, - MaxTokens: 0, + Model: "model-override", + Temperature: 0, + MaxTokens: 0, + TimeoutSeconds: 0, }, }) if err != nil { @@ -214,6 +216,9 @@ func TestRunnerRunSuccessful(t *testing.T) { if llmClient.lastReq.Target.Temperature != 0.4 { t.Fatalf("expected zero-valued request field not to override default temperature, got %v", llmClient.lastReq.Target.Temperature) } + if llmClient.lastReq.Target.TimeoutSeconds != 90 { + t.Fatalf("expected zero-valued request timeout not to override default timeout, got %d", llmClient.lastReq.Target.TimeoutSeconds) + } } func TestRunnerRunProfileLoadFailure(t *testing.T) { @@ -231,6 +236,16 @@ func TestRunnerRunProfileLoadFailure(t *testing.T) { } } +func TestMergeModelTargetTimeoutOverride(t *testing.T) { + base := domain.ModelTarget{TimeoutSeconds: 30} + override := &domain.ModelTarget{TimeoutSeconds: 75} + + got := mergeModelTarget(base, override) + if got.TimeoutSeconds != 75 { + t.Fatalf("expected timeout override to apply, got %d", got.TimeoutSeconds) + } +} + func TestRunnerRunArtifactLoadFailure(t *testing.T) { runner := NewRunner( &fakeProfileRepo{profile: minimalProfile()},