diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index def68f3a2..2db2418c4 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -255,6 +255,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"` @@ -1562,6 +1578,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 } @@ -1926,6 +1945,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) @@ -2205,6 +2233,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 { diff --git a/backend/internal/config/env_reachability_test.go b/backend/internal/config/env_reachability_test.go new file mode 100644 index 000000000..8141a2ec3 --- /dev/null +++ b/backend/internal/config/env_reachability_test.go @@ -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 ")) + } +} diff --git a/backend/internal/config/image_storage_env_test.go b/backend/internal/config/image_storage_env_test.go new file mode 100644 index 000000000..8061ee2ee --- /dev/null +++ b/backend/internal/config/image_storage_env_test.go @@ -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") +} diff --git a/backend/internal/service/wire.go b/backend/internal/service/wire.go index 59037d188..8c6abd899 100644 --- a/backend/internal/service/wire.go +++ b/backend/internal/service/wire.go @@ -12,6 +12,7 @@ import ( "github.com/Wei-Shaw/sub2api/internal/pkg/logger" "github.com/google/wire" "github.com/redis/go-redis/v9" + "go.uber.org/zap" ) // BuildInfo contains build information @@ -529,7 +530,10 @@ func ProvideAPIKeyAuthCacheInvalidator(apiKeyService *APIKeyService) APIKeyAuthC func ProvideImageTaskService(store ImageTaskStore, storage ImageStorage, cfg *config.Config) *ImageTaskService { if !cfg.ImageStorage.Active() { if cfg.ImageStorage.Enabled { - logger.L().Warn("image_storage.enabled is true but object storage is not fully configured; async image tasks are disabled") + // 列出具体缺失的键。若这些键其实已在环境变量里设过，说明它们没被读进来， + // 请确认 setDefaults 中已为其注册默认值（见 config.setEnvReachableDefaults）。 + logger.L().Warn("image_storage.enabled is true but object storage is not fully configured; async image tasks are disabled", + zap.Strings("missing_keys", cfg.ImageStorage.MissingCredentialKeys())) } return NewImageTaskService(store) } diff --git a/docs/ASYNC_IMAGE_TASKS.md b/docs/ASYNC_IMAGE_TASKS.md index 4c6b744e9..12793f2a7 100644 --- a/docs/ASYNC_IMAGE_TASKS.md +++ b/docs/ASYNC_IMAGE_TASKS.md @@ -41,6 +41,22 @@ When a task completes, each generated image is uploaded to the bucket and the re To support a different vendor beyond the S3-compatible client, implement the `service.ImageStorage` interface (`Save(ctx, key, contentType, data) (url, error)`) and provide it in place of the S3 implementation. +### Troubleshooting: the endpoints return 404 after enabling + +`404 async image tasks are not enabled` means `image_storage` did not resolve to a complete configuration, so the feature stayed off. The route exists either way — the 404 comes from the handler, not from an unregistered path, which makes it easy to mistake for a missing build. + +Check the startup log for: + +```text +WARN image_storage.enabled is true but object storage is not fully configured; async image tasks are disabled missing_keys=[...] +``` + +`missing_keys` names exactly which credentials were empty when the config was loaded. + +Note that releases **before v0.1.161 silently dropped `IMAGE_STORAGE_ENDPOINT`, `_BUCKET`, `_ACCESS_KEY_ID`, `_SECRET_ACCESS_KEY` and `_PUBLIC_BASE_URL`** when they were supplied only through the environment: those keys had no registered default, and viper cannot see an environment variable for a key it does not already know about. Deployments driven purely by `environment:` — which is what `deploy/docker-compose.yml` does by default — therefore reported `enabled: true` with empty credentials and 404'd on every async call. On an affected release the workaround is to also place the `image_storage` block in `/app/data/config.yaml` (copy it from `deploy/config.example.yaml`); once the keys exist in the file, the environment overrides apply normally. + +Two further causes of a 404 that are unrelated to storage: the API key's group must be on the **OpenAI or Grok** platform (any other platform, or a key with no group at all, yields `Images API is not supported for this platform`), and a task may only be polled with the **same API key that submitted it** — polling with a different key of the same user returns `image task not found` by design. + ## Submit a task ```bash