Merge pull request #4593 from StarryKira/fix/image-storage-env-unreachable

fix: 异步生图开关配了却不生效（环境变量被静默丢弃 + 迁移到后台开关）
2026-10-27 00:04:32 +00:00 · 2026-07-20 09:20:15 +08:00
parent f3312cf7d9 6102d64274
commit bfabfe60c8
18 changed files with 1324 additions and 35 deletions
@@ -257,6 +257,22 @@ func (c *ImageStorageConfig) Active() bool {
 	return c.Enabled && c.IsConfigured()
 }

+// MissingCredentialKeys 返回 IsConfigured 所缺的配置键名。
+// 用于启动日志：只说"凭证不完整"会让运维以为自己漏填了，而实际可能是值填了却没被读到。
+func (c *ImageStorageConfig) MissingCredentialKeys() []string {
+	var missing []string
+	if c.Bucket == "" {
+		missing = append(missing, "image_storage.bucket")
+	}
+	if c.AccessKeyID == "" {
+		missing = append(missing, "image_storage.access_key_id")
+	}
+	if c.SecretAccessKey == "" {
+		missing = append(missing, "image_storage.secret_access_key")
+	}
+	return missing
+}
+
 type LinuxDoConnectConfig struct {
 	Enabled             bool   `mapstructure:"enabled"`
 	ClientID            string `mapstructure:"client_id"`
@@ -1656,6 +1672,9 @@ func load(allowMissingJWTSecret bool) (*Config, error) {
 	if cfg.Gateway.OpenAIScheduler.StickyEscapeErrorRate == 0 {
 		cfg.Gateway.OpenAIScheduler.StickyEscapeErrorRate = 0.5
 	}
+	// Kept as a backstop: setEnvReachableDefaults now registers this key with its
+	// effective default (true), so IsSet always reports true and this branch no
+	// longer fires. It still guards the default if that registration is dropped.
 	if !cfg.Gateway.OpenAIScheduler.StickyEscapeEnabled && !viper.IsSet("gateway.openai_scheduler.sticky_escape_enabled") {
 		cfg.Gateway.OpenAIScheduler.StickyEscapeEnabled = true
 	}
@@ -2025,6 +2044,15 @@ func setDefaults() {
 	viper.SetDefault("image_storage.force_path_style", false)
 	viper.SetDefault("image_storage.presign_expiry_hours", 24)
 	viper.SetDefault("image_storage.max_download_bytes", 33554432)
+	// Registered with empty defaults so AutomaticEnv can reach them: viper only
+	// decodes keys present in AllKeys(), so a credential that is supplied purely
+	// via IMAGE_STORAGE_* and never appears in config.yaml would be dropped and
+	// silently disable the whole async image feature.
+	viper.SetDefault("image_storage.endpoint", "")
+	viper.SetDefault("image_storage.bucket", "")
+	viper.SetDefault("image_storage.access_key_id", "")
+	viper.SetDefault("image_storage.secret_access_key", "")
+	viper.SetDefault("image_storage.public_base_url", "")

 	// Ops (vNext)
 	viper.SetDefault("ops.enabled", true)
@@ -2311,6 +2339,76 @@ func setDefaults() {
 	viper.SetDefault("subscription_maintenance.worker_count", 2)
 	viper.SetDefault("subscription_maintenance.queue_size", 1024)

+	setEnvReachableDefaults()
+}
+
+// setEnvReachableDefaults registers zero-valued defaults for keys that are
+// documented in deploy/config.example.yaml but had no default of their own.
+//
+// viper.Unmarshal only decodes the keys returned by AllKeys(), which unions
+// SetDefault keys, config-file keys and explicitly bound BindEnv keys.
+// AutomaticEnv can override a key already in that union, but it never adds one,
+// and the viper_bind_struct escape hatch is compiled out (we build with
+// -tags embed). So a key that lives only in the example file was unreachable by
+// environment variable: the value was read from the process environment and
+// then silently dropped. Deployments driven purely by env — which is what
+// deploy/docker-compose.yml does — got the zero value with no warning.
+//
+// The values below are deliberately zero rather than the documented example
+// values: an absent key already unmarshalled to the zero value, so registering
+// zero keeps behavior identical while making the key addressable from the
+// environment. Any subsystem that wants a richer default still applies it after
+// unmarshal, exactly as before.
+func setEnvReachableDefaults() {
+	viper.SetDefault("gateway.forced_codex_instructions_template_file", "")
+	viper.SetDefault("gateway.session_idle_timeout_minutes", 0)
+	viper.SetDefault("gateway.user_message_queue.mode", "")
+	viper.SetDefault("update.proxy_url", "")
+
+	// sticky_escape_enabled is the one exception to the zero-value rule: its
+	// effective default is true, applied post-unmarshal via a viper.IsSet guard.
+	// Registering false would make IsSet always report true and permanently
+	// disable sticky escape, so register the effective default instead. An
+	// explicit false in config or env still wins.
+	viper.SetDefault("gateway.openai_scheduler.sticky_escape_enabled", true)
+	viper.SetDefault("gateway.openai_scheduler.sticky_escape_error_rate", 0.0)
+	viper.SetDefault("gateway.openai_scheduler.sticky_escape_ttft_ms", 0)
+
+	// Third-party login providers. These carry client secrets and are exactly
+	// the settings an operator expects to inject via the environment, but every
+	// key here was previously unreachable that way.
+	for _, provider := range []string{"github_oauth", "google_oauth"} {
+		viper.SetDefault(provider+".enabled", false)
+		viper.SetDefault(provider+".client_id", "")
+		viper.SetDefault(provider+".client_secret", "")
+		viper.SetDefault(provider+".authorize_url", "")
+		viper.SetDefault(provider+".token_url", "")
+		viper.SetDefault(provider+".userinfo_url", "")
+		viper.SetDefault(provider+".emails_url", "")
+		viper.SetDefault(provider+".scopes", "")
+		viper.SetDefault(provider+".redirect_url", "")
+		viper.SetDefault(provider+".frontend_redirect_url", "")
+	}
+
+	viper.SetDefault("dingtalk_connect.client_id", "")
+	viper.SetDefault("dingtalk_connect.client_secret", "")
+	viper.SetDefault("dingtalk_connect.internal_corp_id", "")
+	viper.SetDefault("dingtalk_connect.redirect_url", "")
+	viper.SetDefault("dingtalk_connect.bypass_registration", false)
+	viper.SetDefault("dingtalk_connect.username_attribute_key", "")
+	viper.SetDefault("dingtalk_connect.enable_attribute_matching", false)
+	viper.SetDefault("dingtalk_connect.enable_attribute_sync", false)
+	viper.SetDefault("dingtalk_connect.attribute_sync_fields", []string{})
+	viper.SetDefault("dingtalk_connect.attribute_sync_overwrite_policy", "")
+	viper.SetDefault("dingtalk_connect.sync_display_name", false)
+	viper.SetDefault("dingtalk_connect.sync_display_name_attr_key", "")
+	viper.SetDefault("dingtalk_connect.sync_display_name_attr_name", "")
+	viper.SetDefault("dingtalk_connect.sync_dept", false)
+	viper.SetDefault("dingtalk_connect.sync_dept_attr_key", "")
+	viper.SetDefault("dingtalk_connect.sync_dept_attr_name", "")
+	viper.SetDefault("dingtalk_connect.sync_corp_email", false)
+	viper.SetDefault("dingtalk_connect.sync_corp_email_attr_key", "")
+	viper.SetDefault("dingtalk_connect.sync_corp_email_attr_name", "")
 }

 func (c *Config) Validate() error {
@@ -0,0 +1,90 @@
+//go:build unit
+
+package config
+
+import (
+	"reflect"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/spf13/viper"
+)
+
+// collectMapstructureKeys walks a config struct and returns every dotted key
+// viper would need in order to populate it.
+func collectMapstructureKeys(t reflect.Type, prefix string, out map[string]string) {
+	for i := 0; i < t.NumField(); i++ {
+		field := t.Field(i)
+		if field.PkgPath != "" {
+			continue // unexported
+		}
+		tag := field.Tag.Get("mapstructure")
+		name, _, _ := strings.Cut(tag, ",")
+		if name == "-" {
+			continue
+		}
+		if name == "" {
+			name = strings.ToLower(field.Name)
+		}
+		key := name
+		if prefix != "" {
+			key = prefix + "." + name
+		}
+
+		ft := field.Type
+		for ft.Kind() == reflect.Ptr {
+			ft = ft.Elem()
+		}
+		if ft.Kind() == reflect.Struct {
+			collectMapstructureKeys(ft, key, out)
+			continue
+		}
+		if ft.Kind() == reflect.Map {
+			// A map cannot be expressed in a single environment variable, so it
+			// is out of scope here — such settings need a config file either way.
+			continue
+		}
+		out[strings.ToLower(key)] = ft.String()
+	}
+}
+
+// TestConfigKeysAreEnvReachable is the systemic guard behind the image_storage
+// bug: viper.Unmarshal only decodes keys returned by AllKeys(), which unions
+// SetDefault keys, config-file keys and explicit BindEnv keys. AutomaticEnv can
+// override a key already in that union but never introduces one, and the
+// viper_bind_struct escape hatch is compiled out (we build with -tags embed).
+//
+// So a Config field with no registered default is unreachable by environment
+// variable whenever the deployment has no config.yaml containing it — the
+// operator sets the variable, the loader discards it, and the feature behaves
+// as if it were never configured. That is exactly how image_storage credentials
+// were lost, silently disabling async image tasks for env-driven deployments.
+//
+// When this fails, register a zero-valued default in setEnvReachableDefaults
+// for each reported key.
+func TestConfigKeysAreEnvReachable(t *testing.T) {
+	bound := map[string]string{}
+	collectMapstructureKeys(reflect.TypeOf(Config{}), "", bound)
+
+	viper.Reset()
+	t.Cleanup(viper.Reset)
+	setDefaults()
+	registered := map[string]struct{}{}
+	for _, key := range viper.AllKeys() {
+		registered[key] = struct{}{}
+	}
+
+	var unreachable []string
+	for key, kind := range bound {
+		if _, ok := registered[key]; !ok {
+			unreachable = append(unreachable, key+" ("+kind+")")
+		}
+	}
+	sort.Strings(unreachable)
+
+	if len(unreachable) > 0 {
+		t.Fatalf("%d config keys have no default registered, so their environment variables are silently ignored:\n  %s",
+			len(unreachable), strings.Join(unreachable, "\n  "))
+	}
+}
@@ -0,0 +1,41 @@
+//go:build unit
+
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestLoadImageStorageFromEnv guards against a viper trap that silently disabled
+// asynchronous image tasks for every environment-variable-only deployment.
+//
+// viper only decodes keys returned by AllKeys(), which unions SetDefault keys,
+// config-file keys and explicit BindEnv keys. AutomaticEnv can override a key
+// that is already in that list, but it never introduces a new one. Credentials
+// such as image_storage.bucket therefore need an (empty) default registered, or
+// IMAGE_STORAGE_BUCKET is dropped on the floor and Active() stays false while
+// image_storage.enabled reads true — the endpoints 404 with no useful signal.
+func TestLoadImageStorageFromEnv(t *testing.T) {
+	resetViperWithJWTSecret(t)
+	t.Setenv("IMAGE_STORAGE_ENABLED", "true")
+	t.Setenv("IMAGE_STORAGE_ENDPOINT", "https://acct.r2.cloudflarestorage.com")
+	t.Setenv("IMAGE_STORAGE_BUCKET", "my-images")
+	t.Setenv("IMAGE_STORAGE_ACCESS_KEY_ID", "ak")
+	t.Setenv("IMAGE_STORAGE_SECRET_ACCESS_KEY", "sk")
+	t.Setenv("IMAGE_STORAGE_PUBLIC_BASE_URL", "https://cdn.example.com")
+
+	cfg, err := Load()
+	require.NoError(t, err)
+
+	require.True(t, cfg.ImageStorage.Enabled)
+	require.Equal(t, "https://acct.r2.cloudflarestorage.com", cfg.ImageStorage.Endpoint)
+	require.Equal(t, "my-images", cfg.ImageStorage.Bucket)
+	require.Equal(t, "ak", cfg.ImageStorage.AccessKeyID)
+	require.Equal(t, "sk", cfg.ImageStorage.SecretAccessKey)
+	require.Equal(t, "https://cdn.example.com", cfg.ImageStorage.PublicBaseURL)
+
+	require.True(t, cfg.ImageStorage.IsConfigured())
+	require.True(t, cfg.ImageStorage.Active(), "async image tasks must be active when every credential is supplied via env")
+}