diff --git a/config.example.yaml b/config.example.yaml index 504e6f44..19507fbc 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -14,6 +14,14 @@ base_url: "http://localhost:8080" # Set to "0" to disable the timeout. Default: "30s". # http_timeout: "30s" +# Response write deadline, including download and scan time before sending data. +# Set to "0" to disable. Default: "5m". +# write_timeout: "5m" + +# Timeout for each upstream artifact download while serving requests; excludes +# scanning. Set to "0" to disable. Default: "5m". Does not limit proxy mirror. +# artifact_timeout: "5m" + # Largest upstream artifact to fetch (e.g. "2GB"). Larger ones are refused # and not cached. Default: unlimited. # artifact_max_size: "" diff --git a/docs/configuration.md b/docs/configuration.md index 33eb2ed7..569cff55 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -22,6 +22,22 @@ See `config.example.yaml` in the repository root for a complete example. | `trust_forwarded_for` | `PROXY_TRUST_FORWARDED_FOR` | - | `false` | Attribute requests to the leftmost `X-Forwarded-For` entry instead of the TCP peer address, in the structured log, the access log and the request-source table on `/ui/analytics`. | | `ui_request_sources` | `PROXY_UI_REQUEST_SOURCES` | - | `false` | Show the request-source table on `/ui/analytics`, which reports caller addresses, the tool each ran and how much each pulled. | | `ui_failure_details` | `PROXY_UI_FAILURE_DETAILS` | - | `false` | List the latest failures behind each failure count on `/ui/analytics`: the package, its upstream URL or storage path, and the error. URLs are shown without credentials or query strings. | +| `http_timeout` | `PROXY_HTTP_TIMEOUT` | - | `30s` | Timeout for upstream metadata and pass-through requests. | +| `write_timeout` | `PROXY_WRITE_TIMEOUT` | - | `5m` | Response write deadline, including download and scanning time before the response starts. | +| `artifact_timeout` | `PROXY_ARTIFACT_TIMEOUT` | - | `5m` | Timeout for each upstream artifact download while serving requests; excludes subsequent scanning. Does not limit `proxy mirror`. | + +These timeouts accept Go durations such as `30s`, `5m`, or `1h`. An omitted value uses the default; `"0"` disables that timeout. Invalid or negative values are rejected at startup. Environment variables override file values. + +For long scans, increase `write_timeout` to cover the download, scan, and response transfer. `scanning.timeout` controls the scan separately; changing it alone does not extend the response deadline. `artifact_timeout` is independent of both. For example: + +```yaml +write_timeout: "18m" +artifact_timeout: "10m" +scanning: + timeout: "16m" +``` + +These are independent limits, not reserved time slices: a download approaching 10 minutes followed by a 16-minute scan would still exceed an 18-minute response deadline. Any reverse proxy in front of the server also needs sufficient timeout allowance. The upstream transport's existing response-header timeout remains unchanged. ## Storage diff --git a/internal/config/config.go b/internal/config/config.go index 89a5c20c..6c1a6052 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -186,6 +186,15 @@ type Config struct { // response headers within 60 seconds. HTTPTimeout string `json:"http_timeout" yaml:"http_timeout"` + // WriteTimeout bounds response writes, including time spent downloading and + // scanning before the response starts. Default: "5m"; "0" disables it. + WriteTimeout string `json:"write_timeout" yaml:"write_timeout"` + + // ArtifactTimeout bounds each upstream artifact download while serving + // requests, excluding scanning. Default: "5m"; "0" disables it. + // The mirror command keeps its unlimited artifact download timeout. + ArtifactTimeout string `json:"artifact_timeout" yaml:"artifact_timeout"` + // MirrorAPI enables the /api/mirror endpoints for starting mirror jobs via HTTP. // Disabled by default to prevent unauthenticated users from triggering downloads. MirrorAPI bool `json:"mirror_api" yaml:"mirror_api"` @@ -1258,6 +1267,8 @@ func (c *Config) LoadFromEnv() { setEnvString(&c.ArtifactMaxSize, "PROXY_ARTIFACT_MAX_SIZE") setEnvString(&c.MetadataRewriteCacheSize, "PROXY_METADATA_REWRITE_CACHE_SIZE") setEnvString(&c.HTTPTimeout, "PROXY_HTTP_TIMEOUT") + setEnvString(&c.WriteTimeout, "PROXY_WRITE_TIMEOUT") + setEnvString(&c.ArtifactTimeout, "PROXY_ARTIFACT_TIMEOUT") c.setGradleReadOnlyFromEnv() setEnvString(&c.Gradle.BuildCache.MaxUploadSize, "PROXY_GRADLE_BUILD_CACHE_MAX_UPLOAD_SIZE") setEnvString(&c.Gradle.BuildCache.MaxAge, "PROXY_GRADLE_BUILD_CACHE_MAX_AGE") @@ -1357,7 +1368,7 @@ func (c *Config) Validate() error { return err } - if err := validateHTTPTimeout(c.HTTPTimeout); err != nil { + if err := c.validateRequestTimeouts(); err != nil { return err } @@ -1481,6 +1492,8 @@ func (g *GradleBuildCacheConfig) Validate() error { const ( defaultMetadataTTL = 5 * time.Minute //nolint:mnd // sensible default defaultDirectServeTTL = 15 * time.Minute //nolint:mnd // sensible default + defaultWriteTimeout = 5 * time.Minute + defaultArtifactTimeout = 5 * time.Minute defaultHTTPTimeout = 30 * time.Second //nolint:mnd // sensible default defaultHitFlushInterval = time.Second defaultMetadataMaxSize = 100 << 20 @@ -1509,16 +1522,30 @@ func (c *Config) ParseMaxSize() int64 { return size } -func validateHTTPTimeout(s string) error { +func (c *Config) validateRequestTimeouts() error { + for _, timeout := range []struct{ field, value string }{ + {"http_timeout", c.HTTPTimeout}, + {"write_timeout", c.WriteTimeout}, + {"artifact_timeout", c.ArtifactTimeout}, + } { + if err := validateRequestTimeout(timeout.field, timeout.value); err != nil { + return err + } + } + + return nil +} + +func validateRequestTimeout(field, s string) error { if s == "" || s == "0" { return nil } d, err := time.ParseDuration(s) if err != nil { - return fmt.Errorf("invalid http_timeout %q: %w", s, err) + return fmt.Errorf("invalid %s %q: %w", field, s, err) } if d < 0 { - return fmt.Errorf("invalid http_timeout %q: must be non-negative", s) + return fmt.Errorf("invalid %s %q: must be non-negative", field, s) } return nil } @@ -1617,15 +1644,28 @@ func (c *Config) ParseArtifactMaxSize() int64 { // ParseHTTPTimeout returns the upstream HTTP client timeout. // Returns 30s if unset, 0 (no timeout) if explicitly set to "0". func (c *Config) ParseHTTPTimeout() time.Duration { - if c.HTTPTimeout == "" { - return defaultHTTPTimeout - } - if c.HTTPTimeout == "0" { - return 0 + return parseRequestTimeout(c.HTTPTimeout, defaultHTTPTimeout) +} + +// ParseWriteTimeout returns the response write timeout, defaulting to 5m. +// An explicit zero disables the timeout. +func (c *Config) ParseWriteTimeout() time.Duration { + return parseRequestTimeout(c.WriteTimeout, defaultWriteTimeout) +} + +// ParseArtifactTimeout returns the upstream artifact download timeout for +// serving requests, defaulting to 5m. An explicit zero disables the timeout. +func (c *Config) ParseArtifactTimeout() time.Duration { + return parseRequestTimeout(c.ArtifactTimeout, defaultArtifactTimeout) +} + +func parseRequestTimeout(value string, fallback time.Duration) time.Duration { + if value == "" { + return fallback } - d, err := time.ParseDuration(c.HTTPTimeout) + d, err := time.ParseDuration(value) if err != nil || d < 0 { - return defaultHTTPTimeout + return fallback } return d } diff --git a/internal/config/request_timeouts_test.go b/internal/config/request_timeouts_test.go new file mode 100644 index 00000000..01848c67 --- /dev/null +++ b/internal/config/request_timeouts_test.go @@ -0,0 +1,76 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestRequestTimeouts(t *testing.T) { + for _, value := range []string{"", "0", "0s", "18m", "invalid", "-1s", "999999999999999999h"} { + t.Run(value, func(t *testing.T) { + cfg := Default() + cfg.WriteTimeout, cfg.ArtifactTimeout = value, value + valid := value == "" || value == "0" || value == "0s" || value == "18m" + want := 5 * time.Minute + if value == "0" || value == "0s" { + want = 0 + } + if value == "18m" { + want = 18 * time.Minute + } + if got := cfg.ParseWriteTimeout(); got != want { + t.Errorf("write timeout = %v, want %v", got, want) + } + if got := cfg.ParseArtifactTimeout(); got != want { + t.Errorf("artifact timeout = %v, want %v", got, want) + } + err := cfg.Validate() + if (err == nil) != valid { + t.Fatalf("Validate() = %v, valid = %v", err, valid) + } + if !valid { + if !strings.Contains(err.Error(), "write_timeout") { + t.Fatalf("error does not identify write_timeout: %v", err) + } + cfg.WriteTimeout = "" + if err := cfg.Validate(); err == nil || !strings.Contains(err.Error(), "artifact_timeout") { + t.Fatalf("artifact validation = %v", err) + } + } + }) + } +} + +func TestLoadRequestTimeouts(t *testing.T) { + for _, format := range []struct{ extension, data string }{ + {"yaml", "write_timeout: 18m\nartifact_timeout: 10m\nhttp_timeout: 90s\n"}, + {"json", `{"write_timeout":"18m","artifact_timeout":"10m","http_timeout":"90s"}`}, + } { + t.Run(format.extension, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config."+format.extension) + if err := os.WriteFile(path, []byte(format.data), 0600); err != nil { + t.Fatal(err) + } + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if cfg.ParseWriteTimeout() != 18*time.Minute || cfg.ParseArtifactTimeout() != 10*time.Minute || cfg.ParseHTTPTimeout() != 90*time.Second { + t.Fatalf("file timeouts = %v, %v, %v", cfg.WriteTimeout, cfg.ArtifactTimeout, cfg.HTTPTimeout) + } + t.Setenv("PROXY_WRITE_TIMEOUT", "20m") + t.Setenv("PROXY_ARTIFACT_TIMEOUT", "0") + t.Setenv("PROXY_HTTP_TIMEOUT", "15s") + cfg.LoadFromEnv() + if cfg.ParseWriteTimeout() != 20*time.Minute || cfg.ParseArtifactTimeout() != 0 || cfg.ParseHTTPTimeout() != 15*time.Second { + t.Fatalf("environment timeouts = %v, %v, %v", cfg.WriteTimeout, cfg.ArtifactTimeout, cfg.HTTPTimeout) + } + if err := cfg.Validate(); err != nil { + t.Fatal(err) + } + }) + } +} diff --git a/internal/server/request_timeouts_test.go b/internal/server/request_timeouts_test.go new file mode 100644 index 00000000..98b0a53b --- /dev/null +++ b/internal/server/request_timeouts_test.go @@ -0,0 +1,131 @@ +package server + +import ( + "context" + "errors" + "io" + "log/slog" + "net" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + "time" + + "github.com/git-pkgs/proxy/internal/config" +) + +// Exercise the real serve wiring: a slow artifact must use artifact_timeout, +// while waiting for it also consumes the independent response write deadline. +func TestServeRequestTimeouts(t *testing.T) { + for _, tc := range []struct { + name, write, artifact string + wantOK bool + wantConnectionError bool + }{ + {"independent from metadata timeout", "5s", "5s", true, false}, + {"write deadline expires while waiting", "1ms", "5s", false, true}, + {"artifact download times out", "5s", "1ms", false, false}, + {"both timeouts disabled", "0", "0", true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + status, body, err := fetchWithRequestTimeouts(t, tc.write, tc.artifact) + if tc.wantConnectionError { + if err == nil { + t.Fatal("response succeeded after its write deadline") + } + return + } + if err != nil { + t.Fatal(err) + } + if tc.wantOK { + if status != http.StatusOK || body != "release asset" { + t.Fatalf("response = %d %q", status, body) + } + } else if status < 500 { + t.Fatalf("artifact timeout response = %d %q, want server error", status, body) + } + }) + } +} + +func fetchWithRequestTimeouts(t *testing.T, writeTimeout, artifactTimeout string) (int, string, error) { + t.Helper() + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(100 * time.Millisecond) + _, _ = io.WriteString(w, "release asset") + })) + defer upstream.Close() + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer func() { _ = listener.Close() }() + dir := t.TempDir() + cfg := config.Default() + cfg.Listen = listener.Addr().String() + cfg.BaseURL = "http://" + cfg.Listen + cfg.Database.Path = filepath.Join(dir, "proxy.db") + cfg.Storage.URL = "file://" + filepath.Join(dir, "artifacts") + cfg.Upstream.Generic = map[string]string{"releases": upstream.URL} + cfg.Upstream.AllowLoopback = true + cfg.HTTPTimeout = "1ns" + cfg.WriteTimeout, cfg.ArtifactTimeout = writeTimeout, artifactTimeout + if err := cfg.Validate(); err != nil { + t.Fatal(err) + } + srv, err := New(cfg, slog.New(slog.NewTextHandler(io.Discard, nil)), BuildInfo{Version: "test"}) + if err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- srv.Start(listener) }() + defer func() { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := srv.Shutdown(ctx); err != nil { + t.Error(err) + } + if err := <-done; !errors.Is(err, http.ErrServerClosed) { + t.Errorf("Start() = %v", err) + } + }() + client := &http.Client{Timeout: 10 * time.Second} + defer client.CloseIdleConnections() + + resp, err := client.Get(cfg.BaseURL + "/generic/releases/owner/repo/releases/download/v1/asset.bin") + if err != nil { + return 0, "", err + } + defer func() { _ = resp.Body.Close() }() + body, err := io.ReadAll(resp.Body) + return resp.StatusCode, string(body), err +} + +func TestMirrorIgnoresServeArtifactTimeout(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(20 * time.Millisecond) + _, _ = io.WriteString(w, "mirrored artifact") + })) + defer upstream.Close() + db, store := setupEvictionTest(t) + defer func() { _ = store.Close() }() + cfg := config.Default() + cfg.Upstream.AllowLoopback = true + cfg.ArtifactTimeout = "1ns" + cfg.WriteTimeout = "1ns" + proxy, _ := NewMirrorProxy(cfg, db, store, slog.New(slog.NewTextHandler(io.Discard, nil))) + defer proxy.HTTPClient.CloseIdleConnections() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + result, err := proxy.GetOrFetchArtifactFromURL(ctx, "generic", "mirror-timeout", "v1", "asset.bin", upstream.URL+"/asset.bin") + if err != nil { + t.Fatal(err) + } + defer func() { _ = result.Reader.Close() }() + body, err := io.ReadAll(result.Reader) + if err != nil || string(body) != "mirrored artifact" { + t.Fatalf("mirror result = %q, %v", body, err) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 321633b4..5921cf0f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -90,11 +90,10 @@ import ( ) const ( - serverReadTimeout = 30 * time.Second - serverWriteTimeout = 5 * time.Minute - serverIdleTimeout = 60 * time.Second - dashboardTopN = 10 - hoursPerDay = 24 + serverReadTimeout = 30 * time.Second + serverIdleTimeout = 60 * time.Second + dashboardTopN = 10 + hoursPerDay = 24 // Upstream transport defaults, matching what fetch.NewFetcher would use // if we did not hand it our own client. Go's default transport keeps only @@ -226,7 +225,7 @@ func (s *Server) Start(listeners ...net.Listener) error { } func (s *Server) serve(listener net.Listener) error { - metadataClient, artifactClient := s.newUpstreamClients(serverWriteTimeout) + metadataClient, artifactClient := s.newUpstreamClients(s.cfg.ParseArtifactTimeout()) // Create shared components with circuit breaker. baseFetcher := fetch.NewFetcher(fetch.WithHTTPClient(artifactClient)) @@ -372,7 +371,7 @@ func (s *Server) serve(listener net.Listener) error { Addr: s.cfg.Listen, Handler: r, ReadTimeout: serverReadTimeout, - WriteTimeout: serverWriteTimeout, // Large artifacts need time + WriteTimeout: s.cfg.ParseWriteTimeout(), // Large artifacts need time IdleTimeout: serverIdleTimeout, }