From b766b981250f9f342a499b7209ddaae32495be3d Mon Sep 17 00:00:00 2001 From: xfy Date: Thu, 11 Jun 2026 15:23:47 +0800 Subject: [PATCH] docs(plans): add bug fix implementation plan --- .../superpowers/plans/2026-06-11-bug-fixes.md | 578 ++++++++++++++++++ 1 file changed, 578 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-11-bug-fixes.md diff --git a/docs/superpowers/plans/2026-06-11-bug-fixes.md b/docs/superpowers/plans/2026-06-11-bug-fixes.md new file mode 100644 index 0000000..b3d06bd --- /dev/null +++ b/docs/superpowers/plans/2026-06-11-bug-fixes.md @@ -0,0 +1,578 @@ +# Lolly 实际问题修复实施计划 + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** 修复 code review 中识别的关键实际问题和风险：配置默认值未应用、状态端点注册条件错误、生产代码中的非 zerolog 日志、以及小技术债。 + +**Architecture:** Task A 和 Task B 有依赖关系：Task A 修复默认值合并后，会让 `Monitoring.Status.Path` 和 `Allow` 变成非空，从而触发 `startSingleMode` 中错误的注册条件（原本用 Path/Allow 判断，应该改为 Enabled）。因此两个修复必须在一个 commit 中或按正确顺序提交。Task C 独立。 + +**Tech Stack:** Go 1.26+, zerolog, fasthttp, yaml.v3 + +--- + +## 文件结构映射 + +### Task A+B: 配置默认值合并 + Monitoring 注册条件修复 + 日志统一 +- **Modify:** `internal/config/config.go` — `Load()` 从 DefaultConfig() 开始 unmarshal +- **Create:** `internal/config/load_test.go` — 默认值合并测试 +- **Modify:** `internal/server/server.go` — monitoring 注册条件改为 `Enabled` 检查 +- **Create:** `internal/server/monitoring_registration_test.go` — 注册条件回归测试 +- **Modify:** `internal/server/status.go` — 替换 4 处 `log.Printf` 为 zerolog +- **Modify:** `internal/lua/api_timer.go` — 替换 4 处 `log.Printf` 为 zerolog + +### Task C: 小技术债清理 +- **Modify:** `internal/proxy/proxy.go` — 删除 `//go:generate go test -v ./...` + +--- + +## Task A+B: 配置默认值合并 + Monitoring 条件修复 + 日志统一 + +### 子任务 A1: 实现 config.Load() 默认值合并 + +**Files:** +- Modify: `internal/config/config.go:131-158` +- Create: `internal/config/load_test.go` + +- [ ] **Step A1.1: 编写失败测试验证当前行为** + +创建 `internal/config/load_test.go`： + +```go +package config + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestLoad_MergesDefaults(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "minimal.yaml") + content := ` +servers: + - listen: ":8080" +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load failed: %v", err) + } + + if cfg.Performance.FileCache.MaxEntries == 0 { + t.Error("Performance.FileCache.MaxEntries should have default value") + } + if cfg.Performance.FileCache.MaxSize == 0 { + t.Error("Performance.FileCache.MaxSize should have default value") + } + if cfg.Performance.FileCache.Inactive == 0 { + t.Error("Performance.FileCache.Inactive should have default value") + } + if cfg.Monitoring.Status.Path != "/_status" { + t.Errorf("Monitoring.Status.Path = %q, want %q", cfg.Monitoring.Status.Path, "/_status") + } + if cfg.Monitoring.Pprof.Path != "/debug/pprof" { + t.Errorf("Monitoring.Pprof.Path = %q, want %q", cfg.Monitoring.Pprof.Path, "/debug/pprof") + } + if cfg.Resolver.CacheTTL == 0 { + t.Error("Resolver.CacheTTL should have default value") + } +} + +func TestLoad_ExplicitOverridesDefault(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "explicit.yaml") + content := ` +performance: + file_cache: + max_entries: 500 + max_size: 52428800 + inactive: 120s +servers: + - listen: ":8080" +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load failed: %v", err) + } + + if cfg.Performance.FileCache.MaxEntries != 500 { + t.Errorf("MaxEntries = %d, want 500", cfg.Performance.FileCache.MaxEntries) + } + if cfg.Performance.FileCache.MaxSize != 52428800 { + t.Errorf("MaxSize = %d, want 52428800", cfg.Performance.FileCache.MaxSize) + } + if cfg.Performance.FileCache.Inactive != 120*time.Second { + t.Errorf("Inactive = %v, want 120s", cfg.Performance.FileCache.Inactive) + } +} + +func TestLoad_MonitoringDisabledByDefault(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "minimal.yaml") + content := ` +servers: + - listen: ":8080" +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load failed: %v", err) + } + + // 默认值 Path 存在，但 Enabled 应为 false + if cfg.Monitoring.Status.Enabled { + t.Error("Monitoring.Status.Enabled should be false by default") + } + if cfg.Monitoring.Pprof.Enabled { + t.Error("Monitoring.Pprof.Enabled should be false by default") + } + if cfg.Monitoring.Status.Path != "/_status" { + t.Errorf("Monitoring.Status.Path = %q, want %q", cfg.Monitoring.Status.Path, "/_status") + } +} +``` + +运行： + +```bash +go test -v -run "TestLoad_" ./internal/config/... +``` + +Expected: FAIL — `TestLoad_MergesDefaults` 和 `TestLoad_MonitoringDisabledByDefault` 失败 + +- [ ] **Step A1.2: 修改 Load() 从 DefaultConfig() 开始** + +修改 `internal/config/config.go`： + +```go +func Load(path string) (*Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("读取配置文件失败: %w", err) + } + + // 从默认值开始，YAML 只覆盖显式配置的字段。 + // 注意：yaml.v3 对 slice 会整体替换，因此用户显式配置的 Servers[] + // 元素不会继承 server-level 默认值；但顶层 struct 字段（Performance、 + // Monitoring、Resolver）的默认值会被保留。 + cfg := DefaultConfig() + if err := yaml.Unmarshal(data, cfg); err != nil { + return nil, fmt.Errorf("解析配置文件失败: %w", err) + } + + if len(cfg.Include) > 0 { + absPath, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("获取配置文件绝对路径失败: %w", err) + } + visited := map[string]bool{absPath: true} + if err := processIncludes(cfg, filepath.Dir(path), 0, visited); err != nil { + return nil, fmt.Errorf("处理配置引入失败: %w", err) + } + } + + if err := Validate(cfg); err != nil { + return nil, fmt.Errorf("配置验证失败: %w", err) + } + + return cfg, nil +} +``` + +- [ ] **Step A1.3: 运行测试确认 A1 修复** + +```bash +go test -v -run "TestLoad_" ./internal/config/... +``` + +Expected: `TestLoad_MergesDefaults` PASS, `TestLoad_ExplicitOverridesDefault` PASS, `TestLoad_MonitoringDisabledByDefault` PASS + +--- + +### 子任务 B1: 修复 monitoring 注册条件（防止 Task A 引入 regression） + +**Files:** +- Modify: `internal/server/server.go:478-490`, `492-508`, `510-520` + +**背景：** `startSingleMode` 当前用 `Path != "" || len(Allow) > 0` 判断是否注册 status handler。Task A 修复后，`Monitoring.Status.Path` 默认值变成 `"/_status"`，`Allow` 默认也有值，这会导致即使用户没有启用 monitoring，handler 也会被注册。必须改为检查 `Enabled`，与 vhost 模式保持一致。 + +- [ ] **Step B1.1: 修改 status handler 注册条件** + +`internal/server/server.go` 第 478-490 行： + +```go +// BEFORE +if s.config.Monitoring.Status.Path != "" || len(s.config.Monitoring.Status.Allow) > 0 { + statusHandler, err := NewStatusHandler(s, &s.config.Monitoring.Status) + ... +} + +// AFTER +if s.config.Monitoring.Status.Enabled { + statusHandler, err := NewStatusHandler(s, &s.config.Monitoring.Status) + ... +} +``` + +- [ ] **Step B1.2: 确认 pprof 和 purge 使用 Enabled 检查** + +pprof 已经是 `if s.config.Monitoring.Pprof.Enabled {`，保持不变。 +purge 已经是 `if serverCfg.CacheAPI != nil && serverCfg.CacheAPI.Enabled {`，保持不变。 + +- [ ] **Step B1.3: 添加注册条件回归测试** + +创建 `internal/server/monitoring_registration_test.go`： + +```go +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/valyala/fasthttp" + "rua.plus/lolly/internal/config" +) + +func TestMonitoringEndpoints_OnlyRegisteredWhenEnabled(t *testing.T) { + // Case 1: monitoring 未启用时，/_status 不应注册 + cfg := &config.Config{ + Servers: []config.ServerConfig{{ + Listen: "127.0.0.1:0", + Static: []config.StaticConfig{{ + Path: "/", + Root: t.TempDir(), + }}, + }}, + } + + srv := New(cfg) + go srv.Start() + defer srv.Stop() + + listeners := srv.GetListeners() + if len(listeners) == 0 { + t.Fatal("server has no listeners") + } + addr := listeners[0].Addr().String() + + client := &fasthttp.Client{} + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) + + req.SetRequestURI("http://" + addr + "/_status") + req.Header.SetMethod("GET") + + if err := client.Do(req, resp); err != nil { + t.Fatalf("request failed: %v", err) + } + + // 未启用时应返回 404（被 static handler 处理，找不到文件） + assert.Equal(t, fasthttp.StatusNotFound, resp.StatusCode(), + "status endpoint should NOT be registered when monitoring is disabled") +} + +func TestMonitoringEndpoints_ReachableWhenEnabled(t *testing.T) { + cfg := &config.Config{ + Monitoring: config.MonitoringConfig{ + Status: config.StatusConfig{ + Enabled: true, + Path: "/_status", + Allow: []string{"127.0.0.1"}, + }, + }, + Servers: []config.ServerConfig{{ + Listen: "127.0.0.1:0", + Static: []config.StaticConfig{{ + Path: "/", + Root: t.TempDir(), + }}, + }}, + } + + srv := New(cfg) + go srv.Start() + defer srv.Stop() + + listeners := srv.GetListeners() + if len(listeners) == 0 { + t.Fatal("server has no listeners") + } + addr := listeners[0].Addr().String() + + client := &fasthttp.Client{} + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) + + req.SetRequestURI("http://" + addr + "/_status") + req.Header.SetMethod("GET") + + if err := client.Do(req, resp); err != nil { + t.Fatalf("request failed: %v", err) + } + + assert.Equal(t, fasthttp.StatusOK, resp.StatusCode(), + "status endpoint should be reachable when enabled, even with static handler on /") +} + +func TestLocationEngine_StatusExactBeatsStaticPrefix(t *testing.T) { + // 独立验证 location engine 的优先级：exact match 应该 beat prefix / + engine := matcher.NewLocationEngine() + + var exactCalled, prefixCalled bool + engine.AddExact("/_status", func(ctx *fasthttp.RequestCtx) { + exactCalled = true + ctx.SetStatusCode(fasthttp.StatusOK) + }, false) + engine.AddPrefix("/", func(ctx *fasthttp.RequestCtx) { + prefixCalled = true + ctx.SetStatusCode(fasthttp.StatusNotFound) + }, false) + engine.MarkInitialized() + + result := engine.Match([]byte("/_status")) + if result == nil { + t.Fatal("expected match") + } + if result.LocationType != matcher.LocationTypeExact { + t.Errorf("expected exact match, got %s", result.LocationType) + } +} +``` + +注意：需要导入 `rua.plus/lolly/internal/matcher`。 + +- [ ] **Step B1.4: 运行测试** + +```bash +go test -v -run "TestMonitoringEndpoints|TestLocationEngine" ./internal/server/... +``` + +Expected: +- `TestMonitoringEndpoints_OnlyRegisteredWhenEnabled` PASS（返回 404） +- `TestMonitoringEndpoints_ReachableWhenEnabled` PASS（返回 200） +- `TestLocationEngine_StatusExactBeatsStaticPrefix` PASS + +--- + +### 子任务 B2: 替换生产代码中的 log.Printf + +**Files:** +- Modify: `internal/server/status.go` +- Modify: `internal/lua/api_timer.go` + +- [ ] **Step B2.1: 替换 status.go 中的 log.Printf** + +修改 `internal/server/status.go`： +- 删除 `"log"` import +- 添加 `"rua.plus/lolly/internal/logging"` import（如果不存在） + +替换 4 处： + +```go +// 约 302 行 +logging.Error().Err(err).Msg("failed to write metrics response") + +// 约 318 行 +logging.Error().Err(err).Msg("failed to write status response") + +// 约 375 行 +logging.Error().Err(err).Msg("failed to write text response") + +// 约 480 行 +logging.Error().Err(err).Msg("failed to write html response") +``` + +- [ ] **Step B2.2: 替换 lua/api_timer.go 中的 log.Printf** + +修改 `internal/lua/api_timer.go`： +- 删除 `"log"` import +- 添加 `"rua.plus/lolly/internal/logging"` import + +替换 4 处： + +```go +// 约 264 行 +logging.Warn().Msg("[lua] timer callback dropped: queue full") + +// 约 280 行 +logging.Error().Msg("[lua] timer callback: failed to create function from proto") + +// 约 289 行 +logging.Error().Err(err).Msg("[lua] timer callback error") + +// 约 408 行 +logging.Warn().Int("abandoned", abandoned).Msg("[lua] shutdown timeout: callbacks abandoned") +``` + +- [ ] **Step B2.3: 运行测试** + +```bash +go test -v ./internal/server/... ./internal/lua/... +``` + +Expected: PASS + +--- + +### 子任务 A+B 提交 + +Task A 和 Task B1 必须一起提交，否则单独提交 A 会引入 regression。 + +```bash +git add internal/config/config.go internal/config/load_test.go +-git add internal/server/server.go internal/server/monitoring_registration_test.go +-git add internal/server/status.go internal/lua/api_timer.go +git commit -m "fix(config,server): merge defaults on Load and fix monitoring registration + +Two related fixes that must land together: + +1. config.Load() now starts from DefaultConfig() before unmarshaling + YAML. This ensures missing top-level fields (Performance, + Monitoring, Resolver) use their documented defaults instead of + zero values. Most importantly, file_cache is no longer silently + disabled when users omit the performance: section. + +2. startSingleMode() now checks Monitoring.Status.Enabled instead of + Path/Allow to decide whether to register the status endpoint. + Without this change, fix #1 would have caused a regression where + the status handler is registered even when monitoring is disabled, + because DefaultConfig() sets Path and Allow defaults. + +Also replace remaining log.Printf in status.go and lua/api_timer.go +with zerolog to follow project logging conventions. + +Added tests: +- config/load_test.go: verifies defaults are applied, explicit values + override defaults, and monitoring stays disabled by default. +- server/monitoring_registration_test.go: verifies /_status is only + registered when enabled and remains reachable with static handler + on path: /." +``` + +--- + +## Task C: 清理 proxy.go 的 go:generate + +**Files:** +- Modify: `internal/proxy/proxy.go:31` + +- [ ] **Step C1: 删除不合理的 go:generate** + +删除 `internal/proxy/proxy.go` 第 31 行： + +```go +//go:generate go test -v ./... +``` + +- [ ] **Step C2: 运行测试** + +```bash +go test ./internal/proxy/... +``` + +Expected: PASS + +- [ ] **Step C3: Commit Task C** + +```bash +git add internal/proxy/proxy.go +git commit -m "chore(proxy): remove misplaced go:generate directive + +The //go:generate go test -v ./... directive is not what +go:generate is intended for (code generation, not test execution). +Remove it to avoid confusion." +``` + +--- + +## Task D: 最终验证 + +- [ ] **Step D1: 运行全量测试** + +```bash +go test ./internal/... -count=1 2>&1 | tail -30 +``` + +Expected: 除已知 integration DNS 测试外全部 PASS + +- [ ] **Step D2: 运行 race detector** + +```bash +go test -race ./internal/config/... ./internal/server/... ./internal/lua/... ./internal/proxy/... -count=1 +``` + +Expected: 零 race + +- [ ] **Step D3: 运行 linter** + +```bash +make lint +``` + +Expected: 0 issues + +- [ ] **Step D4: 检查无 log.Printf 残留** + +```bash +grep -rn "^[[:space:]]*log\.Printf\|^[[:space:]]*log\.Println\|^[[:space:]]*log\.Fatal" internal/ --include='*.go' | grep -v '_test.go' | grep -v '//' +``` + +Expected: 空（`app/import.go` 中的 `fmt.Printf` 是 CLI 输出，允许保留） + +- [ ] **Step D5: 运行 build** + +```bash +make build +``` + +Expected: 构建成功 + +--- + +## Spec Coverage Check + +| 发现的问题 | 对应修复 | +|-----------|---------| +| config.Load() 不合并 DefaultConfig() | Task A: Load() 从 DefaultConfig() 开始 unmarshal | +| Task A 会引入 monitoring Enabled 回归 | Task B1: 注册条件改为 Enabled | +| status 端点未启用时返回 404 / 启用时不工作 | Task B1: Enabled 条件 + 回归测试 | +| server/status.go 使用 log.Printf | Task B2: 替换为 zerolog | +| lua/api_timer.go 使用 log.Printf | Task B2: 替换为 zerolog | +| proxy.go 的 go:generate 不合理 | Task C: 删除 | + +--- + +## 附录：常用验证命令 + +```bash +# 关键测试 +go test -v -run "TestLoad_" ./internal/config/... +go test -v -run "TestMonitoringEndpoints|TestLocationEngine" ./internal/server/... + +# 相关包 +go test ./internal/config/... ./internal/server/... ./internal/lua/... ./internal/proxy/... + +# 全量 +go test ./internal/... -count=1 + +# Lint + Build +make lint +make build +```